[PATCH] usb: xhci: Fix use-after-free race in xhci_stop_device()

[PATCH] usb: xhci: Fix use-after-free race in xhci_stop_device()

[YuanZhe Liu]

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.

Re-check xhci->devs[slot_id] against the pointer fetched earlier after
taking xhci->lock and bail out if it no longer matches, and trace the
device only while udev is still attached.  xhci_free_virt_device()
frees the device under xhci->lock, so a matching pointer while holding
the lock keeps virt_dev 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 <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
 drivers/usb/host/xhci-hub.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index b0264bd8577a..f058cb4d3534 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -467,13 +467,31 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 	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);
+
+	/*
+	 * Callers drop xhci->lock before calling us and allocating cmd
+	 * above can sleep, so xhci_free_dev() may have freed the device in
+	 * the meantime.  The device is detached from xhci->devs[slot_id]
+	 * under xhci->lock before it is freed, so re-check under the lock
+	 * that this slot still points to it.  The device cannot be freed
+	 * while xhci->lock is held, and virt_dev is not used after the
+	 * lock is dropped below.
+	 */
+	if (xhci->devs[slot_id] != virt_dev) {
+		spin_unlock_irqrestore(&xhci->lock, flags);
+		xhci_free_command(xhci, cmd);
+		return -ENODEV;
+	}
+
+	/* xhci_free_dev() may already have cleared udev without the lock */
+	if (virt_dev->udev)
+		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;
-- 
2.34.1

Re: [PATCH] usb: xhci: Fix use-after-free race in xhci_stop_device()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: xhci: TOCTOU race on virt_dev->udev in xhci_stop_device()
--

commit 8ca23763bf2babc34d266f80e2a0ad16a5640afa
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

usb: xhci: Fix use-after-free race in xhci_stop_device()

This commit fixes a use-after-free race in xhci_stop_device() where a
concurrent usb_disconnect() -> xhci_free_dev() frees virt_dev while
xhci_stop_device() sleeps in xhci_alloc_command(). It re-checks
xhci->devs[slot_id] after acquiring xhci->lock and moves
trace_xhci_stop_device() under the lock with a virt_dev->udev check.

> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 470bafe1802be..828cf347bd32d 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -467,13 +467,31 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
>  	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);
> +
> +	/*
> +	 * Callers drop xhci->lock before calling us and allocating cmd
> +	 * above can sleep, so xhci_free_dev() may have freed the device in
> +	 * the meantime.  The device is detached from xhci->devs[slot_id]
> +	 * under xhci->lock before it is freed, so re-check under the lock
> +	 * that this slot still points to it.  The device cannot be freed
> +	 * while xhci->lock is held, and virt_dev is not used after the
> +	 * lock is dropped below.
> +	 */
> +	if (xhci->devs[slot_id] != virt_dev) {
> +		spin_unlock_irqrestore(&xhci->lock, flags);
> +		xhci_free_command(xhci, cmd);
> +		return -ENODEV;
> +	}
> +
> +	/* xhci_free_dev() may already have cleared udev without the lock */
> +	if (virt_dev->udev)
> +		trace_xhci_stop_device(virt_dev);

[Severity: High]
Can trace_xhci_stop_device() still dereference a NULL virt_dev->udev pointer
here when racing with usb_disconnect() -> xhci_free_dev()?

In drivers/usb/host/xhci.c, xhci_free_dev() clears virt_dev->udev without
holding xhci->lock before calling xhci_disable_slot() and
xhci_free_virt_device():

    for (i = 0; i < 31; i++)
        virt_dev->eps[i].ep_state &= ~EP_STOP_CMD_PENDING;
    virt_dev->udev = NULL;
    xhci_disable_slot(xhci, udev->slot_id);

    spin_lock_irqsave(&xhci->lock, flags);
    xhci_free_virt_device(xhci, virt_dev, udev->slot_id);
    spin_unlock_irqrestore(&xhci->lock, flags);

Because xhci_free_dev() does not hold xhci->lock when setting
virt_dev->udev = NULL, checking if (virt_dev->udev) under xhci->lock in
xhci_stop_device() does not synchronize with that store.

When the tracepoint is enabled, trace_xhci_stop_device(virt_dev) invokes the
xhci_log_virt_dev trace event class in drivers/usb/host/xhci-trace.h, which
reloads vdev->udev from memory inside TP_fast_assign:

    TP_fast_assign(
        __entry->vdev = vdev;
        __entry->in_ctx = (unsigned long long) vdev->in_ctx->dma;
        __entry->out_ctx = (unsigned long long) vdev->out_ctx->dma;
        __entry->devnum = vdev->udev->devnum;
        __entry->state = vdev->udev->state;
        __entry->speed = vdev->udev->speed;
        __entry->portnum = vdev->udev->portnum;
        __entry->level = vdev->udev->level;
        __entry->slot_id = vdev->udev->slot_id;
    ),

If xhci_free_dev() clears virt_dev->udev on another CPU between the
if (virt_dev->udev) check in xhci_stop_device() and the reload in
TP_fast_assign, won't trace_xhci_stop_device() dereference NULL while
holding xhci->lock with interrupts disabled?

Additionally, if virt_dev->udev is already NULL when xhci_stop_device()
checks it, xhci_free_dev() has already started tearing down the device and
is disabling the slot in xhci_disable_slot(). Should xhci_free_dev() clear
virt_dev->udev under xhci->lock, and should xhci_stop_device() bail out with
-ENODEV when !virt_dev->udev rather than continuing to queue stop-endpoint
commands for a slot being disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026105203.1250-1-25031212351@stu.xidian.edu.cn?part=1