Re: [PATCH v2] usb: hcd: Cancel BH giveback works on removal

[Alan Stern]

On Fri, Oct 30, 2026 at 07:21:29AM +0100, Michal Pecio wrote:
> Turns out, we do actually need to flush them, because workers use the
> 'high_prio_bh' and 'low_prio_bh' members of 'usb_hcd' for a brief time
> after all URBs are completed, to track pending completions and possibly
> reschedule themselves - see usb_giveback_urb_bh() implementation.
> 
> Flushing would suffice if the works don't reschedule themselves, but
> cancel_work_sync() is more robust against stray completions.
> 
> Syzbot hit this issues twice, but not reproducibly yet. I needed a 3ms
> delay in usb_giveback_urb_bh() to get reliable UAF after unbinding an
> HCD. The works run in soft IRQ, so they can be preempted by hard IRQs,
> on PREEMPT_RT, or in a VM. The latter in particular seems to offer an
> opportunity for "unlimited" delays and may have affected syzbot.
> 
> HCD unbinding can occur on hot removal of Thunderbolt peripherals like
> docks and monitors which contain xHCI controllers.
> 
> Reported-by: syzbot+cade843a1e4af0651f5e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/linux-usb/6a8a5047.dbb3a75c.13dd47.003e.GAE@xxxxxxxxxx/
> Reported-by: syzbot+a5b220cacb0618384d5a@xxxxxxxxxxxxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/linux-usb/6abb3515.c6a7fab7.e5ea7.0459.GAE@xxxxxxxxxx/
> Fixes: 8fea0c8fda30129b ("usb: core: hcd: Convert from tasklet to BH workqueue")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Michal Pecio <michal.pecio@xxxxxxxxx>
> ---
> 
> v2:
> - changed "Fixes" as per Alan's suggestion
> - clarified the comment as per Alan's suggestion in other thread,
>   let's see if this will pacify the review bot
> - another syzbot report
> - more elaborate/plausible speculation on how syzbot hits this
> 
> v1:
> https://lore.kernel.org/linux-usb/20260823125831.6ea35650.michal.pecio@xxxxxxxxx/

Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>

>  drivers/usb/core/hcd.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index ee19628cd653..88f88e7ba464 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -3053,14 +3053,13 @@ void usb_remove_hcd(struct usb_hcd *hcd)
>  	mutex_unlock(&usb_bus_idr_lock);
>  
>  	/*
> -	 * flush_work() isn't needed here because:
> -	 * - driver's disconnect() called from usb_disconnect() should
> -	 *   make sure its URBs are completed during the disconnect()
> -	 *   callback
> -	 *
> -	 * - it is too late to run complete() here since driver may have
> -	 *   been removed already now
> +	 * Hopefully no complete() callbacks are running anymore; disconnect()
> +	 * routines should have waited for them to prevent UAF of driver data.
> +	 * But we must wait for workers to finish accessing the hcd which is
> +	 * about to be freed (and shamelessly drop any remaining completions).
>  	 */
> +	cancel_work_sync(&hcd->high_prio_bh.bh);
> +	cancel_work_sync(&hcd->low_prio_bh.bh);
>  
>  	/* Prevent any more root-hub status calls from the timer.
>  	 * The HCD might still restart the timer (if a port status change
> -- 
> 2.48.1