usb: hcd: Cancel BH giveback works on removal
PATCH 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 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
--- drivers/usb/core/hcd.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/usb/core/hcd.c +5 -7
@@ -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
Signature · 1 line
2.48.1
Quoted 12 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 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:
Quoted 7 lines
[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 ffff88814a950978Reported-by [email protected]Fixes 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")
While that is logically correct, in fact the patch won't apply as-is to any commit earlier than 8fea0c8fda30129b ("usb: core: hcd: Convert from tasklet to BH workqueue").
Quoted 3 lines
Signed-off-by Michal Pecio <[email protected]>---
Quoted 29 lines
drivers/usb/core/hcd.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)drivers/usb/core/hcd.c +5
-7@@ -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 Signature · 1 line
2.48.1
Quoted 17 lines · Alan Stern
Quoted 16 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 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
Quoted 7 lines
Quoted 3 lines
Reported-by [email protected]Fixes 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")While that is logically correct, in fact the patch won't apply as-is to any commit earlier than
8fea0c8fda30129b("usb: core: hcd: Convert from tasklet to BH workqueue").
Hmm, it's a fairly recent one, so this will only work on v6.12+.
Maybe not a big deal considering Greg's recent opinion about unbind issues, though TBH I'm not really convinced about unbind's irrelevance in this era where everything (including USB HCs) is hotpluggable.
I'm OK with this patch being dropped (I will maintain it for myself), stripped of Cc:stable or limited to branches where it applies. And I probably won't bother backporting it all they way to stone age.
Regards,
Michal
Quoted 16 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 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
Quoted 6 lines
Reported-by [email protected]Fixes 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")Signed-off-by Michal Pecio <[email protected]>---
Hi Greg,
Any interest in this fix? If you think it's too theoretical I can drop the stable tag, but this code just isn't really correct.
Syzbot has found another case: primary HCD of vhci-hcd is freed if secondary HCD creation fails (e.g. USB bus number limit). This (again) races with the giveback work still using the primary HCD after unlinking its root hub URB.
https://lore.kernel.org/linux-usb/[email protected]/
Quoted 14 lines
slab-use-after-free in
usb_giveback_urb_bh+0x441/0x560drivers/usb/core/hcd.c:1692Freed by task 1: hcd_release drivers/usb/core/hcd.c:2690 [inline] kref_put include/linux/kref.h:65 [inline] usb_put_hcd drivers/usb/core/hcd.c:2704 [inline] usb_put_hcd+0x149/0x1f0 drivers/usb/core/hcd.c:2701 vhci_hcd_probe+0x342/0x4e0 drivers/usb/usbip/vhci_hcd.c:1415 Last potentially related work creation: queue_work include/linux/workqueue.h:700 [inline] usb_hcd_giveback_urb+0x330/0x4a0 drivers/usb/core/hcd.c:1758 usb_rh_urb_dequeue drivers/usb/core/hcd.c:845 [inline] unlink1+0x418/0x510 drivers/usb/core/hcd.c:1580
Regards,
Michal