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

[Shuah Khan <skhan@linuxfoundation.org>]

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