| 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: <dec32556-c28e-aeed-8516-2e0bb56c3a58@zhaoxin.com>
Date: Wed, 30 Jul 2025 01:25:00 +0800
From: "WeitaoWang-oc@...oxin.com" <WeitaoWang-oc@...oxin.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>,
<gregkh@...uxfoundation.org>, <mathias.nyman@...el.com>,
<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: <WeitaoWang@...oxin.com>, <wwt8723@....com>, <CobeChen@...oxin.com>,
<stable@...r.kernel.org>
Subject: Re: [PATCH v2] usb:xhci:Fix slot_id resource race conflict
On 2025/7/28 21:16, Mathias Nyman wrote:
>
> On 25.7.2025 21.51, Weitao Wang wrote:
>> In such a scenario, device-A with slot_id equal to 1 is disconnecting
>> while device-B is enumerating, device-B will fail to enumerate in the
>> follow sequence.
>>
>> 1.[device-A] send disable slot command
>> 2.[device-B] send enable slot command
>> 3.[device-A] disable slot command completed and wakeup waiting thread
>> 4.[device-B] enable slot command completed with slot_id equal to 1 and
>> wakeup waiting thread
>> 5.[device-B] driver check this slot_id was used by someone(device-A) in
>> xhci_alloc_virt_device, this device fails to enumerate as this conflict
>> 6.[device-A] xhci->devs[slot_id] set to NULL in xhci_free_virt_device
>>
>> To fix driver's slot_id resources conflict, let the xhci_free_virt_device
>> functionm call in the interrupt handler when disable slot command success.
>>
>> Cc: stable@...r.kernel.org
>> Fixes: 7faac1953ed1 ("xhci: avoid race between disable slot command and host runtime
>> suspend")
>> Signed-off-by: Weitao Wang <WeitaoWang-oc@...oxin.com>
>
> Nice catch, good to get this fixed.
>
> This however has the downside of doing a lot in interrupt context.
>
> what if we only clear some strategic pointers in the interrupt context,
> and then do all the actual unmapping and endpoint ring segments freeing,
> contexts freeing ,etc later?
>
> Pseudocode:
>
> xhci_handle_cmd_disable_slot(xhci, slot_id, comp_code)
> {
> if (cmd_comp_code == COMP_SUCCESS) {
> xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
> xhci->devs[slot_id] = NULL;
> }
> }
>
> xhci_disable_and_free_slot(xhci, slot_id)
> {
> struct xhci_virt_device *vdev = xhci->devs[slot_id];
>
> xhci_disable_slot(xhci, slot_id);
> xhci_free_virt_device(xhci, vdev, slot_id);
> }
>
> xhci_free_virt_device(xhci, vdev, slot_id)
> {
> if (xhci->dcbaa->dev_context_ptrs[slot_id] == vdev->out_ctx->dma)
> xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
>
> // free and unmap things just like before
> ...
>
> if (xhci->devs[slot_id] == vdev)
> xhci->devs[slot_id] = NULL;
>
> kfee(vdev);
> }
Hi Mathias,
Yes, your suggestion is a better revision, I made some modifications
to the patch which is listed below. Please help to review again.
Thanks for your help.
---
drivers/usb/host/xhci-hub.c | 3 +--
drivers/usb/host/xhci-mem.c | 21 ++++++++++-----------
drivers/usb/host/xhci-ring.c | 9 +++++++--
drivers/usb/host/xhci.c | 23 ++++++++++++++++-------
drivers/usb/host/xhci.h | 3 ++-
5 files changed, 36 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 92bb84f8132a..b3a59ce1b3f4 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -704,8 +704,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
if (!xhci->devs[i])
continue;
- retval = xhci_disable_slot(xhci, i);
- xhci_free_virt_device(xhci, i);
+ retval = xhci_disable_and_free_slot(xhci, i);
if (retval)
xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
i, retval);
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6680afa4f596..fc4aca2e65bc 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -865,21 +865,18 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci,
* will be manipulated by the configure endpoint, allocate device, or update
* hub functions while this function is removing the TT entries from the list.
*/
-void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
+void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
+ int slot_id)
{
- struct xhci_virt_device *dev;
int i;
int old_active_eps = 0;
/* Slot ID 0 is reserved */
- if (slot_id == 0 || !xhci->devs[slot_id])
+ if (slot_id == 0 || !dev)
return;
- dev = xhci->devs[slot_id];
-
- xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
- if (!dev)
- return;
+ if (xhci->dcbaa->dev_context_ptrs[slot_id] == dev->out_ctx->dma)
+ xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
trace_xhci_free_virt_device(dev);
@@ -920,8 +917,10 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
dev->udev->slot_id = 0;
if (dev->rhub_port && dev->rhub_port->slot_id == slot_id)
dev->rhub_port->slot_id = 0;
- kfree(xhci->devs[slot_id]);
- xhci->devs[slot_id] = NULL;
+ if (xhci->devs[slot_id] == dev)
+ xhci->devs[slot_id] = NULL;
+ kfree(dev);
}
/*
@@ -962,7 +961,7 @@ static void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci,
int slot_i
out:
/* we are now at a leaf device */
xhci_debugfs_remove_slot(xhci, slot_id);
- xhci_free_virt_device(xhci, slot_id);
+ xhci_free_virt_device(xhci, vdev, slot_id);
}
int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 94c9c9271658..7a440ec52ff6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1589,7 +1589,8 @@ static void xhci_handle_cmd_enable_slot(int slot_id, struct
xhci_command *comman
command->slot_id = 0;
}
-static void xhci_handle_cmd_disable_slot(struct xhci_hcd *xhci, int slot_id)
+static void xhci_handle_cmd_disable_slot(struct xhci_hcd *xhci, int slot_id,
+ u32 cmd_comp_code)
{
struct xhci_virt_device *virt_dev;
struct xhci_slot_ctx *slot_ctx;
@@ -1604,6 +1605,10 @@ static void xhci_handle_cmd_disable_slot(struct xhci_hcd *xhci, int
slot_id)
if (xhci->quirks & XHCI_EP_LIMIT_QUIRK)
/* Delete default control endpoint resources */
xhci_free_device_endpoint_resources(xhci, virt_dev, true);
+ if (cmd_comp_code == COMP_SUCCESS) {
+ xhci->dcbaa->dev_context_ptrs[slot_id] = 0;
+ xhci->devs[slot_id] = NULL;
+ }
}
static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, int slot_id)
@@ -1853,7 +1858,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
xhci_handle_cmd_enable_slot(slot_id, cmd, cmd_comp_code);
break;
case TRB_DISABLE_SLOT:
- xhci_handle_cmd_disable_slot(xhci, slot_id);
+ xhci_handle_cmd_disable_slot(xhci, slot_id, cmd_comp_code);
break;
case TRB_CONFIG_EP:
if (!cmd->completion)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 8a819e853288..b3b39b2498f6 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3930,8 +3930,7 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd,
* Obtaining a new device slot to inform the xHCI host that
* the USB device has been reset.
*/
- ret = xhci_disable_slot(xhci, udev->slot_id);
- xhci_free_virt_device(xhci, udev->slot_id);
+ ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
if (!ret) {
ret = xhci_alloc_dev(hcd, udev);
if (ret == 1)
@@ -4088,7 +4087,7 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
xhci_disable_slot(xhci, udev->slot_id);
spin_lock_irqsave(&xhci->lock, flags);
- xhci_free_virt_device(xhci, udev->slot_id);
+ xhci_free_virt_device(xhci, virt_dev, udev->slot_id);
spin_unlock_irqrestore(&xhci->lock, flags);
}
@@ -4137,6 +4136,18 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
return 0;
}
+int xhci_disable_and_free_slot(struct xhci_hcd *xhci, u32 slot_id)
+{
+ struct xhci_virt_device *vdev = xhci->devs[slot_id];
+ int ret;
+
+ ret = xhci_disable_slot(xhci, slot_id);
+ xhci_free_virt_device(xhci, vdev, slot_id);
+ return ret;
+}
+
/*
* Checks if we have enough host controller resources for the default control
* endpoint.
@@ -4243,8 +4254,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
return 1;
disable_slot:
- xhci_disable_slot(xhci, udev->slot_id);
- xhci_free_virt_device(xhci, udev->slot_id);
+ xhci_disable_and_free_slot(xhci, udev->slot_id);
return 0;
}
@@ -4380,8 +4390,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device
*udev,
dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
mutex_unlock(&xhci->mutex);
- ret = xhci_disable_slot(xhci, udev->slot_id);
- xhci_free_virt_device(xhci, udev->slot_id);
+ ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
if (!ret) {
if (xhci_alloc_dev(hcd, udev) == 1)
xhci_setup_addressable_virt_dev(xhci, udev);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index a20f4e7cd43a..85d5b964bf1e 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1791,7 +1791,7 @@ void xhci_dbg_trace(struct xhci_hcd *xhci, void (*trace)(struct
va_format *),
/* xHCI memory management */
void xhci_mem_cleanup(struct xhci_hcd *xhci);
int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags);
-void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id);
+void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev, int slot_id);
int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id, struct usb_device *udev,
gfp_t flags);
int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *udev);
void xhci_copy_ep0_dequeue_into_input_ctx(struct xhci_hcd *xhci,
@@ -1888,6 +1888,7 @@ void xhci_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
int xhci_update_hub_device(struct usb_hcd *hcd, struct usb_device *hdev,
struct usb_tt *tt, gfp_t mem_flags);
int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
+int xhci_disable_and_free_slot(struct xhci_hcd *xhci, u32 slot_id);
int xhci_ext_cap_init(struct xhci_hcd *xhci);
int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
--
Best Regards,
weitao
Powered by blists - more mailing lists