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

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

[mhun512]

Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
vudc_remove due to race condition") deletes the timer before
usb_del_gadget_udc() stops the receive thread.  v_kick_timer() can rearm
it even in VUDC_TR_STOPPED, leaving v_timer() to use freed vudc.

Use timer_shutdown_sync() to reject later rearms.  Replace the
inaccurate blanket lock comment with __must_hold(&udc->lock) on
v_start_timer() and v_kick_timer(); v_init_timer() and v_stop_timer() run
unlocked.

A KASAN/DEBUG_OBJECTS_TIMERS x86_64 QEMU harness binds g_zero to
usbip-vudc.0, sends CMD_SUBMIT to usbip_sockfd via a socketpair, and
repeatedly unbinds/rebinds vudc.  The unpatched kernel reported a
free-active timer and a use-after-free in v_timer(); the patched kernel
ran 4000 iterations without either report.  No physical device or
remote client was tested.

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 v3:
- Describe the harness, test scope and lock annotations (Shuah Khan).

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 v3] usbip: vudc: Prevent transfer timer rearm during teardown

[Greg KH]

On Thu, Sep 24, 2026 at 09:51:44PM +0000, mhun512@gmail.com wrote:
> Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
> vudc_remove due to race condition") deletes the timer before
> usb_del_gadget_udc() stops the receive thread.  v_kick_timer() can rearm
> it even in VUDC_TR_STOPPED, leaving v_timer() to use freed vudc.
> 
> Use timer_shutdown_sync() to reject later rearms.  Replace the
> inaccurate blanket lock comment with __must_hold(&udc->lock) on
> v_start_timer() and v_kick_timer(); v_init_timer() and v_stop_timer() run
> unlocked.

That is two different things you are doing here, so this should be a
patch series of 2 patches, right?

thanks,

greg k-h

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

[Greg KH]

On Thu, Sep 24, 2026 at 09:51:44PM +0000, mhun512@gmail.com wrote:
> Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
> vudc_remove due to race condition") deletes the timer before
> usb_del_gadget_udc() stops the receive thread.  v_kick_timer() can rearm
> it even in VUDC_TR_STOPPED, leaving v_timer() to use freed vudc.
> 
> Use timer_shutdown_sync() to reject later rearms.  Replace the
> inaccurate blanket lock comment with __must_hold(&udc->lock) on
> v_start_timer() and v_kick_timer(); v_init_timer() and v_stop_timer() run
> unlocked.
> 
> A KASAN/DEBUG_OBJECTS_TIMERS x86_64 QEMU harness binds g_zero to
> usbip-vudc.0, sends CMD_SUBMIT to usbip_sockfd via a socketpair, and
> repeatedly unbinds/rebinds vudc.  The unpatched kernel reported a
> free-active timer and a use-after-free in v_timer(); the patched kernel
> ran 4000 iterations without either report.  No physical device or
> remote client was tested.
> 
> Fixes: d96209626a29 ("usbip: vudc: Fix use after free bug in
> vudc_remove due to race condition")

Do not line wrap.

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

[Shuah Khan]

On 9/24/26 15:51, mhun512@gmail.com wrote:
> Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
> vudc_remove due to race condition") deletes the timer before
> usb_del_gadget_udc() stops the receive thread.  v_kick_timer() can rearm
> it even in VUDC_TR_STOPPED, leaving v_timer() to use freed vudc.
> 
> Use timer_shutdown_sync() to reject later rearms.  Replace the
> inaccurate blanket lock comment with __must_hold(&udc->lock) on
> v_start_timer() and v_kick_timer(); v_init_timer() and v_stop_timer() run
> unlocked.
> 
> A KASAN/DEBUG_OBJECTS_TIMERS x86_64 QEMU harness binds g_zero to
> usbip-vudc.0, sends CMD_SUBMIT to usbip_sockfd via a socketpair, and
> repeatedly unbinds/rebinds vudc.  The unpatched kernel reported a
> free-active timer and a use-after-free in v_timer(); the patched kernel
> ran 4000 iterations without either report.  No physical device or
> remote client was tested.
> > Fixes: d96209626a29 ("usbip: vudc: Fix use after free bug in
> vudc_remove due to race condition")

If you haven't tested this physical devices and a remote client, how
do you know this change fixes the problem. Sorry I can't accept this
patch.

thanks,
-- Shuah