[PATCH v2] usb: xhci: clear dangling sideband pointer in xhci_free_virt_device()

[PATCH v2] usb: xhci: clear dangling sideband pointer in xhci_free_virt_device()

[胡连勤]

xhci_free_virt_device() must not leave any dangling pointers.
If vdev->sideband is still set at this point then something is
wrong, e.g. the sideband client did not unregister before the
virtual device was freed. This can happen when
xhci_setup_device() gets COMP_USB_TRANSACTION_ERROR (device not
responding to setup address during bus reset recovery), causing
xhci_disable_and_free_slot() -> xhci_free_virt_device() to free
vdev before the sideband client has a chance to unregister.

  hub_event()
    xhci_setup_device()                  <-- COMP_USB_TRANSACTION_ERROR
    xhci_disable_and_free_slot()
      xhci_free_virt_device()
        kfree(out_ctx), kfree(vdev)
        xhci->devs[slot_id] = NULL
    ...
    usb_disconnect()
      uaudio_disconnect()
        xhci_sideband_unregister()
          xhci_stop_endpoint_sync()
            xhci_get_ep_ctx()            <-- CRASH (deref freed out_ctx)

Unable to handle kernel paging request at virtual address dead000000000122
Call trace:
 xhci_get_ep_ctx+0x0/0x38
 xhci_sideband_unregister+0x68/0xf0
 uaudio_disconnect+0x70/0x144
 usb_audio_disconnect+0x7c/0x268
 usb_unbind_interface+0x13c/0x340
 device_release_driver_internal+0x1c4/0x2bc
 device_release_driver+0x18/0x28
 bus_remove_device+0x158/0x170
 device_del+0x1c8/0x320
 usb_disable_device+0x84/0x190
 usb_disconnect+0xe8/0x338
 hub_event+0xbd8/0x19ac
 process_scheduled_works+0x200/0x9d8
 worker_thread+0x154/0x3b0
 kthread+0x11c/0x1a0

Fix this by clearing any remaining sideband pointer in
xhci_free_virt_device() before freeing vdev. If vdev->sideband is
still set, set vdev->sideband->vdev = NULL to break the dangling
pointer at the source.

Additionally, in xhci_sideband_unregister(), check sb->vdev before
issuing stop endpoint commands. If vdev is already NULL (cleared by
xhci_free_virt_device), skip endpoint cleanup as the xHC has already
disabled the slot, but still remove the interrupter and free the
sideband instance to avoid leaks.

Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
Cc: stable@vger.kernel.org
Signed-off-by: Lianqin Hu <hulianqin@vivo.com>
---

Changes in v2:
 - Move fix to xhci_free_virt_device() per maintainer suggestion.
 - Use xhci_dbg per maintainer suggestion.
 - Ensure interrupter cleanup is unconditional when vdev is NULL.
 - Clear dangling sb->eps[] when vdev is NULL (per Selva).
 - Update patch commit message.
 - Link to v1: https://lore.kernel.org/all/TYUPR06MB6217000B59003EDF233D7246D2B22@TYUPR06MB6217.apcprd06.prod.outlook.com/

 drivers/usb/host/xhci-mem.c      |  9 +++++++++
 drivers/usb/host/xhci-sideband.c | 28 +++++++++++++++++++++-------
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index af8d4b74c4ba..448d28aaff3e 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -15,6 +15,7 @@
 #include <linux/dmapool.h>
 #include <linux/dma-mapping.h>
 #include <linux/bitfield.h>
+#include <linux/usb/xhci-sideband.h>
 
 #include "xhci.h"
 #include "xhci-trace.h"
@@ -922,6 +923,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
 		dev->rhub_port->slot_id = 0;
 	if (xhci->devs[slot_id] == dev)
 		xhci->devs[slot_id] = NULL;
+
+	if (dev->sideband) {
+		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
+			 slot_id);
+		dev->sideband->vdev = NULL;
+		dev->sideband = NULL;
+	}
+
 	kfree(dev);
 }
 
diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index a5deeee4d5dc..f979ce517163 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -472,12 +472,25 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	scoped_guard(mutex, &sb->mutex) {
 		vdev = sb->vdev;
-		if (!vdev)
-			return;
-
-		for (i = 0; i < EP_CTX_PER_DEV; i++)
-			if (sb->eps[i])
-				__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
+		/*
+		 * If vdev is NULL, xhci_free_virt_device() has already
+		 * cleared sb->vdev and freed vdev (e.g. on
+		 * COMP_USB_TRANSACTION_ERROR during address device
+		 * recovery). Skip endpoint cleanup as the xHC has already
+		 * disabled the slot.
+		 *
+		 * The interrupter and sideband instance are host-level
+		 * resources independent of vdev, so still remove and free
+		 * them to avoid leaks.
+		 */
+		if (vdev) {
+			for (i = 0; i < EP_CTX_PER_DEV; i++)
+				if (sb->eps[i])
+					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
+		} else {
+			for (i = 0; i < EP_CTX_PER_DEV; i++)
+				sb->eps[i] = NULL;
+		}
 
 		__xhci_sideband_remove_interrupter(sb);
 
@@ -486,7 +499,8 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
 
 	spin_lock_irq(&xhci->lock);
 	sb->xhci = NULL;
-	vdev->sideband = NULL;
+	if (vdev)
+		vdev->sideband = NULL;
 	spin_unlock_irq(&xhci->lock);
 
 	kfree(sb);
-- 
2.48.1

Re: [PATCH v2] usb: xhci: clear dangling sideband pointer in xhci_free_virt_device()

[Michal Pecio]

On Fri, 11 Sep 2026 10:22:37 +0000, 胡连勤 wrote:
> xhci_free_virt_device() must not leave any dangling pointers.
> If vdev->sideband is still set at this point then something is
> wrong, e.g. the sideband client did not unregister before the
> virtual device was freed. This can happen when
> xhci_setup_device() gets COMP_USB_TRANSACTION_ERROR (device not
> responding to setup address during bus reset recovery), causing
> xhci_disable_and_free_slot() -> xhci_free_virt_device() to free
> vdev before the sideband client has a chance to unregister.

When and how is the sideband client driver supposed to learn that
its device has been reset?

There is some code in xhci_discover_or_reset_device() which sends
notification that endpoints have been removed. Does it run before
or after vdev can potentially be freed?

BEFORE: it seems we had an opportunity to get rid of the sideband
completely before running into trouble here.

AFTER: freeing vdev will break those notifications, is it a bug?

The suggestion by Mathias that sideband should be fully destroyed
by the client *before* USB core begins reset doesn't look bad.

> 
>   hub_event()
>     xhci_setup_device()                  <-- COMP_USB_TRANSACTION_ERROR
>     xhci_disable_and_free_slot()
>       xhci_free_virt_device()
>         kfree(out_ctx), kfree(vdev)
>         xhci->devs[slot_id] = NULL
>     ...
>     usb_disconnect()
>       uaudio_disconnect()
>         xhci_sideband_unregister()
>           xhci_stop_endpoint_sync()
>             xhci_get_ep_ctx()            <-- CRASH (deref freed out_ctx)
> 
> Unable to handle kernel paging request at virtual address dead000000000122
> Call trace:
>  xhci_get_ep_ctx+0x0/0x38
>  xhci_sideband_unregister+0x68/0xf0
>  uaudio_disconnect+0x70/0x144
>  usb_audio_disconnect+0x7c/0x268
>  usb_unbind_interface+0x13c/0x340
>  device_release_driver_internal+0x1c4/0x2bc
>  device_release_driver+0x18/0x28
>  bus_remove_device+0x158/0x170
>  device_del+0x1c8/0x320
>  usb_disable_device+0x84/0x190
>  usb_disconnect+0xe8/0x338
>  hub_event+0xbd8/0x19ac
>  process_scheduled_works+0x200/0x9d8
>  worker_thread+0x154/0x3b0
>  kthread+0x11c/0x1a0
> 
> Fix this by clearing any remaining sideband pointer in
> xhci_free_virt_device() before freeing vdev. If vdev->sideband is
> still set, set vdev->sideband->vdev = NULL to break the dangling
> pointer at the source.
> 
> Additionally, in xhci_sideband_unregister(), check sb->vdev before
> issuing stop endpoint commands. If vdev is already NULL (cleared by
> xhci_free_virt_device), skip endpoint cleanup as the xHC has already
> disabled the slot, but still remove the interrupter and free the
> sideband instance to avoid leaks.
> 
> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lianqin Hu <hulianqin@vivo.com>
> ---
> 
> Changes in v2:
>  - Move fix to xhci_free_virt_device() per maintainer suggestion.
>  - Use xhci_dbg per maintainer suggestion.
>  - Ensure interrupter cleanup is unconditional when vdev is NULL.
>  - Clear dangling sb->eps[] when vdev is NULL (per Selva).
>  - Update patch commit message.
>  - Link to v1: https://lore.kernel.org/all/TYUPR06MB6217000B59003EDF233D7246D2B22@TYUPR06MB6217.apcprd06.prod.outlook.com/
> 
>  drivers/usb/host/xhci-mem.c      |  9 +++++++++
>  drivers/usb/host/xhci-sideband.c | 28 +++++++++++++++++++++-------
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index af8d4b74c4ba..448d28aaff3e 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -15,6 +15,7 @@
>  #include <linux/dmapool.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/bitfield.h>
> +#include <linux/usb/xhci-sideband.h>
>  
>  #include "xhci.h"
>  #include "xhci-trace.h"
> @@ -922,6 +923,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>  		dev->rhub_port->slot_id = 0;
>  	if (xhci->devs[slot_id] == dev)
>  		xhci->devs[slot_id] = NULL;
> +
> +	if (dev->sideband) {

Could be:

  if (IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND) && dev->sideband)

> +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
> +			 slot_id);
> +		dev->sideband->vdev = NULL;
> +		dev->sideband = NULL;
> +	}
> +
>  	kfree(dev);
>  }
>  
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index a5deeee4d5dc..f979ce517163 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -472,12 +472,25 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>  
>  	scoped_guard(mutex, &sb->mutex) {
>  		vdev = sb->vdev;
> -		if (!vdev)
> -			return;
> -
> -		for (i = 0; i < EP_CTX_PER_DEV; i++)
> -			if (sb->eps[i])
> -				__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> +		/*
> +		 * If vdev is NULL, xhci_free_virt_device() has already
> +		 * cleared sb->vdev and freed vdev (e.g. on
> +		 * COMP_USB_TRANSACTION_ERROR during address device
> +		 * recovery). Skip endpoint cleanup as the xHC has already
> +		 * disabled the slot.
> +		 *
> +		 * The interrupter and sideband instance are host-level
> +		 * resources independent of vdev, so still remove and free
> +		 * them to avoid leaks.
> +		 */
> +		if (vdev) {
> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> +				if (sb->eps[i])
> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> +		} else {
> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> +				sb->eps[i] = NULL;
> +		}
>  
>  		__xhci_sideband_remove_interrupter(sb);
>  
> @@ -486,7 +499,8 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>  
>  	spin_lock_irq(&xhci->lock);
>  	sb->xhci = NULL;
> -	vdev->sideband = NULL;
> +	if (vdev)
> +		vdev->sideband = NULL;
>  	spin_unlock_irq(&xhci->lock);
>  
>  	kfree(sb);
> -- 
> 2.48.1
> 

答复: [PATCH v2] usb: xhci: clear dangling sideband pointer in xhci_free_virt_device()

[胡连勤]

Hi Michal,

Thanks for the detailed review.

> > xhci_free_virt_device() must not leave any dangling pointers.
> > If vdev->sideband is still set at this point then something is
> > wrong, e.g. the sideband client did not unregister before the
> > virtual device was freed. This can happen when
> > xhci_setup_device() gets COMP_USB_TRANSACTION_ERROR (device not
> > responding to setup address during bus reset recovery), causing
> > xhci_disable_and_free_slot() -> xhci_free_virt_device() to free
> > vdev before the sideband client has a chance to unregister.
> 
> When and how is the sideband client driver supposed to learn that
> its device has been reset?
> 
> There is some code in xhci_discover_or_reset_device() which sends
> notification that endpoints have been removed. Does it run before
> or after vdev can potentially be freed?

You're absolutely right. Looking at the code:

xhci_discover_or_reset_device() notifies sideband BEFORE freeing:
  line 4074: xhci_sideband_notify_ep_ring_free(ep->sideband, i);
  line 4077: xhci_free_endpoint_ring(xhci, virt_dev, i);

So the sideband client gets a chance to release its ring references
before they are freed. This is the BEFORE case.

But xhci_setup_device() on COMP_USB_TRANSACTION_ERROR takes a different
path (line 4434-4445):
  - xhci_disable_and_free_slot()
    - xhci_free_virt_device()
      - kfree(out_ctx), kfree(vdev)
  - No sideband notification at all

This is the AFTER case you described - vdev is freed without notifying
the sideband client, breaking the notification mechanism itself.

> 
> BEFORE: it seems we had an opportunity to get rid of the sideband
> completely before running into trouble here.
> 
> AFTER: freeing vdev will break those notifications, is it a bug?
> 
> The suggestion by Mathias that sideband should be fully destroyed
> by the client *before* USB core begins reset doesn't look bad.

Agreed. The root cause is that the sideband client is unaware of the
device reset/free in the COMP_USB_TRANSACTION_ERROR path.

This v2 patch is a defensive fix to prevent the immediate crash
(NULL pointer dereference in xhci_sideband_unregister()). It doesn't
address the root cause.

For the complete fix, I think we have two options:

1. In xhci_setup_device() COMP_USB_TRANSACTION_ERROR path, add
   sideband notification before calling xhci_disable_and_free_slot().
   Unlike xhci_discover_or_reset_device() which iterates endpoints
   and calls xhci_sideband_notify_ep_ring_free() before freeing
   rings (line 4062-4077), the TRANSACTION_ERROR path skips this
   step entirely and goes straight to xhci_free_virt_device().

2. As Mathias suggested, use drv->pre_reset/post_reset to unregister
   and re-register sideband around device reset, so the client
   driver handles the lifecycle explicitly.

I'll prepare a follow-up patch for the root cause fix. Would you
or Mathias have a preference between option 1 and 2?

Thanks,
Lianqin