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

[Michal Pecio]

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 may have found the issue due to unlucky hard IRQ timing. It can
be reproduced by adding udelay(3000) in the work function, disabling RH
autosuspend to maintain the status URB and unbinding a real HC:

[10818.828029] ehci-pci 0000:00:12.0: USB bus 1 deregistered
[10818.828077] hcd_release freeing high_prio_bh ffff88814a950978
[10818.829211] usb_giveback_urb_bh still running on bh ffff88814a950978

Reported-by: syzbot+cade843a1e4af0651f5e@xxxxxxxxxxxxxxxxxxxxxxxxx
Link: https://lore.kernel.org/linux-usb/6a8a5047.dbb3a75c.13dd47.003e.GAE@xxxxxxxxxx/
Fixes: 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Michal Pecio <michal.pecio@xxxxxxxxx>
---
 drivers/usb/core/hcd.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index ee19628cd653..b17fd8a0a90a 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -3053,14 +3053,12 @@ 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()
+	 * methods should have waited for them to prevent UAF of driver data.
+	 * However, we still must kill these works so they don't UAF the HCD.
 	 */
+	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