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 Signed-off-by: Ijae Kim Signed-off-by: Myeonghun Pak --- 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