| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251028165153.283980-1-mathias.nyman@linux.intel.com>
Date: Tue, 28 Oct 2025 18:51:53 +0200
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: uttkarsh.aggarwal@....qualcomm.com
Cc: mathias.nyman@...ux.intel.com,
gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org,
mathias.nyman@...el.com,
wesley.cheng@....qualcomm.com
Subject: [RFT PATCH v2] xhci: sideband: Fix race condition in sideband unregister
Uttkarsh Aggarwal observed a kernel panic during sideband un-register
and found it was caused by a race condition between sideband unregister,
and creating sideband interrupters.
The issue occurrs when thread T1 runs uaudio_disconnect() and released
sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
a crash.
Ensure new endpoints or interrupter can't be added to a sidenband after
xhci_sideband_unregister() cleared the existing ones, and unlocked the
sideband mutex.
Reorganize code so that mutex is only taken and released once in
xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.
Use mutex guards to reduce human unlock errors in code
Refuse to add endpoints or interrupter if sb->vdev is not set.
sb->vdev is set when sideband is created and registered.
Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@....qualcomm.com>
Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
Signed-off-by: Mathias Nyman <mathias.nyman@...ux.intel.com>
---
v2:
use guard() and fix missing mutex_unlock as recommended by greg k-h
---
drivers/usb/host/xhci-sideband.c | 97 +++++++++++++++++---------------
1 file changed, 53 insertions(+), 44 deletions(-)
diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index e771a476fef2..2daa0ba7ad9a 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -86,6 +86,22 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
sb->eps[ep->ep_index] = NULL;
}
+static void
+__xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
+{
+ struct usb_device *udev;
+
+ if (!sb->ir)
+ return;
+
+ xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
+ sb->ir = NULL;
+ udev = sb->vdev->udev;
+
+ if (udev->state != USB_STATE_NOTATTACHED)
+ usb_offload_put(udev);
+}
+
/* sideband api functions */
/**
@@ -131,14 +147,17 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
struct xhci_virt_ep *ep;
unsigned int ep_index;
- mutex_lock(&sb->mutex);
+ guard(mutex)(&sb->mutex);
+
+ if (!sb->vdev)
+ return -ENODEV;
+
ep_index = xhci_get_endpoint_index(&host_ep->desc);
ep = &sb->vdev->eps[ep_index];
- if (ep->ep_state & EP_HAS_STREAMS) {
- mutex_unlock(&sb->mutex);
+ if (ep->ep_state & EP_HAS_STREAMS)
return -EINVAL;
- }
+
/*
* Note, we don't know the DMA mask of the audio DSP device, if its
@@ -148,14 +167,11 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
* and let this function add the endpoint and allocate the ring buffer
* with the smallest common DMA mask
*/
- if (sb->eps[ep_index] || ep->sideband) {
- mutex_unlock(&sb->mutex);
+ if (sb->eps[ep_index] || ep->sideband)
return -EBUSY;
- }
ep->sideband = sb;
sb->eps[ep_index] = ep;
- mutex_unlock(&sb->mutex);
return 0;
}
@@ -180,18 +196,16 @@ xhci_sideband_remove_endpoint(struct xhci_sideband *sb,
struct xhci_virt_ep *ep;
unsigned int ep_index;
- mutex_lock(&sb->mutex);
+ guard(mutex)(&sb->mutex);
+
ep_index = xhci_get_endpoint_index(&host_ep->desc);
ep = sb->eps[ep_index];
- if (!ep || !ep->sideband || ep->sideband != sb) {
- mutex_unlock(&sb->mutex);
+ if (!ep || !ep->sideband || ep->sideband != sb)
return -ENODEV;
- }
__xhci_sideband_remove_endpoint(sb, ep);
xhci_initialize_ring_info(ep->ring);
- mutex_unlock(&sb->mutex);
return 0;
}
@@ -316,28 +330,25 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
if (!sb || !sb->xhci)
return -ENODEV;
- mutex_lock(&sb->mutex);
- if (sb->ir) {
- ret = -EBUSY;
- goto out;
- }
+ guard(mutex)(&sb->mutex);
+
+ if (!sb->vdev)
+ return -ENODEV;
+
+ if (sb->ir)
+ return -EBUSY;
sb->ir = xhci_create_secondary_interrupter(xhci_to_hcd(sb->xhci),
num_seg, imod_interval,
intr_num);
- if (!sb->ir) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!sb->ir)
+ return -ENOMEM;
udev = sb->vdev->udev;
ret = usb_offload_get(udev);
sb->ir->ip_autoclear = ip_autoclear;
-out:
- mutex_unlock(&sb->mutex);
-
return ret;
}
EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
@@ -352,21 +363,12 @@ EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
void
xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
{
- struct usb_device *udev;
-
- if (!sb || !sb->ir)
+ if (!sb)
return;
- mutex_lock(&sb->mutex);
- xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
-
- sb->ir = NULL;
- udev = sb->vdev->udev;
+ guard(mutex)(&sb->mutex);
- if (udev->state != USB_STATE_NOTATTACHED)
- usb_offload_put(udev);
-
- mutex_unlock(&sb->mutex);
+ __xhci_sideband_remove_interrupter(sb);
}
EXPORT_SYMBOL_GPL(xhci_sideband_remove_interrupter);
@@ -465,6 +467,7 @@ EXPORT_SYMBOL_GPL(xhci_sideband_register);
void
xhci_sideband_unregister(struct xhci_sideband *sb)
{
+ struct xhci_virt_device *vdev;
struct xhci_hcd *xhci;
int i;
@@ -473,17 +476,23 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
xhci = sb->xhci;
- mutex_lock(&sb->mutex);
- for (i = 0; i < EP_CTX_PER_DEV; i++)
- if (sb->eps[i])
- __xhci_sideband_remove_endpoint(sb, sb->eps[i]);
- mutex_unlock(&sb->mutex);
+ 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]);
- xhci_sideband_remove_interrupter(sb);
+ __xhci_sideband_remove_interrupter(sb);
+
+ sb->vdev = NULL;
+ }
spin_lock_irq(&xhci->lock);
sb->xhci = NULL;
- sb->vdev->sideband = NULL;
+ vdev->sideband = NULL;
spin_unlock_irq(&xhci->lock);
kfree(sb);
--
2.43.0
Powered by blists - more mailing lists