[PATCH v2] usbip: vudc: Prevent transfer timer rearm during teardown

[PATCH v2] usbip: vudc: Prevent transfer timer rearm during teardown

[Myeonghun Pak]

Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
vudc_remove due to race condition") deletes the transfer timer before
vudc_remove() frees the vudc, but the receive thread keeps running
until usb_del_gadget_udc().  v_kick_timer() calls mod_timer() for
CMD_SUBMIT and CMD_UNLINK even in VUDC_TR_STOPPED, so a packet after
timer_delete_sync() rearms the timer and v_timer() uses the freed
vudc.

Use timer_shutdown_sync(), which ignores later arming.  The transfer
state is freed with the timer.  Annotate v_start_timer() and
v_kick_timer() with __must_hold(&udc->lock).

A KASAN and DEBUG_OBJECTS x86_64 QEMU guest reproduced the free-active
timer warning and the use-after-free in v_timer().  The same harness
completed 4000 iterations with this change.

Fixes: d96209626a29 ("usbip: vudc: Fix use after free bug in vudc_remove due to race condition")
Link: https://lore.kernel.org/all/20230316180940.1601515-1-zyytlz.wz@163.com/
Cc: stable@vger.kernel.org
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Changes in v2:
- Drop the lock-held comment and mark v_start_timer() and
  v_kick_timer() with __must_hold(&udc->lock), on the prototypes and
  the definitions (Greg Kroah-Hartman).

 drivers/usb/usbip/vudc.h          | 6 ++++--
 drivers/usb/usbip/vudc_transfer.c | 8 +++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/usbip/vudc.h b/drivers/usb/usbip/vudc.h
index 5ef0e7d9b23a..c129f4c56e8a 100644
--- a/drivers/usb/usbip/vudc.h
+++ b/drivers/usb/usbip/vudc.h
@@ -157,8 +157,10 @@ int v_rx_loop(void *data);
 /* vudc_transfer.c */
 
 void v_init_timer(struct vudc *udc);
-void v_start_timer(struct vudc *udc);
-void v_kick_timer(struct vudc *udc, unsigned long time);
+void v_start_timer(struct vudc *udc)
+	__must_hold(&udc->lock);
+void v_kick_timer(struct vudc *udc, unsigned long time)
+	__must_hold(&udc->lock);
 void v_stop_timer(struct vudc *udc);
 
 /* vudc_dev.c */
diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
index d4ce85c4c6a2..ba625622975e 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -441,8 +441,6 @@ static void v_timer(struct timer_list *t)
 	spin_unlock_irqrestore(&udc->lock, flags);
 }
 
-/* All timer functions are run with udc->lock held */
-
 void v_init_timer(struct vudc *udc)
 {
 	struct transfer_timer *t = &udc->tr_timer;
@@ -452,6 +450,7 @@ void v_init_timer(struct vudc *udc)
 }
 
 void v_start_timer(struct vudc *udc)
+	__must_hold(&udc->lock)
 {
 	struct transfer_timer *t = &udc->tr_timer;
 
@@ -470,6 +469,7 @@ void v_start_timer(struct vudc *udc)
 }
 
 void v_kick_timer(struct vudc *udc, unsigned long time)
+	__must_hold(&udc->lock)
 {
 	struct transfer_timer *t = &udc->tr_timer;
 
@@ -490,8 +490,6 @@ void v_stop_timer(struct vudc *udc)
 {
 	struct transfer_timer *t = &udc->tr_timer;
 
-	/* 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;
+	timer_shutdown_sync(&t->timer);
 }

base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
-- 
2.53.0

Re: [PATCH v2] usbip: vudc: Prevent transfer timer rearm during teardown

[Shuah Khan]

On 9/22/26 20:20, Myeonghun Pak wrote:
> Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
> vudc_remove due to race condition") deletes the transfer timer before
> vudc_remove() frees the vudc, but the receive thread keeps running
> until usb_del_gadget_udc().  v_kick_timer() calls mod_timer() for
> CMD_SUBMIT and CMD_UNLINK even in VUDC_TR_STOPPED, so a packet after
> timer_delete_sync() rearms the timer and v_timer() uses the freed
> vudc.
> 
> Use timer_shutdown_sync(), which ignores later arming.  The transfer
> state is freed with the timer.  Annotate v_start_timer() and
> v_kick_timer() with __must_hold(&udc->lock).

Can you explain why __must_hold(&udc->lock) necessary?

> 
> A KASAN and DEBUG_OBJECTS x86_64 QEMU guest reproduced the free-active
> timer warning and the use-after-free in v_timer().  The same harness
> completed 4000 iterations with this change.__must_hold(&udc->lock)

What does the harness do? You mentioned it in a response, but I would
like to see this in the change log.

> 
> Fixes: d96209626a29 ("usbip: vudc: Fix use after free bug in vudc_remove due to race condition")
> Link: https://lore.kernel.org/all/20230316180940.1601515-1-zyytlz.wz@163.com/
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
> Changes in v2:
> - Drop the lock-held comment and mark v_start_timer() and
>    v_kick_timer() with __must_hold(&udc->lock), on the prototypes and
>    the definitions (Greg Kroah-Hartman).
> 
>   drivers/usb/usbip/vudc.h          | 6 ++++--
>   drivers/usb/usbip/vudc_transfer.c | 8 +++-----
>   2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vudc.h b/drivers/usb/usbip/vudc.h
> index 5ef0e7d9b23a..c129f4c56e8a 100644
> --- a/drivers/usb/usbip/vudc.h
> +++ b/drivers/usb/usbip/vudc.h
> @@ -157,8 +157,10 @@ int v_rx_loop(void *data);
>   /* vudc_transfer.c */
>   
>   void v_init_timer(struct vudc *udc);
> -void v_start_timer(struct vudc *udc);
> -void v_kick_timer(struct vudc *udc, unsigned long time);
> +void v_start_timer(struct vudc *udc)
> +	__must_hold(&udc->lock);
> +void v_kick_timer(struct vudc *udc, unsigned long time)
> +	__must_hold(&udc->lock);
>   void v_stop_timer(struct vudc *udc);
>   
>   /* vudc_dev.c */
> diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
> index d4ce85c4c6a2..ba625622975e 100644
> --- a/drivers/usb/usbip/vudc_transfer.c
> +++ b/drivers/usb/usbip/vudc_transfer.c
> @@ -441,8 +441,6 @@ static void v_timer(struct timer_list *t)
>   	spin_unlock_irqrestore(&udc->lock, flags);
>   }
>   
> -/* All timer functions are run with udc->lock held */
> -
>   void v_init_timer(struct vudc *udc)
>   {
>   	struct transfer_timer *t = &udc->tr_timer;
> @@ -452,6 +450,7 @@ void v_init_timer(struct vudc *udc)
>   }
>   
>   void v_start_timer(struct vudc *udc)
> +	__must_hold(&udc->lock)
>   {
>   	struct transfer_timer *t = &udc->tr_timer;
>   
> @@ -470,6 +469,7 @@ void v_start_timer(struct vudc *udc)
>   }
>   
>   void v_kick_timer(struct vudc *udc, unsigned long time)
> +	__must_hold(&udc->lock)
>   {
>   	struct transfer_timer *t = &udc->tr_timer;
>   
> @@ -490,8 +490,6 @@ void v_stop_timer(struct vudc *udc)
>   {
>   	struct transfer_timer *t = &udc->tr_timer;
>   
> -	/* 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;
> +	timer_shutdown_sync(&t->timer);
>   }
> 
> base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5

Have you tested this with real devices bound to a host and exported
to a client?

thanks,
-- Shuah