[v1] usbip: vudc: Prevent transfer timer rearm during teardown

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

Posted by Myeonghun Pak 2 days, 18 hours ago

Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
vudc_remove due to race condition") made v_stop_timer() delete the
transfer timer synchronously before vudc_remove() frees its containing
struct vudc.

That does not close the race with the receive thread, which is stopped
later through usb_del_gadget_udc() and vudc_shutdown().  The thread
calls v_kick_timer() for CMD_SUBMIT and CMD_UNLINK packets, and
v_kick_timer() deliberately calls mod_timer() when the transfer state is
VUDC_TR_STOPPED.  A packet received after v_stop_timer() returns can
therefore enqueue the timer again before the vudc is freed.

timer_delete_sync() only drains the current timer instance and cannot
prevent such a rearm.  Use timer_shutdown_sync(), which makes subsequent
attempts to arm the timer no-ops.  The transfer state no longer needs to
be updated, as it is freed along with the timer.

On a KASAN and DEBUG_OBJECTS enabled x86_64 QEMU guest, repeatedly
binding the device, submitting a request and unbinding it produced a
"free active timer_list" warning followed by a slab-use-after-free in
v_timer().  With this change the same harness completed 4000 iterations
cleanly.

This issue was identified during our ongoing static-analysis research
while reviewing kernel code.

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>
---
 drivers/usb/usbip/vudc_transfer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
index d4ce85c4c6a2..4146b746a320 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -441,7 +441,7 @@ static void v_timer(struct timer_list *t)
 	spin_unlock_irqrestore(&udc->lock, flags);
 }
 
-/* All timer functions are run with udc->lock held */
+/* v_start_timer() and v_kick_timer() are called with udc->lock held. */
 
 void v_init_timer(struct vudc *udc)
 {
@@ -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] usbip: vudc: Prevent transfer timer rearm during teardown

Posted by Shuah Khan 2 days, 7 hours ago

On 9/21/26 16:41, Myeonghun Pak wrote:
> Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
> vudc_remove due to race condition") made v_stop_timer() delete the
> transfer timer synchronously before vudc_remove() frees its containing
> struct vudc.
> 
> That does not close the race with the receive thread, which is stopped
> later through usb_del_gadget_udc() and vudc_shutdown().  The thread
> calls v_kick_timer() for CMD_SUBMIT and CMD_UNLINK packets, and
> v_kick_timer() deliberately calls mod_timer() when the transfer state is
> VUDC_TR_STOPPED.  A packet received after v_stop_timer() returns can
> therefore enqueue the timer again before the vudc is freed.
> 
> timer_delete_sync() only drains the current timer instance and cannot
> prevent such a rearm.  Use timer_shutdown_sync(), which makes subsequent
> attempts to arm the timer no-ops.  The transfer state no longer needs to
> be updated, as it is freed along with the timer.
> 
> On a KASAN and DEBUG_OBJECTS enabled x86_64 QEMU guest, repeatedly
> binding the device, submitting a request and unbinding it produced a
> "free active timer_list" warning followed by a slab-use-after-free in
> v_timer().  With this change the same harness completed 4000 iterations
> cleanly.
> 
> This issue was identified during our ongoing static-analysis research
> while reviewing kernel code.

Were you able to reproduce this problem to test and verify that this patch
fixes it?

