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

[PATCH v2] 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, 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() 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>
---
Changes since v1:
 - xhci_free_dev(): clear virt_dev->udev under xhci->lock, so that
   checking it under the same lock actually synchronizes with the
   store.
 - xhci_stop_device(): bail out with -ENODEV once ->udev has been
   cleared, instead of queueing stop-endpoint commands for a slot
   that is already being disabled.

 drivers/usb/host/xhci-hub.c | 19 +++++++++++++++++--
 drivers/usb/host/xhci.c     |  3 +++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index b0264bd8577a..f4401f2fcea0 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -467,13 +467,28 @@ 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 detached and freed
+	 * the device in the meantime.  It removes the device from
+	 * xhci->devs[slot_id] and clears ->udev under xhci->lock, so while
+	 * the lock is held a matching, still-attached device stays alive
+	 * and keeps its udev.
+	 */
+	if (xhci->devs[slot_id] != 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

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

[YuanZhe Liu]

Hi Mathias,

Could you please review this patch?

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

It synchronizes access to the virtual device and its USB device pointer
with xhci->lock to avoid a disconnect race in xhci_stop_device().

The patch is unchanged. I am copying this address for visibility.

Original submission:
https://lore.kernel.org/r/20261026123628.7493-1-25031212351@stu.xidian.edu.cn

Thanks,
YuanZhe

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

[Mathias Nyman]

Hi


On 10/26/26 14:36, YuanZhe Liu wrote:
> 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, 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() 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>

Thanks for the patch, good catch.

I have some small minor adjustments that would make final code a bit cleaner

> ---
> Changes since v1:
>   - xhci_free_dev(): clear virt_dev->udev under xhci->lock, so that
>     checking it under the same lock actually synchronizes with the
>     store.
>   - xhci_stop_device(): bail out with -ENODEV once ->udev has been
>     cleared, instead of queueing stop-endpoint commands for a slot
>     that is already being disabled.
> 
>   drivers/usb/host/xhci-hub.c | 19 +++++++++++++++++--
>   drivers/usb/host/xhci.c     |  3 +++
>   2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index b0264bd8577a..f4401f2fcea0 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -467,13 +467,28 @@ 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 detached and freed
> +	 * the device in the meantime.  It removes the device from
> +	 * xhci->devs[slot_id] and clears ->udev under xhci->lock, so while
> +	 * the lock is held a matching, still-attached device stays alive
> +	 * and keeps its udev.
> +	 */

Comment above is not needed, can be removed

> +	if (xhci->devs[slot_id] != virt_dev || !virt_dev->udev) {
> +		spin_unlock_irqrestore(&xhci->lock, flags);
> +		xhci_free_command(xhci, cmd);
> +		return -ENODEV;
> +	}

No point in keeping the 'virt_dev = xhci->devs[slot_id]' before taking the lock.

Just move that part inside the lock as well:

virt_dev = xhci->devs[slot_id];
if (!virt_dev || !virt_dev->udev) {
	unlock
	free command
	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);
> +

Should work, we are now using virt_dev->udev as a flag to tell xhci_stop_device() to
not queue any stop endpoint commands as we know a 'disable slot' command will be
queued shortly.

Another option would be to add an actual 'disable_slot_pending' flag to struct
xhci_virt_device, and set it in xhci_disable_slot() when we really are queuing the
disable slot command with lock held.

While looking at this I realize that xhci_disable_and_free_slot() might also race
with xhci_stop_device(), but that change doesn't need to be part of this patch.

Thanks
Mathias

[PATCH v3] 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.

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 <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>
---
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