A KASAN slab-use-after-free read was observed in xhci_stop_device(): BUG: KASAN: slab-use-after-free in xhci_stop_device.constprop.0.isra.0+0x395/0x430 drivers/usb/host/xhci-hub.c:479 Read of size 8 at addr ffff88804e85d2f0 Freed by task 585700: kfree+0xf8/0x390 mm/slub.c:6566 xhci_free_virt_device+0x3f/0x50 drivers/usb/host/xhci-mem.c:881 xhci_free_dev+0x286/0x350 drivers/usb/host/xhci.c:4131 usb_disconnect+0x6b6/0x910 drivers/usb/core/hub.c:2391 ... usbdev_ioctl+0x6f05/0x86d0 drivers/usb/core/devio.c:2826 Allocated by task 572283: xhci_alloc_virt_device+0x171/0xb70 drivers/usb/host/xhci-mem.c:986 xhci_alloc_dev+0x475/0x840 drivers/usb/host/xhci.c:4271 usb_alloc_dev+0xe8/0xdc0 drivers/usb/core/usb.c:661 hub_event+0x1c82/0x4050 drivers/usb/core/hub.c:5953 xhci_stop_device() reads xhci->devs[slot_id] without xhci->lock and keeps using the pointer across xhci_alloc_command(GFP_NOIO), which may sleep. All callers (the port suspend and bus suspend paths in xhci_hub_control() and xhci_bus_suspend()) drop xhci->lock before calling it, so a concurrent usb_disconnect() -> xhci_free_dev() can clear xhci->devs[slot_id] and free virt_dev under xhci->lock in that window. The endpoint loop then dereferences the freed virt_dev. commit 88716a93766b ("usb: xhci: Fix panic if disconnect") added a NULL check for this case, but that only helps when the device was already freed before the check; it does not close the window. Look up xhci->devs[slot_id] only after taking xhci->lock, and bail out if it is NULL or if the device is already being torn down. The tracepoint reloads virt_dev->udev, so it moves under the lock and only runs while the device is still attached. xhci_free_dev() clears virt_dev->udev without xhci->lock, and only later detaches the device from xhci->devs[] in xhci_free_virt_device(), also under the lock. Take xhci->lock when clearing ->udev so that xhci_stop_device() can rely on it while holding the same lock. xhci_free_virt_device() clears xhci->devs[slot_id] and frees the device under xhci->lock, so a device found there while holding the lock stays alive until xhci_stop_device() drops the lock, after which virt_dev is not used anymore. Fixes: be88fe4f4dda93e3 ("USB: xHCI: port power management implementation") Cc: stable@vger.kernel.org Co-developed-by: Bin Yu Signed-off-by: Bin Yu Co-developed-by: Cheng Wen Signed-off-by: Cheng Wen Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn> --- Hi Mathias, Thanks for the review. Both points are addressed: Changes since v2: - Moved the xhci->devs[slot_id] lookup inside xhci->lock and dropped the separate pointer comparison, so the unlocked read before the lock is gone. - Dropped the explanatory comment. I kept virt_dev->udev as the marker for "teardown already started" instead of adding a disable_slot_pending flag. It is now written under xhci->lock, so reading it there is enough to tell xhci_stop_device() not to queue stop endpoint commands, and it keeps the change small. The xhci_disable_and_free_slot() race is left for a separate patch. drivers/usb/host/xhci-hub.c | 14 +++++++++----- drivers/usb/host/xhci.c | 3 +++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index b0264bd8577a..9a3437248e04 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -463,17 +463,21 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) int i; ret = 0; - virt_dev = xhci->devs[slot_id]; - if (!virt_dev) - return -ENODEV; - - trace_xhci_stop_device(virt_dev); cmd = xhci_alloc_command(xhci, true, GFP_NOIO); if (!cmd) return -ENOMEM; spin_lock_irqsave(&xhci->lock, flags); + virt_dev = xhci->devs[slot_id]; + if (!virt_dev || !virt_dev->udev) { + spin_unlock_irqrestore(&xhci->lock, flags); + xhci_free_command(xhci, cmd); + return -ENODEV; + } + + trace_xhci_stop_device(virt_dev); + for (i = LAST_EP_INDEX; i > 0; i--) { if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue) { struct xhci_ep_ctx *ep_ctx; diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 5c759ad28199..9e375eb48f38 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -4128,9 +4128,12 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) trace_xhci_free_dev(slot_ctx); /* Stop any wayward timer functions (which may grab the lock) */ + spin_lock_irqsave(&xhci->lock, flags); for (i = 0; i < 31; i++) virt_dev->eps[i].ep_state &= ~EP_STOP_CMD_PENDING; virt_dev->udev = NULL; + spin_unlock_irqrestore(&xhci->lock, flags); + xhci_disable_slot(xhci, udev->slot_id); spin_lock_irqsave(&xhci->lock, flags); -- 2.34.1