> 
> 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>
> ---
>   drivers/usb/usbip/vudc_transfer.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
> index d4ce85c4c6a2..4146b746a320 100644
> --- a/drivers/usb/usbip/vudc_transfer.c
> +++ b/drivers/usb/usbip/vudc_transfer.c
> @@ -441,7 +441,7 @@ static void v_timer(struct timer_list *t)
>   	spin_unlock_irqrestore(&udc->lock, flags);
>   }
>   
> -/* All timer functions are run with udc->lock held */
> +/* v_start_timer() and v_kick_timer() are called with udc->lock held. */
>   
>   void v_init_timer(struct vudc *udc)
>   {
> @@ -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

thanks,
-- Shuah

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

Posted by Myeonghun Pak 1 day, 14 hours ago

Yes. On an x86_64 QEMU guest running v7.2 with KASAN and
DEBUG_OBJECTS_TIMERS, the harness binds usbip-vudc.0, sends
USBIP_CMD_SUBMIT while unbinding it, and repeats.

Without this change that hits the free-active timer warning and a
KASAN use-after-free in v_timer(). With timer_shutdown_sync(), the
same harness completed 4000 iterations with no KASAN, ODEBUG, or
WARNING. v2 does not change that timer fix.

thanks,

Myeonghun Pak

2026년 9월 22일 (화) 오전 5:47, Shuah Khan <skhan@linuxfoundation.org>님이 작성:
>
> On 9/21/26 16:41, Myeonghun Pak wrote:
> > Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
> > vudc_remove due to race condition") made v_stop_timer() delete the
> > transfer timer synchronously before vudc_remove() frees its containing
> > struct vudc.
> >
> > That does not close the race with the receive thread, which is stopped
> > later through usb_del_gadget_udc() and vudc_shutdown().  The thread
> > calls v_kick_timer() for CMD_SUBMIT and CMD_UNLINK packets, and
> > v_kick_timer() deliberately calls mod_timer() when the transfer state is
> > VUDC_TR_STOPPED.  A packet received after v_stop_timer() returns can
> > therefore enqueue the timer again before the vudc is freed.
> >
> > timer_delete_sync() only drains the current timer instance and cannot
> > prevent such a rearm.  Use timer_shutdown_sync(), which makes subsequent
> > attempts to arm the timer no-ops.  The transfer state no longer needs to
> > be updated, as it is freed along with the timer.
> >
> > On a KASAN and DEBUG_OBJECTS enabled x86_64 QEMU guest, repeatedly
> > binding the device, submitting a request and unbinding it produced a
> > "free active timer_list" warning followed by a slab-use-after-free in
> > v_timer().  With this change the same harness completed 4000 iterations
> > cleanly.
> >
> > This issue was identified during our ongoing static-analysis research
> > while reviewing kernel code.
>
> Were you able to reproduce this problem to test and verify that this patch
> fixes it?
>
> >
> > 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>
> > ---
> >   drivers/usb/usbip/vudc_transfer.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
> > index d4ce85c4c6a2..4146b746a320 100644
> > --- a/drivers/usb/usbip/vudc_transfer.c
> > +++ b/drivers/usb/usbip/vudc_transfer.c
> > @@ -441,7 +441,7 @@ static void v_timer(struct timer_list *t)
> >       spin_unlock_irqrestore(&udc->lock, flags);
> >   }
> >
> > -/* All timer functions are run with udc->lock held */
> > +/* v_start_timer() and v_kick_timer() are called with udc->lock held. */
> >
> >   void v_init_timer(struct vudc *udc)
> >   {
> > @@ -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
>
> thanks,
> -- Shuah
>

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

Posted by Greg Kroah-Hartman 2 days, 11 hours ago

On Mon, Sep 21, 2026 at 06:41:30PM -0400, Myeonghun Pak wrote:
> Commit d96209626a29 ("usbip: vudc: Fix use after free bug in
> vudc_remove due to race condition") made v_stop_timer() delete the
> transfer timer synchronously before vudc_remove() frees its containing
> struct vudc.
> 
> That does not close the race with the receive thread, which is stopped
> later through usb_del_gadget_udc() and vudc_shutdown().  The thread
> calls v_kick_timer() for CMD_SUBMIT and CMD_UNLINK packets, and
> v_kick_timer() deliberately calls mod_timer() when the transfer state is
> VUDC_TR_STOPPED.  A packet received after v_stop_timer() returns can
> therefore enqueue the timer again before the vudc is freed.
> 
> timer_delete_sync() only drains the current timer instance and cannot
> prevent such a rearm.  Use timer_shutdown_sync(), which makes subsequent
> attempts to arm the timer no-ops.  The transfer state no longer needs to
> be updated, as it is freed along with the timer.
> 
> On a KASAN and DEBUG_OBJECTS enabled x86_64 QEMU guest, repeatedly
> binding the device, submitting a request and unbinding it produced a
> "free active timer_list" warning followed by a slab-use-after-free in
> v_timer().  With this change the same harness completed 4000 iterations
> cleanly.
> 
> This issue was identified during our ongoing static-analysis research
> while reviewing kernel code.
> 
> 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>
> ---
>  drivers/usb/usbip/vudc_transfer.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vudc_transfer.c b/drivers/usb/usbip/vudc_transfer.c
> index d4ce85c4c6a2..4146b746a320 100644
> --- a/drivers/usb/usbip/vudc_transfer.c
> +++ b/drivers/usb/usbip/vudc_transfer.c
> @@ -441,7 +441,7 @@ static void v_timer(struct timer_list *t)
>  	spin_unlock_irqrestore(&udc->lock, flags);
>  }
>  
> -/* All timer functions are run with udc->lock held */
> +/* v_start_timer() and v_kick_timer() are called with udc->lock held. */

If this is true, use the correct documentation/metadata style for this,
so that checkers can catch this.  A comment like this will not work.

thanks,

greg k-h