usb: hcd: Cancel BH giveback works on removal
PATCH v2 1/1 usb: hcd: Cancel BH giveback works on removal
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.
---
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/[email protected]/
drivers/usb/core/hcd.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/usb/core/hcd.c +6 -7
@@ -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
Signature · 1 line
2.48.1
[email protected]
AI reviewSashiko has reviewed this patch and found no issues.
Quoted 17 lines
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 - seeusb_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, onPREEMPT_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.
Quoted 7 lines
Reported-by [email protected]Reported-by [email protected]Fixes 8fea0c8fda30129b ("usb: core: hcd: Convert from tasklet to BH workqueue")Signed-off-by Michal Pecio <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 33 lines · 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 - seeusb_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, onPREEMPT_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 [email protected]Reported-by [email protected]Fixes 8fea0c8fda30129b ("usb: core: hcd: Convert from tasklet to BH workqueue")Signed-off-by Michal Pecio <[email protected]>---
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
Quoted 2 lines
Quoted 30 lines
drivers/usb/core/hcd.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)drivers/usb/core/hcd.c +6
-7@@ -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 Signature · 1 line
2.48.1