答复: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister
From: 胡连勤
Date: Fri Sep 11 2026 - 03:33:21 EST
Hi Selva,
> >
> > drivers/usb/host/xhci-mem.c | 8 ++++++++
> > drivers/usb/host/xhci-sideband.c | 25 ++++++++++++++++++-------
> > 2 files changed, 26 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> > index af8d4b74c4ba..afdcfb38b35f 100644
> > --- a/drivers/usb/host/xhci-mem.c
> > +++ b/drivers/usb/host/xhci-mem.c
> > @@ -922,6 +922,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;
>
> Thanks for your updated patch.
>
> Dont forget to add #include <linux/usb/xhci-sideband.h> in this
> xhci-mem.c file otherwise getting below error,
>
> drivers/usb/host/xhci-mem.c:928:30: error: invalid use of undefined type
> ‘struct xhci_sideband’
> 928 | dev->sideband->vdev = NULL;
>
Include the corresponding header file
#include <linux/dmapool.h>
#include <linux/dma-mapping.h>
#include <linux/bitfield.h>
+#include <linux/usb/xhci-sideband.h>
> > + dev->sideband = NULL;
> > + }
> > +
> > kfree(dev);
> > }
> >
> > diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> > index a5deeee4d5dc..6312c9e3af65 100644
> > --- a/drivers/usb/host/xhci-sideband.c
> > +++ b/drivers/usb/host/xhci-sideband.c
> > @@ -472,12 +472,22 @@ 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]);
>
> I have one query on skip endpoint cleanup due to vdev is NULL, in this
> case the pointers in sb->eps are not cleared. Since these pointers point
> into the virtual device eps, any subsequent call to sideband API
> functions like xhci_sideband_get_endpoint_buffer() that dereference
> sb->eps could result in a use after free if the virtual device has been
> freed. Is it possible?
Yes, you're absolutely right. If sb->eps[] is not cleared when vdev
is NULL, subsequent calls to xhci_sideband_get_endpoint_buffer() or
similar API functions would dereference dangling pointers into the
freed vdev, resulting in use-after-free.
I added the else branch to clear sb->eps[]:
} else {
for (i = 0; i < EP_CTX_PER_DEV; i++)
sb->eps[i] = NULL;
}
The complete code modification is as follows:
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);
Thanks
Lianqin