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

[Michal Pecio <michal.pecio@gmail.com>]

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@syzkaller.appspotmail.com
Link: https://lore.kernel.org/linux-usb/6a8a5047.dbb3a75c.13dd47.003e.GAE@google.com/
Reported-by: syzbot+a5b220cacb0618384d5a@syzkaller.appspotmail.com
Link: https://lore.kernel.org/linux-usb/6abb3515.c6a7fab7.e5ea7.0459.GAE@google.com/
Fixes: 8fea0c8fda30129b ("usb: core: hcd: Convert from tasklet to BH workqueue")
Cc: stable@vger.kernel.org
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---

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@gmail.com/

 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