[PATCH] usbip: vudc: Fix use after free bug in vudc_remove due to race condition

Michael Bommarito posted 1 patch 1 month, 4 weeks ago
There is a newer version of this series
drivers/usb/usbip/vudc_dev.c      | 1 +
drivers/usb/usbip/vudc_transfer.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
[PATCH] usbip: vudc: Fix use after free bug in vudc_remove due to race condition
Posted by Michael Bommarito 1 month, 4 weeks ago
This patch follows up Zheng Wang's 2023 report of a use-after-free in
vudc_remove(). The original thread stalled on Shuah Khan's request for
runtime testing of the unplug/unbind path. This patch supplies that
testing and keeps Zheng's original fix shape.

In vudc_probe(), v_init_timer() binds udc->tr_timer.timer to v_timer().
usbip_sockfd_store() starts the timer via v_start_timer()/v_kick_timer().
vudc_remove() can then free the containing struct vudc while the timer is
still pending or executing.

KASAN confirms the race on an unpatched x86_64 QEMU guest with
CONFIG_KASAN=y, CONFIG_USBIP_VUDC=y, CONFIG_USB_ZERO=y, and a tight loop
that repeatedly writes a socket fd to usbip_sockfd, closes the socket
pair, and unbinds/rebinds usbip-vudc.0:

  BUG: KASAN: slab-use-after-free in __run_timer_base.part.0+0x8ba/0x8e0
  Write of size 8 at addr ffff888001b80740 by task trigger_and_unb/239
  Allocated by task 239:
    vudc_probe+0x4d/0xaa0
  Freed by task 239:
    kfree+0x18f/0x520
    device_release_driver_internal+0x388/0x540
    unbind_store+0xd9/0x100

This lands in the timer core rather than v_timer() itself because the
embedded timer_list is being walked after its containing struct vudc has
already been freed. The underlying lifetime bug is the same one Zheng
reported.

With v_stop_timer() called from vudc_remove() and the timer deleted
synchronously, the same harness completed 5000 bind/unbind iterations
with no KASAN report.

Fixes: b6a0ca111867 ("usbip: vudc: Add UDC specific ops")
Reported-by: Zheng Wang <zyytlz.wz@163.com>
Closes: https://lore.kernel.org/linux-usb/20230317100954.2626573-1-zyytlz.wz@163.com/
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/usb/usbip/vudc_dev.c      | 1 +
 drivers/usb/usbip/vudc_transfer.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 90383107b660..c5f079c5a1ea 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -632,6 +632,7 @@ void vudc_remove(struct platform_device *pdev)
 {
 	struct vudc *udc = platform_get_drvdata(pdev);
 
+	v_stop_timer(udc);
 	usb_del_gadget_udc(&udc->gadget);
 	cleanup_vudc_hw(udc);
 	kfree(udc);
diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
index a4f02ea3e3ef..d4ce85c4c6a2 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -490,7 +490,8 @@ void v_stop_timer(struct vudc *udc)
 {
 	struct transfer_timer *t = &udc->tr_timer;
 
-	/* timer itself will take care of stopping */
+	/* Delete the timer synchronously before teardown frees udc. */
 	dev_dbg(&udc->pdev->dev, "timer stop");
+	timer_delete_sync(&t->timer);
 	t->state = VUDC_TR_STOPPED;
 }
-- 
2.53.0
Re: [PATCH] usbip: vudc: Fix use after free bug in vudc_remove due to race condition
Posted by Shuah Khan 1 month, 1 week ago
On 4/17/26 10:35, Michael Bommarito wrote:
> This patch follows up Zheng Wang's 2023 report of a use-after-free in
> vudc_remove(). The original thread stalled on Shuah Khan's request for
> runtime testing of the unplug/unbind path. This patch supplies that
> testing and keeps Zheng's original fix shape.
> 
> In vudc_probe(), v_init_timer() binds udc->tr_timer.timer to v_timer().
> usbip_sockfd_store() starts the timer via v_start_timer()/v_kick_timer().
> vudc_remove() can then free the containing struct vudc while the timer is
> still pending or executing.
> 
> KASAN confirms the race on an unpatched x86_64 QEMU guest with
> CONFIG_KASAN=y, CONFIG_USBIP_VUDC=y, CONFIG_USB_ZERO=y, and a tight loop
> that repeatedly writes a socket fd to usbip_sockfd, closes the socket
> pair, and unbinds/rebinds usbip-vudc.0:
> 
>    BUG: KASAN: slab-use-after-free in __run_timer_base.part.0+0x8ba/0x8e0
>    Write of size 8 at addr ffff888001b80740 by task trigger_and_unb/239
>    Allocated by task 239:
>      vudc_probe+0x4d/0xaa0
>    Freed by task 239:
>      kfree+0x18f/0x520
>      device_release_driver_internal+0x388/0x540
>      unbind_store+0xd9/0x100
> 
> This lands in the timer core rather than v_timer() itself because the
> embedded timer_list is being walked after its containing struct vudc has
> already been freed. The underlying lifetime bug is the same one Zheng
> reported.
> 
> With v_stop_timer() called from vudc_remove() and the timer deleted
> synchronously, the same harness completed 5000 bind/unbind iterations
> with no KASAN report.
> 
> Fixes: b6a0ca111867 ("usbip: vudc: Add UDC specific ops")
> Reported-by: Zheng Wang <zyytlz.wz@163.com>
> Closes: https://lore.kernel.org/linux-usb/20230317100954.2626573-1-zyytlz.wz@163.com/
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
>   drivers/usb/usbip/vudc_dev.c      | 1 +
>   drivers/usb/usbip/vudc_transfer.c | 3 ++-
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
> index 90383107b660..c5f079c5a1ea 100644
> --- a/drivers/usb/usbip/vudc_dev.c
> +++ b/drivers/usb/usbip/vudc_dev.c
> @@ -632,6 +632,7 @@ void vudc_remove(struct platform_device *pdev)
>   {
>   	struct vudc *udc = platform_get_drvdata(pdev);
>   
> +	v_stop_timer(udc);
>   	usb_del_gadget_udc(&udc->gadget);
>   	cleanup_vudc_hw(udc);
>   	kfree(udc);
> diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
> index a4f02ea3e3ef..d4ce85c4c6a2 100644
> --- a/drivers/usb/usbip/vudc_transfer.c
> +++ b/drivers/usb/usbip/vudc_transfer.c
> @@ -490,7 +490,8 @@ void v_stop_timer(struct vudc *udc)
>   {
>   	struct transfer_timer *t = &udc->tr_timer;
>   
> -	/* timer itself will take care of stopping */
> +	/* Delete the timer synchronously before teardown frees udc. */
>   	dev_dbg(&udc->pdev->dev, "timer stop");
> +	timer_delete_sync(&t->timer);
>   	t->state = VUDC_TR_STOPPED;
>   }


Looks good to me.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah