[PATCH 00/14] xhci features and fixes for usb-next

[PATCH 00/14] xhci features and fixes for usb-next

[Mathias Nyman]

Hi Greg

xhci improvements and non-urgent fixes for usb-next.

One series by Michal to improve transfer event handling, otherwise
smaller scattered patches for dbc, early dbc, sideband and generic
xhci cleanups.

Thanks
Mathias

Fabio Estevam (1):
  usb: xhci-pci: Add TUSB73x0 definitions

Henry Tseng (1):
  usb: xhci: return an error if the host is not halted

Hongyu Xie (1):
  xhci: check device notification type before forwarding wake event

Mathias Nyman (1):
  xhci: Prevent invalid vdev dereference during sideband unregister

Michal Pecio (6):
  usb: xhci: Unlock for command abort polling
  usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun
  usb: xhci: Don't set the skip flag on non-isoc endpoints
  usb: xhci: Shorten the TD skipping loop
  usb: xhci: Rework and improve the TD matching and skipping logic
  usb: xhci: Fix bounce buffer overflow

Sang-Hoon Choi (1):
  xhci: dbc: lock the minor IDR on registration failure

Umang Jain (1):
  early: usb: xhci-dbc: Handle out of bounds xhci-xdbc capability

Wesley Cheng (1):
  usb: xhci: sideband: fix ring sg table for sub-page TRB segments

Zain Aboobacker (1):
  usb: xhci: fix typos in comments

 drivers/usb/early/xhci-dbc.c     |  81 +++++++++-
 drivers/usb/early/xhci-dbc.h     |   1 +
 drivers/usb/host/xhci-dbgtty.c   |   2 +
 drivers/usb/host/xhci-pci.c      |  11 +-
 drivers/usb/host/xhci-ring.c     | 247 +++++++++++++++----------------
 drivers/usb/host/xhci-sideband.c |  57 +++----
 drivers/usb/host/xhci.c          |  27 ++--
 drivers/usb/host/xhci.h          |   5 +
 8 files changed, 246 insertions(+), 185 deletions(-)

-- 
2.43.0

[PATCH 01/14] early: usb: xhci-dbc: Handle out of bounds xhci-xdbc capability

[Mathias Nyman]

From: Umang Jain <uajain@igalia.com>

Currently, the early xhci-dbc assumes that the entire PCIe memory IO
can be entirely mapped  within the fixed boot time mappings
dictated by NR_FIX_BTMAPS. This patch handles the case where the PCIe
memory IO size can be larger than the fixed boot time mappings and
query the xhci debug extended capability in xdbc_map_pci_mmio().

This commit ensures that the xHCI debug capability can still be queried
when the PCIe memory IO space exceeds the fixmap size. In this scenario,
the base address is mapped uptil fixmap size and debug capabilities are
queried thereafter. Iterating over the entire PCIe BAR address size is
left for future improvement as and when, such a case arises.

Additionally, this brings the need to track the early_ioremap() mapped
size separately hence, introduce additional struct member xhci_base_length
in struct xdbc_state.

Signed-off-by: Umang Jain <uajain@igalia.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/early/xhci-dbc.c | 81 ++++++++++++++++++++++++++++++++----
 drivers/usb/early/xhci-dbc.h |  1 +
 2 files changed, 74 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index 41118bba9197..f2ed8e52cc56 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -35,10 +35,23 @@ static bool early_console_keep;
 static inline void xdbc_trace(const char *fmt, ...) { }
 #endif /* XDBC_TRACE */
 
+/* Size of xHCI debug capability structure as per section 7.6.8 of xHCI spec. */
+#define XDBC_MAPPING_SIZE	64
+
+enum xdbc_capability_flags {
+	XDBC_CAP_FLAG_NONE		= 0,
+	XDBC_CAP_FLAG_LEGACY		= 1 << 0,
+	XDBC_CAP_FLAG_PROTOCOL		= 1 << 1,
+	XDBC_CAP_FLAG_DEBUG		= 1 << 2,
+};
+
 static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
 {
-	u64 val64, sz64, mask64;
+	u64 val64, sz64, mask64, fixmap_size;
+	enum xdbc_capability_flags cap_flags = XDBC_CAP_FLAG_NONE;
+	bool found_all_caps = false;
 	void __iomem *base;
+	int offset;
 	u32 val, sz;
 	u8 byte;
 
@@ -85,7 +98,59 @@ static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
 
 	xdbc.xhci_start = val64;
 	xdbc.xhci_length = sz64;
-	base = early_ioremap(val64, sz64);
+
+	fixmap_size = NR_FIX_BTMAPS << PAGE_SHIFT;
+	if (sz64 < fixmap_size) {
+		xdbc.xhci_base_length = sz64;
+		return early_ioremap(val64, sz64);
+	}
+
+	/*
+	 * Base address size is greater than fixed size boot time mappings
+	 * hence, map maximum allowed fixmap size from base address and
+	 * determine if the required extended capabilities lies within the
+	 * fixmap.
+	 */
+	base = early_ioremap(val64, fixmap_size);
+	if (!base)
+		return NULL;
+
+	offset = xhci_find_next_ext_cap(base, 0, 0);
+
+	while (offset < fixmap_size) {
+		val = readl(base + offset);
+		switch (XHCI_EXT_CAPS_ID(val)) {
+		case XHCI_EXT_CAPS_DEBUG:
+			if (offset + XDBC_MAPPING_SIZE < fixmap_size)
+				cap_flags |= XDBC_CAP_FLAG_DEBUG;
+			break;
+		case XHCI_EXT_CAPS_PROTOCOL:
+			cap_flags |= XDBC_CAP_FLAG_PROTOCOL;
+			break;
+		case XHCI_EXT_CAPS_LEGACY:
+			cap_flags |= XDBC_CAP_FLAG_LEGACY;
+			break;
+		}
+
+		if ((cap_flags & XDBC_CAP_FLAG_DEBUG) &&
+		    (cap_flags & XDBC_CAP_FLAG_PROTOCOL) &&
+		    (cap_flags & XDBC_CAP_FLAG_LEGACY)) {
+			found_all_caps = true;
+			break;
+		}
+
+		offset = xhci_find_next_ext_cap(base, offset, 0);
+		if (!offset)
+			break;
+	}
+
+	if (found_all_caps) {
+		xdbc.xhci_base_length = fixmap_size;
+	} else {
+		early_iounmap(base, fixmap_size);
+		xdbc.xhci_base_length = 0;
+		base = NULL;
+	}
 
 	return base;
 }
@@ -643,9 +708,9 @@ int __init early_xdbc_parse_parameter(char *s, int keep_early)
 	offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_DEBUG);
 	if (!offset) {
 		pr_notice("xhci host doesn't support debug capability\n");
-		early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+		early_iounmap(xdbc.xhci_base, xdbc.xhci_base_length);
 		xdbc.xhci_base = NULL;
-		xdbc.xhci_length = 0;
+		xdbc.xhci_base_length = 0;
 
 		return -ENODEV;
 	}
@@ -682,9 +747,9 @@ int __init early_xdbc_setup_hardware(void)
 		xdbc.table_base = NULL;
 		xdbc.out_buf = NULL;
 
-		early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+		early_iounmap(xdbc.xhci_base, xdbc.xhci_base_length);
 		xdbc.xhci_base = NULL;
-		xdbc.xhci_length = 0;
+		xdbc.xhci_base_length = 0;
 	}
 
 	return ret;
@@ -987,7 +1052,7 @@ static int __init xdbc_init(void)
 	}
 
 	raw_spin_lock_irqsave(&xdbc.lock, flags);
-	early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+	early_iounmap(xdbc.xhci_base, xdbc.xhci_base_length);
 	xdbc.xhci_base = base;
 	offset = xhci_find_next_ext_cap(xdbc.xhci_base, 0, XHCI_EXT_CAPS_DEBUG);
 	xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
@@ -1004,7 +1069,7 @@ static int __init xdbc_init(void)
 	memblock_phys_free(xdbc.table_dma, PAGE_SIZE);
 	memblock_phys_free(xdbc.out_dma, PAGE_SIZE);
 	writel(0, &xdbc.xdbc_reg->control);
-	early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
+	early_iounmap(xdbc.xhci_base, xdbc.xhci_base_length);
 
 	return ret;
 }
diff --git a/drivers/usb/early/xhci-dbc.h b/drivers/usb/early/xhci-dbc.h
index 8b4d71de45fc..e2aefb796084 100644
--- a/drivers/usb/early/xhci-dbc.h
+++ b/drivers/usb/early/xhci-dbc.h
@@ -144,6 +144,7 @@ struct xdbc_state {
 	u32			dev;
 	u32			func;
 	void __iomem		*xhci_base;
+	size_t			xhci_base_length;
 	u64			xhci_start;
 	size_t			xhci_length;
 	int			port_number;
-- 
2.43.0

Re: [PATCH 01/14] early: usb: xhci-dbc: Handle out of bounds xhci-xdbc capability

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] usb: xhci-dbc: OOB MMIO read during capability traversal via truncated early_ioremap
- [High] usb: xhci-dbc: strictly requiring optional LEGACY capability breaks valid hardware
- [High] usb: xhci-dbc: missing bounds check allows OOB access to LEGACY capability registers
- [High] usb: xhci-dbc: lockless unmap on ioremap failure causes UAF with active console
- [Medium] usb: xhci-dbc: off-by-one check incorrectly rejects capabilities ending at fixmap boundary
- [Medium] usb: xhci-dbc: missing offset=0 check processes base registers as capability
--

commit c5569785c857fd1c3eacf9d2844609e296560040
Author: Umang Jain <uajain@igalia.com>

early: usb: xhci-dbc: Handle out of bounds xhci-xdbc capability

This commit handles the case where the PCIe memory IO size exceeds the
fixed boot time mappings dictated by NR_FIX_BTMAPS. It ensures that the
xHCI debug capability can still be queried by mapping the base address
up to the fixmap size and querying capabilities within that range. It also
introduces xhci_base_length to track the early_ioremap() mapped size.

> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
> index 41118bba91978..f2ed8e52cc56c 100644
> --- a/drivers/usb/early/xhci-dbc.c
> +++ b/drivers/usb/early/xhci-dbc.c

[ ... ]

> @@ -85,7 +98,59 @@ static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
>  
>  	xdbc.xhci_start = val64;
>  	xdbc.xhci_length = sz64;
> -	base = early_ioremap(val64, sz64);
> +
> +	fixmap_size = NR_FIX_BTMAPS << PAGE_SHIFT;
> +	if (sz64 < fixmap_size) {
> +		xdbc.xhci_base_length = sz64;
> +		return early_ioremap(val64, sz64);
> +	}
> +
> +	/*
> +	 * Base address size is greater than fixed size boot time mappings
> +	 * hence, map maximum allowed fixmap size from base address and
> +	 * determine if the required extended capabilities lies within the
> +	 * fixmap.
> +	 */
> +	base = early_ioremap(val64, fixmap_size);
> +	if (!base)
> +		return NULL;
> +
> +	offset = xhci_find_next_ext_cap(base, 0, 0);
> +
> +	while (offset < fixmap_size) {
> +		val = readl(base + offset);

[Severity: Medium]
If xhci_find_next_ext_cap() returns 0 (indicating no capabilities found),
doesn't this loop unconditionally enter because 0 < fixmap_size?

Inside the loop, readl(base + 0) would read the base xHCI Capability
Registers rather than an extended capability header. XHCI_EXT_CAPS_ID(val)
could then mistakenly extract the CAPLENGTH byte, potentially misinterpreting
it as an extended capability ID like XDBC_CAP_FLAG_LEGACY.

Should there be an explicit check for offset == 0 before entering the loop?

> +		switch (XHCI_EXT_CAPS_ID(val)) {
> +		case XHCI_EXT_CAPS_DEBUG:
> +			if (offset + XDBC_MAPPING_SIZE < fixmap_size)

[Severity: Medium]
If the 64-byte DEBUG capability struct ends exactly at the fixmap_size
boundary (offset + XDBC_MAPPING_SIZE == fixmap_size), it is fully mapped
and safe to access.

Does the strict less-than operator here incorrectly reject a perfectly valid
capability that ends exactly at the boundary?

> +				cap_flags |= XDBC_CAP_FLAG_DEBUG;
> +			break;
> +		case XHCI_EXT_CAPS_PROTOCOL:
> +			cap_flags |= XDBC_CAP_FLAG_PROTOCOL;
> +			break;
> +		case XHCI_EXT_CAPS_LEGACY:
> +			cap_flags |= XDBC_CAP_FLAG_LEGACY;
> +			break;

[Severity: High]
This accepts the LEGACY capability without ensuring its extended registers
fit within the mapped fixmap region.

If the LEGACY capability header is located at fixmap_size - 4, the while
condition (offset < fixmap_size) is satisfied, and the header is read.
However, later in xdbc_bios_handoff(), the code accesses the control register
at offset + XHCI_LEGACY_CONTROL_OFFSET (offset + 4).

Will this result in an out-of-bounds MMIO read when offset + 4 equals
fixmap_size?

> +		}
> +
> +		if ((cap_flags & XDBC_CAP_FLAG_DEBUG) &&
> +		    (cap_flags & XDBC_CAP_FLAG_PROTOCOL) &&
> +		    (cap_flags & XDBC_CAP_FLAG_LEGACY)) {
> +			found_all_caps = true;
> +			break;
> +		}

[Severity: High]
According to Section 7.1 of the xHCI specification, the "USB Legacy Support
Capability" is strictly optional.

By requiring XDBC_CAP_FLAG_LEGACY to be present here, does this patch
introduce a regression that breaks DbC initialization on valid, compliant
hardware that safely omits it?

> +
> +		offset = xhci_find_next_ext_cap(base, offset, 0);

[Severity: High]
Does this blindly request the next offset without verifying bounds beforehand?

Inside xhci_find_next_ext_cap(), the helper computes the offset to the next
capability and immediately dereferences it:

xhci_find_next_ext_cap():
    do {
        ...
        next = XHCI_EXT_CAPS_NEXT(val);
        offset += next << 2;
    } while (next);  // loops back to readl(base + offset)

If the computed offset exceeds fixmap_size, readl() will access unmapped
MMIO memory and cause a kernel page fault BEFORE xhci_find_next_ext_cap()
returns.

The offset < fixmap_size check at the top of the loop is too late to prevent
this out-of-bounds access.

> +		if (!offset)
> +			break;
> +	}

[ ... ]

> @@ -1004,7 +1069,7 @@ static int __init xdbc_init(void)
>  	memblock_phys_free(xdbc.table_dma, PAGE_SIZE);
>  	memblock_phys_free(xdbc.out_dma, PAGE_SIZE);
>  	writel(0, &xdbc.xdbc_reg->control);
> -	early_iounmap(xdbc.xhci_base, xdbc.xhci_length);
> +	early_iounmap(xdbc.xhci_base, xdbc.xhci_base_length);

[Severity: High]
If keep_bootcon is used, the console remains active. If the permanent
ioremap() fails in xdbc_init(), execution jumps to the free_and_quit error
path.

This error path locklessly unmaps the underlying MMIO regions by calling
early_iounmap() and writel() without acquiring xdbc.lock and without
unregistering the console.

Concurrently, if another CPU calls printk(), it could invoke
early_xdbc_write(), which acquires xdbc.lock and attempts to write to the
now-unmapped xdbc.xhci_base. Can this cause an MMIO use-after-free or page
fault?

>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=1

Re: [PATCH 01/14] early: usb: xhci-dbc: Handle out of bounds xhci-xdbc capability

[Mathias Nyman]

On 10/9/26 13:11, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] usb: xhci-dbc: OOB MMIO read during capability traversal via truncated early_ioremap
> - [High] usb: xhci-dbc: strictly requiring optional LEGACY capability breaks valid hardware
> - [High] usb: xhci-dbc: missing bounds check allows OOB access to LEGACY capability registers
> - [High] usb: xhci-dbc: lockless unmap on ioremap failure causes UAF with active console
> - [Medium] usb: xhci-dbc: off-by-one check incorrectly rejects capabilities ending at fixmap boundary
> - [Medium] usb: xhci-dbc: missing offset=0 check processes base registers as capability
> --
> 
> commit c5569785c857fd1c3eacf9d2844609e296560040
> Author: Umang Jain <uajain@igalia.com>
> 
> early: usb: xhci-dbc: Handle out of bounds xhci-xdbc capability
> 
> This commit handles the case where the PCIe memory IO size exceeds the
> fixed boot time mappings dictated by NR_FIX_BTMAPS. It ensures that the
> xHCI debug capability can still be queried by mapping the base address
> up to the fixmap size and querying capabilities within that range. It also
> introduces xhci_base_length to track the early_ioremap() mapped size.
> 
>> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
>> index 41118bba91978..f2ed8e52cc56c 100644
>> --- a/drivers/usb/early/xhci-dbc.c
>> +++ b/drivers/usb/early/xhci-dbc.c
> 
> [ ... ]
> 
>> @@ -85,7 +98,59 @@ static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
>>   
>>   	xdbc.xhci_start = val64;
>>   	xdbc.xhci_length = sz64;
>> -	base = early_ioremap(val64, sz64);
>> +
>> +	fixmap_size = NR_FIX_BTMAPS << PAGE_SHIFT;
>> +	if (sz64 < fixmap_size) {
>> +		xdbc.xhci_base_length = sz64;
>> +		return early_ioremap(val64, sz64);
>> +	}
>> +
>> +	/*
>> +	 * Base address size is greater than fixed size boot time mappings
>> +	 * hence, map maximum allowed fixmap size from base address and
>> +	 * determine if the required extended capabilities lies within the
>> +	 * fixmap.
>> +	 */
>> +	base = early_ioremap(val64, fixmap_size);
>> +	if (!base)
>> +		return NULL;
>> +
>> +	offset = xhci_find_next_ext_cap(base, 0, 0);
>> +
>> +	while (offset < fixmap_size) {
>> +		val = readl(base + offset);
> 
> [Severity: Medium]
> If xhci_find_next_ext_cap() returns 0 (indicating no capabilities found),
> doesn't this loop unconditionally enter because 0 < fixmap_size?
> 
> Inside the loop, readl(base + 0) would read the base xHCI Capability
> Registers rather than an extended capability header. XHCI_EXT_CAPS_ID(val)
> could then mistakenly extract the CAPLENGTH byte, potentially misinterpreting
> it as an extended capability ID like XDBC_CAP_FLAG_LEGACY.
> 
> Should there be an explicit check for offset == 0 before entering the loop?

will fix, but not a real world issue as all xHC hosts have extended capabilities.
without extended capabilities host won't have any ports.

> 
>> +		switch (XHCI_EXT_CAPS_ID(val)) {
>> +		case XHCI_EXT_CAPS_DEBUG:
>> +			if (offset + XDBC_MAPPING_SIZE < fixmap_size)
> 
> [Severity: Medium]
> If the 64-byte DEBUG capability struct ends exactly at the fixmap_size
> boundary (offset + XDBC_MAPPING_SIZE == fixmap_size), it is fully mapped
> and safe to access.
> 
> Does the strict less-than operator here incorrectly reject a perfectly valid
> capability that ends exactly at the boundary?
> 

will fix, this just rejects a border case that could work.

>> +				cap_flags |= XDBC_CAP_FLAG_DEBUG;
>> +			break;
>> +		case XHCI_EXT_CAPS_PROTOCOL:
>> +			cap_flags |= XDBC_CAP_FLAG_PROTOCOL;
>> +			break;
>> +		case XHCI_EXT_CAPS_LEGACY:
>> +			cap_flags |= XDBC_CAP_FLAG_LEGACY;
>> +			break;
> 
> [Severity: High]
> This accepts the LEGACY capability without ensuring its extended registers
> fit within the mapped fixmap region.
> 
> If the LEGACY capability header is located at fixmap_size - 4, the while
> condition (offset < fixmap_size) is satisfied, and the header is read.
> However, later in xdbc_bios_handoff(), the code accesses the control register
> at offset + XHCI_LEGACY_CONTROL_OFFSET (offset + 4).
> 
> Will this result in an out-of-bounds MMIO read when offset + 4 equals
> fixmap_size?
> 

will fix, size of legacy capability is 8 bytes. Odd and extremely unlucky
if these 8 bytes would cross the NR_FIX_BTMAPS << PAGE_SHIFT limit.

>> +		}
>> +
>> +		if ((cap_flags & XDBC_CAP_FLAG_DEBUG) &&
>> +		    (cap_flags & XDBC_CAP_FLAG_PROTOCOL) &&
>> +		    (cap_flags & XDBC_CAP_FLAG_LEGACY)) {
>> +			found_all_caps = true;
>> +			break;
>> +		}
> 
> [Severity: High]
> According to Section 7.1 of the xHCI specification, the "USB Legacy Support
> Capability" is strictly optional.
> 
> By requiring XDBC_CAP_FLAG_LEGACY to be present here, does this patch
> introduce a regression that breaks DbC initialization on valid, compliant
> hardware that safely omits it?

Not an issue. No regression.

This patch adds early dbc support for more hosts. Doesn't remove support.

> 
>> +
>> +		offset = xhci_find_next_ext_cap(base, offset, 0);
> 
> [Severity: High]
> Does this blindly request the next offset without verifying bounds beforehand?
> 
> Inside xhci_find_next_ext_cap(), the helper computes the offset to the next
> capability and immediately dereferences it:
> 
> xhci_find_next_ext_cap():
>      do {
>          ...
>          next = XHCI_EXT_CAPS_NEXT(val);
>          offset += next << 2;
>      } while (next);  // loops back to readl(base + offset)
> 
> If the computed offset exceeds fixmap_size, readl() will access unmapped
> MMIO memory and cause a kernel page fault BEFORE xhci_find_next_ext_cap()
> returns.
> 
> The offset < fixmap_size check at the top of the loop is too late to prevent
> this out-of-bounds access.

True, this was actually a good find.

xhci_find_next_ext_cap(base, offset, 0) does unnecessarily read the next
capability even if we don't care about the content (the capability id)

xhci_find_next_cap() was never designed to work with a limit size mmio map
of this PCI device.

Could be somewhat easily fixed in xhci_find_next_ext_cap(), but I don't want
to touch it at this stage without properly testing.
It's used everywhere, and risk of regression is high.

I'll drop this until properly fixed.

Thanks
Mathias

[PATCH 02/14] usb: xhci: return an error if the host is not halted

[Mathias Nyman]

From: Henry Tseng <henrytseng@qnap.com>

xhci_reset() returns 0 when the host is not halted, without ever writing
CMD_RESET. Every other path that fails to reset the host returns an
error, so callers that check the return value are told the reset
succeeded on the one path where it did not happen.

Return -EBUSY when the reset is aborted because the host is not halted.

Signed-off-by: Henry Tseng <henrytseng@qnap.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a9e47e178c28..2af6a7b91e55 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -195,7 +195,7 @@ int xhci_reset(struct xhci_hcd *xhci, u64 timeout_us)
 
 	if ((state & STS_HALT) == 0) {
 		xhci_warn(xhci, "Host controller not halted, aborting reset.\n");
-		return 0;
+		return -EBUSY;
 	}
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "// Reset the HC");
-- 
2.43.0

Re: [PATCH 02/14] usb: xhci: return an error if the host is not halted

[sashiko-bot]

> From: Henry Tseng <henrytseng@qnap.com>
> 
> xhci_reset() returns 0 when the host is not halted, without ever writing
> CMD_RESET. Every other path that fails to reset the host returns an
> error, so callers that check the return value are told the reset
> succeeded on the one path where it did not happen.
> 
> Return -EBUSY when the reset is aborted because the host is not halted.
> 
> Signed-off-by: Henry Tseng <henrytseng@qnap.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=2

[PATCH 03/14] usb: xhci: Unlock for command abort polling

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

xhci_abort_cmd_ring() requests abort, waits for the CRR bit to clear,
drops xhci->lock and waits for the Command Ring Stopped event.

The CRR wait timeout is 5 seconds as suggested by xHCI 4.6.1.2, which
means that if the xHC fails to complete the operation at all, we poll
with the lock held and IRQs disabled for several seconds. If any other
CPU tries to acquire the lock, it will spin likewise. IRQs get delays,
drivers log errors, tasks freeze, it's a mess.

So drop the lock earlier, before waiting for the CRR bit. It should be
safe - the sole caller sets cmd_ring_state to CMD_RING_STATE_ABORTED
before calling us, which will prevent others from ringing the command
doorbell and interfering with the abort. Queuing new commands during
this time poses no danger, and if the command we try to abort actually
completes concurrently, existing code already needs to deal with this.
And in my testing it does - it's trivial to trigger this on ASM1042,
where Address Device can't be aborted, but it completes as soon as the
offending device is unplugged, including during abort attempt.

Note that the lock still covers reinit_completion(), so it won't race
with complete() being called by the event handler. And works are not
reentrant, so another timeout can't expire while the lock is dropped.
We will configure timeout anew when restarting the ring.

One other difference is that now we also drop the lock if abort fails.
This too should be harmless. Commands queued during this time will be
released like any other pending commands. If the aborted command does
complete before we regain the lock, it's a waste, but not regression.

Reported-by: Pedro Fonseca <pedro@fonseca.com.pt>
Link: https://lore.kernel.org/linux-usb/16f65081-5a3c-4c30-9811-9017796a3373@fonseca.com.pt/
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-ring.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ec278a9f9540..82dd93c2afdd 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -494,7 +494,7 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
 	struct xhci_segment *new_seg	= xhci->cmd_ring->deq_seg;
 	union xhci_trb *new_deq		= xhci->cmd_ring->dequeue;
 	u64 crcr;
-	int ret;
+	int ret, completed;
 
 	xhci_dbg(xhci, "Abort command ring\n");
 
@@ -521,25 +521,27 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
 	 * In the future we should distinguish between -ENODEV and -ETIMEDOUT
 	 * and try to recover a -ETIMEDOUT with a host controller reset.
 	 */
+	spin_unlock_irqrestore(&xhci->lock, flags);
 	ret = xhci_handshake(&xhci->op_regs->cmd_ring,
 			CMD_RING_RUNNING, 0, 5 * 1000 * 1000);
-	if (ret < 0) {
-		xhci_err(xhci, "Abort failed to stop command ring: %d\n", ret);
-		xhci_halt(xhci);
-		xhci_hc_died(xhci);
-		return ret;
-	}
 	/*
 	 * Writing the CMD_RING_ABORT bit should cause a cmd completion event,
 	 * however on some host hw the CMD_RING_RUNNING bit is correctly cleared
 	 * but the completion event in never sent. Wait 2 secs (arbitrary
 	 * number) to handle those cases after negation of CMD_RING_RUNNING.
 	 */
-	spin_unlock_irqrestore(&xhci->lock, flags);
-	ret = wait_for_completion_timeout(&xhci->cmd_ring_stop_completion,
+	if (ret >= 0)
+		completed = wait_for_completion_timeout(&xhci->cmd_ring_stop_completion,
 					  msecs_to_jiffies(2000));
 	spin_lock_irqsave(&xhci->lock, flags);
-	if (!ret) {
+
+	if (ret < 0) {
+		xhci_err(xhci, "Abort failed to stop command ring: %d\n", ret);
+		xhci_halt(xhci);
+		xhci_hc_died(xhci);
+		return ret;
+	}
+	if (!completed) {
 		xhci_dbg(xhci, "No stop event for abort, ring start fail?\n");
 		xhci_cleanup_command_queue(xhci);
 	} else {
-- 
2.43.0

Re: [PATCH 03/14] usb: xhci: Unlock for command abort polling

[sashiko-bot]

> From: Michal Pecio <michal.pecio@gmail.com>
> 
> xhci_abort_cmd_ring() requests abort, waits for the CRR bit to clear,
> drops xhci->lock and waits for the Command Ring Stopped event.
> 
> The CRR wait timeout is 5 seconds as suggested by xHCI 4.6.1.2, which
> means that if the xHC fails to complete the operation at all, we poll
> with the lock held and IRQs disabled for several seconds. If any other
> CPU tries to acquire the lock, it will spin likewise. IRQs get delays,
> drivers log errors, tasks freeze, it's a mess.
> 
> So drop the lock earlier, before waiting for the CRR bit. It should be
> safe - the sole caller sets cmd_ring_state to CMD_RING_STATE_ABORTED
> before calling us, which will prevent others from ringing the command
> doorbell and interfering with the abort. Queuing new commands during
> [ ... ]
> 
> Reported-by: Pedro Fonseca <pedro@fonseca.com.pt>
> Link: https://lore.kernel.org/linux-usb/16f65081-5a3c-4c30-9811-9017796a3373@fonseca.com.pt/
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=3

[PATCH 04/14] usb: xhci: fix typos in comments

[Mathias Nyman]

From: Zain Aboobacker <zainaboobacker33@gmail.com>

Fix various spelling mistakes in comments found by codespell.

Assisted-by: Claude:claude-opus-5-5 codespell
Signed-off-by: Zain Aboobacker <zainaboobacker33@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-ring.c | 14 +++++++-------
 drivers/usb/host/xhci.c      | 14 +++++++-------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 82dd93c2afdd..cbce9f8f07fa 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -243,7 +243,7 @@ static void inc_enq_past_link(struct xhci_hcd *xhci, struct xhci_ring *ring, u32
 		 * fixed in the 0.96 specification errata, but we have to assume that all 0.95
 		 * xHCI hardware can't handle the chain bit being cleared on a link TRB.
 		 *
-		 * On 0.95 and some 0.96 HCs the chain bit is set once at segment initalization
+		 * On 0.95 and some 0.96 HCs the chain bit is set once at segment initialization
 		 * and never changed here. On all others, modify it as requested by the caller.
 		 */
 		if (!xhci_link_chain_quirk(xhci, ring->type)) {
@@ -663,7 +663,7 @@ struct xhci_ring *xhci_triad_to_transfer_ring(struct xhci_hcd *xhci,
  * Get the hw dequeue pointer xHC stopped on, either directly from the
  * endpoint context, or if streams are in use from the stream context.
  * The returned hw_dequeue contains the lowest four bits with cycle state
- * and possbile stream context type.
+ * and possible stream context type.
  */
 static u64 xhci_get_hw_deq(struct xhci_hcd *xhci, struct xhci_virt_device *vdev,
 			   unsigned int ep_index, unsigned int stream_id)
@@ -991,7 +991,7 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
 	int err;
 
 	/*
-	 * Avoid resetting endpoint if link is inactive or device disonnected.
+	 * Avoid resetting endpoint if link is inactive or device disconnected.
 	 * Can cause host hang.
 	 * Device will be reset to recover an inactive link, so don't do anything
 	 */
@@ -1386,7 +1386,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci,
  * held for the URBs to finish during device disconnect, blocking host remove.
  *
  * Call with xhci->lock held.
- * lock is relased and re-acquired while giving back urb.
+ * lock is released and re-acquired while giving back urb.
  */
 void xhci_hc_died(struct xhci_hcd *xhci)
 {
@@ -1971,7 +1971,7 @@ static void handle_device_notification(struct xhci_hcd *xhci,
 }
 
 /*
- * Quirk hanlder for errata seen on Cavium ThunderX2 processor XHCI
+ * Quirk handler for errata seen on Cavium ThunderX2 processor XHCI
  * Controller.
  * As per ThunderX2errata-129 USB 2 device may come up as USB 1
  * If a connection to a USB 1 device is followed by another connection
@@ -3089,7 +3089,7 @@ static void xhci_clear_interrupt_pending(struct xhci_interrupter *ir)
 
 /*
  * Handle all OS-owned events on an interrupter event ring. It may drop
- * and reaquire xhci->lock between event processing.
+ * and reacquire xhci->lock between event processing.
  */
 static int xhci_handle_events(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
 			      bool skip_events)
@@ -3803,7 +3803,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		/*
 		 * If next available TRB is the Link TRB in the ring segment then
 		 * enqueue a No Op TRB, this can prevent the Setup and Data Stage
-		 * TRB to be breaked by the Link TRB.
+		 * TRB to be broken by the Link TRB.
 		 */
 		if (last_trb_on_seg(ep_ring->enq_seg, ep_ring->enqueue + 1)) {
 			field = TRB_TYPE(TRB_TR_NOOP) | ep_ring->cycle_state;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2af6a7b91e55..9564dde8bb34 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -406,7 +406,7 @@ static void compliance_mode_recovery(struct timer_list *t)
  * The quirk creates a timer that polls every 2 seconds the link state of
  * each host controller's port and recovers it by issuing a Warm reset
  * if Compliance mode is detected, otherwise the port will become "dead" (no
- * device connections or disconnections will be detected anymore). Becasue no
+ * device connections or disconnections will be detected anymore). Because no
  * status event is generated when entering compliance mode (per xhci spec),
  * this quirk is needed on systems that have the failing hardware installed.
  */
@@ -796,7 +796,7 @@ static void xhci_save_registers(struct xhci_hcd *xhci)
 	xhci->s3.config_reg = readl(&xhci->op_regs->config_reg);
 
 	/* save both primary and all secondary interrupters */
-	/* fixme, shold we lock  to prevent race with remove secondary interrupter? */
+	/* fixme, should we lock  to prevent race with remove secondary interrupter? */
 	for (i = 0; i < xhci->max_interrupters; i++) {
 		ir = xhci->interrupters[i];
 		if (!ir)
@@ -2046,7 +2046,7 @@ int xhci_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
 
 	/*
 	 * Configuration and alternate setting changes must be done in
-	 * process context, not interrupt context (or so documenation
+	 * process context, not interrupt context (or so documentation
 	 * for usb_set_interface() and usb_set_configuration() claim).
 	 */
 	if (xhci_endpoint_init(xhci, virt_dev, udev, ep, GFP_NOIO) < 0) {
@@ -3302,7 +3302,7 @@ static void xhci_endpoint_disable(struct usb_hcd *hcd,
  * state. For software that wishes to reset the data toggle or sequence number
  * of an endpoint that isn't in the halted state this function will issue a
  * configure endpoint command with the Drop and Add bits set for the target
- * endpoint. Refer to the additional note in xhci spcification section 4.6.8.
+ * endpoint. Refer to the additional note in xhci specification section 4.6.8.
  *
  * vdev may be lost due to xHC restore error and re-initialization during S3/S4
  * resume. A new vdev will be allocated later by xhci_discover_or_reset_device()
@@ -4296,7 +4296,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
 		pm_runtime_get_noresume(hcd->self.controller);
 
 	/* Is this a LS or FS device under a HS hub? */
-	/* Hub or peripherial? */
+	/* Hub or peripheral? */
 	return 1;
 
 disable_slot:
@@ -4512,7 +4512,7 @@ static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
 
 /*
  * Transfer the port index into real index in the HW port status
- * registers. Caculate offset between the port's PORTSC register
+ * registers. Calculate offset between the port's PORTSC register
  * and port status base. Divide the number of per port register
  * to get the real index. The raw port number bases 1.
  */
@@ -5394,7 +5394,7 @@ static void xhci_hcd_init_usb3_data(struct xhci_hcd *xhci, struct usb_hcd *hcd)
 	/*
 	 * Early xHCI 1.1 spec did not mention USB 3.1 capable hosts
 	 * should return 0x31 for sbrn, or that the minor revision
-	 * is a two digit BCD containig minor and sub-minor numbers.
+	 * is a two digit BCD containing minor and sub-minor numbers.
 	 * This was later clarified in xHCI 1.2.
 	 *
 	 * Some USB 3.1 capable hosts therefore have sbrn 0x30, and
-- 
2.43.0

Re: [PATCH 04/14] usb: xhci: fix typos in comments

[sashiko-bot]

> From: Zain Aboobacker <zainaboobacker33@gmail.com>
> 
> Fix various spelling mistakes in comments found by codespell.
> 
> Assisted-by: Claude:claude-opus-5-5 codespell
> Signed-off-by: Zain Aboobacker <zainaboobacker33@gmail.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=4

[PATCH 05/14] xhci: check device notification type before forwarding wake event

[Mathias Nyman]

From: Hongyu Xie <xiehongyu1@kylinos.cn>

The xHCI driver programs the Device Notification Control register to
only enable the Function Wake device notification (N1), so any Device
Notification Event TRB received is expected to be a function wake
notification. handle_device_notification() does however not check the
Notification Type field of the event (xHCI 1.2 section 6.4.2.7, DW0
bits 7:4), and forwards every device notification event as a function
wake.

A host controller that delivers an unexpected notification type (e.g.
due to broken firmware or emulation) would trigger a spurious wake
notification on the parent hub.

Parse the notification type and drop events other than Function Wake
with a warning, mirroring the slot ID validation in the same function.

DEV_NOTE_FWAKE is the DNCTRL register bit for notification type 1
(N1), while the event TRB carries the notification type value itself,
so add a separate DEV_NOTE_TYPE_FWAKE constant for the comparison.

[mn:] reduce warning to a debug message

Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-ring.c | 9 +++++++++
 drivers/usb/host/xhci.h      | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index cbce9f8f07fa..7f480db2983e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1954,6 +1954,7 @@ static void handle_device_notification(struct xhci_hcd *xhci,
 		union xhci_trb *event)
 {
 	u32 slot_id;
+	u32 type;
 	struct usb_device *udev;
 
 	slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->generic.field[3]));
@@ -1963,6 +1964,14 @@ static void handle_device_notification(struct xhci_hcd *xhci,
 		return;
 	}
 
+	/* xHCI 1.2 6.4.2.7: Notification Type is DW0 bits 7:4 */
+	type = TRB_TO_DEV_NOTE_TYPE(le32_to_cpu(event->generic.field[0]));
+	if (type != DEV_NOTE_TYPE_FWAKE) {
+		xhci_dbg(xhci, "Unsupported device notification type %u for slot ID %u\n",
+				type, slot_id);
+		return;
+	}
+
 	xhci_dbg(xhci, "Device Wake Notification event for slot ID %u\n",
 			slot_id);
 	udev = xhci->devs[slot_id]->udev;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index c7bfa7f028d3..ec4bfeb4887c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -187,6 +187,8 @@ struct xhci_op_regs {
  * SW does need to pay attention to function wake notifications.
  */
 #define	DEV_NOTE_FWAKE		BIT(1)
+/* Notification Type value carried by a Device Notification Event TRB (6.4.2.7) */
+#define	DEV_NOTE_TYPE_FWAKE	1
 
 /* CRCR - Command Ring Control Register - cmd_ring bitmasks */
 /* bit 0 - Cycle bit indicates the ownership of the command ring */
@@ -996,6 +998,9 @@ enum xhci_ep_reset_type {
 #define TRB_TO_PACKET_TYPE(p)		((p) & 0x1f)
 #define TRB_TO_ROOTHUB_PORT(p)		(((p) & (0xff << 24)) >> 24)
 
+/* Device Notification Event TRB fields, 6.4.2.7 */
+#define TRB_TO_DEV_NOTE_TYPE(p)		(((p) & (0xf << 4)) >> 4)
+
 enum xhci_setup_dev {
 	SETUP_CONTEXT_ONLY,
 	SETUP_CONTEXT_ADDRESS,
-- 
2.43.0

Re: [PATCH 05/14] xhci: check device notification type before forwarding wake event

[sashiko-bot]

> From: Hongyu Xie <xiehongyu1@kylinos.cn>
> 
> The xHCI driver programs the Device Notification Control register to
> only enable the Function Wake device notification (N1), so any Device
> Notification Event TRB received is expected to be a function wake
> notification. handle_device_notification() does however not check the
> Notification Type field of the event (xHCI 1.2 section 6.4.2.7, DW0
> bits 7:4), and forwards every device notification event as a function
> wake.
> 
> A host controller that delivers an unexpected notification type (e.g.
> due to broken firmware or emulation) would trigger a spurious wake
> notification on the parent hub.
> 
> Parse the notification type and drop events other than Function Wake
> with a warning, mirroring the slot ID validation in the same function.
> 
> DEV_NOTE_FWAKE is the DNCTRL register bit for notification type 1
> (N1), while the event TRB carries the notification type value itself,
> so add a separate DEV_NOTE_TYPE_FWAKE constant for the comparison.
> 
> [mn:] reduce warning to a debug message
> 
> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
> Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=5

[PATCH 06/14] xhci: dbc: lock the minor IDR on registration failure

[Mathias Nyman]

From: Sang-Hoon Choi <csh0052@gmail.com>

dbc_tty_minors is protected by dbc_tty_minors_lock when entries are
allocated and during normal device removal. The registration error path
removes an entry without taking that lock. Different DbC instances have
separate event work items, so this removal can race with an IDR update
for another instance.

Take the same mutex around the error-path removal.

Fixes: e1ec140f273e ("xhci: dbgtty: use IDR to support several dbc instances.")
Reported-by: Changyul Lee <lcy8047@gmail.com>
Assisted-by: LLM
Signed-off-by: Sang-Hoon Choi <csh0052@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-dbgtty.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
index 3d51e8d82659..2249cc16800c 100644
--- a/drivers/usb/host/xhci-dbgtty.c
+++ b/drivers/usb/host/xhci-dbgtty.c
@@ -535,7 +535,9 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc)
 err_free_fifo:
 	kfifo_free(&port->port.xmit_fifo);
 err_exit_port:
+	mutex_lock(&dbc_tty_minors_lock);
 	idr_remove(&dbc_tty_minors, port->minor);
+	mutex_unlock(&dbc_tty_minors_lock);
 err_idr:
 	xhci_dbc_tty_exit_port(port);
 
-- 
2.43.0

Re: [PATCH 06/14] xhci: dbc: lock the minor IDR on registration failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: xhci: dbc: UAF due to premature IDR exposure and missing TTY hangup on error path
--

commit 99c18a1b8449e8389ccac896ceadcc979dbd6a57
Author: Sang-Hoon Choi <csh0052@gmail.com>

xhci: dbc: lock the minor IDR on registration failure

This commit fixes a race condition during DbC TTY registration failure
by adding mutex protection around the removal of the minor IDR entry.

> diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
> index 3d51e8d82659d..2249cc16800cc 100644
> --- a/drivers/usb/host/xhci-dbgtty.c
> +++ b/drivers/usb/host/xhci-dbgtty.c
> @@ -535,7 +535,9 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc)
>  err_free_fifo:
>  	kfifo_free(&port->port.xmit_fifo);
>  err_exit_port:
> +	mutex_lock(&dbc_tty_minors_lock);
>  	idr_remove(&dbc_tty_minors, port->minor);
> +	mutex_unlock(&dbc_tty_minors_lock);
>  err_idr:
>  	xhci_dbc_tty_exit_port(port);

[Severity: High]
Does this error path leave concurrent TTY sessions pointing to freed memory?

Earlier in xhci_dbc_tty_register_device(), the port is added to the globally
visible dbc_tty_minors IDR before internal structures are fully allocated:

	mutex_lock(&dbc_tty_minors_lock);
	port->minor = idr_alloc(&dbc_tty_minors, port, 0, 64, GFP_KERNEL);
	mutex_unlock(&dbc_tty_minors_lock);

A concurrent open() on the device node routes to dbc_tty_install(), which
retrieves the port and installs the TTY without verifying port->registered:

	port = idr_find(&dbc_tty_minors, tty->index);
	...
	if (!port)
		return -ENXIO;

	tty->driver_data = port;

	return tty_port_install(&port->port, driver, tty);

If xhci_dbc_tty_register_device() fails later (for example, during
xhci_dbc_alloc_requests()), the error path frees the kfifo and destroys the
tty_port.

Because tty_port_tty_vhangup() is not called here to terminate the
concurrent TTY session, does this allow userspace to subsequently call
write() and access the freed xmit_fifo and destroyed tty_port?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=6

Re: [PATCH 06/14] xhci: dbc: lock the minor IDR on registration failure

[Greg KH]

On Fri, Oct 09, 2026 at 10:13:10AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] usb: xhci: dbc: UAF due to premature IDR exposure and missing TTY hangup on error path
> --
> 
> commit 99c18a1b8449e8389ccac896ceadcc979dbd6a57
> Author: Sang-Hoon Choi <csh0052@gmail.com>
> 
> xhci: dbc: lock the minor IDR on registration failure
> 
> This commit fixes a race condition during DbC TTY registration failure
> by adding mutex protection around the removal of the minor IDR entry.
> 
> > diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
> > index 3d51e8d82659d..2249cc16800cc 100644
> > --- a/drivers/usb/host/xhci-dbgtty.c
> > +++ b/drivers/usb/host/xhci-dbgtty.c
> > @@ -535,7 +535,9 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc)
> >  err_free_fifo:
> >  	kfifo_free(&port->port.xmit_fifo);
> >  err_exit_port:
> > +	mutex_lock(&dbc_tty_minors_lock);
> >  	idr_remove(&dbc_tty_minors, port->minor);
> > +	mutex_unlock(&dbc_tty_minors_lock);
> >  err_idr:
> >  	xhci_dbc_tty_exit_port(port);
> 
> [Severity: High]
> Does this error path leave concurrent TTY sessions pointing to freed memory?
> 
> Earlier in xhci_dbc_tty_register_device(), the port is added to the globally
> visible dbc_tty_minors IDR before internal structures are fully allocated:
> 
> 	mutex_lock(&dbc_tty_minors_lock);
> 	port->minor = idr_alloc(&dbc_tty_minors, port, 0, 64, GFP_KERNEL);
> 	mutex_unlock(&dbc_tty_minors_lock);
> 
> A concurrent open() on the device node routes to dbc_tty_install(), which
> retrieves the port and installs the TTY without verifying port->registered:

I don't think we need to worry about "concurrent open()" for this as
that's not a real use case that I know of and this _should_ be root only
accessable, right?

Or can normal users open this device node?

thanks,

greg k-h

Re: [PATCH 06/14] xhci: dbc: lock the minor IDR on registration failure

[Greg KH]

Resending with the proper people in cc...

On Fri, Oct 09, 2026 at 12:51:41PM +0200, Greg KH wrote:
> On Fri, Oct 09, 2026 at 10:13:10AM +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] usb: xhci: dbc: UAF due to premature IDR exposure and missing TTY hangup on error path
> > --
> > 
> > commit 99c18a1b8449e8389ccac896ceadcc979dbd6a57
> > Author: Sang-Hoon Choi <csh0052@gmail.com>
> > 
> > xhci: dbc: lock the minor IDR on registration failure
> > 
> > This commit fixes a race condition during DbC TTY registration failure
> > by adding mutex protection around the removal of the minor IDR entry.
> > 
> > > diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
> > > index 3d51e8d82659d..2249cc16800cc 100644
> > > --- a/drivers/usb/host/xhci-dbgtty.c
> > > +++ b/drivers/usb/host/xhci-dbgtty.c
> > > @@ -535,7 +535,9 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc)
> > >  err_free_fifo:
> > >  	kfifo_free(&port->port.xmit_fifo);
> > >  err_exit_port:
> > > +	mutex_lock(&dbc_tty_minors_lock);
> > >  	idr_remove(&dbc_tty_minors, port->minor);
> > > +	mutex_unlock(&dbc_tty_minors_lock);
> > >  err_idr:
> > >  	xhci_dbc_tty_exit_port(port);
> > 
> > [Severity: High]
> > Does this error path leave concurrent TTY sessions pointing to freed memory?
> > 
> > Earlier in xhci_dbc_tty_register_device(), the port is added to the globally
> > visible dbc_tty_minors IDR before internal structures are fully allocated:
> > 
> > 	mutex_lock(&dbc_tty_minors_lock);
> > 	port->minor = idr_alloc(&dbc_tty_minors, port, 0, 64, GFP_KERNEL);
> > 	mutex_unlock(&dbc_tty_minors_lock);
> > 
> > A concurrent open() on the device node routes to dbc_tty_install(), which
> > retrieves the port and installs the TTY without verifying port->registered:
> 
> I don't think we need to worry about "concurrent open()" for this as
> that's not a real use case that I know of and this _should_ be root only
> accessable, right?
> 
> Or can normal users open this device node?
> 
> thanks,
> 
> greg k-h

Re: [PATCH 06/14] xhci: dbc: lock the minor IDR on registration failure

[Mathias Nyman]

On 10/9/26 13:52, Greg KH wrote:
> Resending with the proper people in cc...
> 
> On Fri, Oct 09, 2026 at 12:51:41PM +0200, Greg KH wrote:
>> On Fri, Oct 09, 2026 at 10:13:10AM +0000, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>> - [High] usb: xhci: dbc: UAF due to premature IDR exposure and missing TTY hangup on error path
>>> --
>>>
>>> commit 99c18a1b8449e8389ccac896ceadcc979dbd6a57
>>> Author: Sang-Hoon Choi <csh0052@gmail.com>
>>>
>>> xhci: dbc: lock the minor IDR on registration failure
>>>
>>> This commit fixes a race condition during DbC TTY registration failure
>>> by adding mutex protection around the removal of the minor IDR entry.
>>>
>>>> diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
>>>> index 3d51e8d82659d..2249cc16800cc 100644
>>>> --- a/drivers/usb/host/xhci-dbgtty.c
>>>> +++ b/drivers/usb/host/xhci-dbgtty.c
>>>> @@ -535,7 +535,9 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc)
>>>>   err_free_fifo:
>>>>   	kfifo_free(&port->port.xmit_fifo);
>>>>   err_exit_port:
>>>> +	mutex_lock(&dbc_tty_minors_lock);
>>>>   	idr_remove(&dbc_tty_minors, port->minor);
>>>> +	mutex_unlock(&dbc_tty_minors_lock);
>>>>   err_idr:
>>>>   	xhci_dbc_tty_exit_port(port);
>>>
>>> [Severity: High]
>>> Does this error path leave concurrent TTY sessions pointing to freed memory?
>>>
>>> Earlier in xhci_dbc_tty_register_device(), the port is added to the globally
>>> visible dbc_tty_minors IDR before internal structures are fully allocated:
>>>
>>> 	mutex_lock(&dbc_tty_minors_lock);
>>> 	port->minor = idr_alloc(&dbc_tty_minors, port, 0, 64, GFP_KERNEL);
>>> 	mutex_unlock(&dbc_tty_minors_lock);
>>>
>>> A concurrent open() on the device node routes to dbc_tty_install(), which
>>> retrieves the port and installs the TTY without verifying port->registered:
>>
>> I don't think we need to worry about "concurrent open()" for this as
>> that's not a real use case that I know of and this _should_ be root only
>> accessable, right?
>>
>> Or can normal users open this device node?
>>
No risk of concurrent open as the tty device node is not yet created in this error
path.

This error path is taken if tty_port_register_device() fails, of some other failure
before it

-Mathias

Re: [PATCH 06/14] xhci: dbc: lock the minor IDR on registration failure

[Greg KH]

On Fri, Oct 09, 2026 at 02:16:44PM +0300, Mathias Nyman wrote:
> On 10/9/26 13:52, Greg KH wrote:
> > Resending with the proper people in cc...
> > 
> > On Fri, Oct 09, 2026 at 12:51:41PM +0200, Greg KH wrote:
> > > On Fri, Oct 09, 2026 at 10:13:10AM +0000, sashiko-bot@kernel.org wrote:
> > > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > > - [High] usb: xhci: dbc: UAF due to premature IDR exposure and missing TTY hangup on error path
> > > > --
> > > > 
> > > > commit 99c18a1b8449e8389ccac896ceadcc979dbd6a57
> > > > Author: Sang-Hoon Choi <csh0052@gmail.com>
> > > > 
> > > > xhci: dbc: lock the minor IDR on registration failure
> > > > 
> > > > This commit fixes a race condition during DbC TTY registration failure
> > > > by adding mutex protection around the removal of the minor IDR entry.
> > > > 
> > > > > diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
> > > > > index 3d51e8d82659d..2249cc16800cc 100644
> > > > > --- a/drivers/usb/host/xhci-dbgtty.c
> > > > > +++ b/drivers/usb/host/xhci-dbgtty.c
> > > > > @@ -535,7 +535,9 @@ static int xhci_dbc_tty_register_device(struct xhci_dbc *dbc)
> > > > >   err_free_fifo:
> > > > >   	kfifo_free(&port->port.xmit_fifo);
> > > > >   err_exit_port:
> > > > > +	mutex_lock(&dbc_tty_minors_lock);
> > > > >   	idr_remove(&dbc_tty_minors, port->minor);
> > > > > +	mutex_unlock(&dbc_tty_minors_lock);
> > > > >   err_idr:
> > > > >   	xhci_dbc_tty_exit_port(port);
> > > > 
> > > > [Severity: High]
> > > > Does this error path leave concurrent TTY sessions pointing to freed memory?
> > > > 
> > > > Earlier in xhci_dbc_tty_register_device(), the port is added to the globally
> > > > visible dbc_tty_minors IDR before internal structures are fully allocated:
> > > > 
> > > > 	mutex_lock(&dbc_tty_minors_lock);
> > > > 	port->minor = idr_alloc(&dbc_tty_minors, port, 0, 64, GFP_KERNEL);
> > > > 	mutex_unlock(&dbc_tty_minors_lock);
> > > > 
> > > > A concurrent open() on the device node routes to dbc_tty_install(), which
> > > > retrieves the port and installs the TTY without verifying port->registered:
> > > 
> > > I don't think we need to worry about "concurrent open()" for this as
> > > that's not a real use case that I know of and this _should_ be root only
> > > accessable, right?
> > > 
> > > Or can normal users open this device node?
> > > 
> No risk of concurrent open as the tty device node is not yet created in this error
> path.
> 
> This error path is taken if tty_port_register_device() fails, of some other failure
> before it

Ah, yeah, no need to worry about this "issue" then.

thanks,

greg k-h

[PATCH 07/14] usb: xhci: sideband: fix ring sg table for sub-page TRB segments

[Mathias Nyman]

From: Wesley Cheng <wesley.cheng@oss.qualcomm.com>

xhci_ring_to_sgtable() populated its sg_table via dma_get_sgtable()
per segment and sg_alloc_table_from_pages(), both of which only
operate at whole PAGE_SIZE granularity. Since TRB_SEGMENT_SIZE (4096)
can be smaller than PAGE_SIZE, multiple ring segments can share the
same physical page on larger-PAGE_SIZE kernels (16K/64K), which these
helpers cannot correctly represent.

Build the sg_table directly instead: allocate one sg entry per ring
segment with sg_alloc_table(), and fill each entry explicitly with
sg_set_page() using the segment's own page (resolved via
is_vmalloc_addr()/vmalloc_to_page() or virt_to_page()),
TRB_SEGMENT_SIZE as the length, and offset_in_page() for the exact
intra-page offset. This guarantees each segment gets its own sg
entry regardless of page sharing.

Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-sideband.c | 57 ++++++++++----------------------
 1 file changed, 18 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index a5deeee4d5dc..beb637407e47 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -9,57 +9,42 @@
  */
 
 #include <linux/usb/xhci-sideband.h>
-#include <linux/dma-direct.h>
 
 #include "xhci.h"
 
 /* sideband internal helpers */
 static struct sg_table *
-xhci_ring_to_sgtable(struct xhci_sideband *sb, struct xhci_ring *ring)
+xhci_ring_to_sgtable(struct xhci_ring *ring)
 {
 	struct xhci_segment *seg;
 	struct sg_table	*sgt;
-	unsigned int n_pages;
-	struct page **pages;
-	struct device *dev;
-	size_t sz;
+	struct page *page;
 	int i;
 
-	dev = xhci_to_hcd(sb->xhci)->self.sysdev;
-	sz = ring->num_segs * TRB_SEGMENT_SIZE;
-	n_pages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
-	pages = kvmalloc_objs(struct page *, n_pages);
-	if (!pages)
+	seg = ring->first_seg;
+	if (!seg)
 		return NULL;
 
 	sgt = kzalloc_obj(*sgt);
-	if (!sgt) {
-		kvfree(pages);
+	if (!sgt)
+		return NULL;
+
+	if (sg_alloc_table(sgt, ring->num_segs, GFP_KERNEL)) {
+		kfree(sgt);
 		return NULL;
 	}
 
-	seg = ring->first_seg;
-	if (!seg)
-		goto err;
-	/*
-	 * Rings can potentially have multiple segments, create an array that
-	 * carries page references to allocated segments.  Utilize the
-	 * sg_alloc_table_from_pages() to create the sg table, and to ensure
-	 * that page links are created.
-	 */
 	for (i = 0; i < ring->num_segs; i++) {
-		dma_get_sgtable(dev, sgt, seg->trbs, seg->dma,
-				TRB_SEGMENT_SIZE);
-		pages[i] = sg_page(sgt->sgl);
-		sg_free_table(sgt);
+		if (is_vmalloc_addr(seg->trbs))
+			page = vmalloc_to_page(seg->trbs);
+		else
+			page = virt_to_page(seg->trbs);
+
+		sg_set_page(&sgt->sgl[i], page, TRB_SEGMENT_SIZE,
+			    offset_in_page(seg->trbs));
 		seg = seg->next;
 	}
 
-	if (sg_alloc_table_from_pages(sgt, pages, n_pages, 0, sz, GFP_KERNEL))
-		goto err;
-
-	kvfree(pages);
-
 	/*
 	 * Save first segment dma address to sg dma_address field for the sideband
 	 * client to have access to the IOVA of the ring.
@@ -67,12 +52,6 @@ xhci_ring_to_sgtable(struct xhci_sideband *sb, struct xhci_ring *ring)
 	sg_dma_address(sgt->sgl) = ring->first_seg->dma;
 
 	return sgt;
-
-err:
-	kvfree(pages);
-	kfree(sgt);
-
-	return NULL;
 }
 
 /* Caller must hold sb->mutex */
@@ -254,7 +233,7 @@ xhci_sideband_get_endpoint_buffer(struct xhci_sideband *sb,
 	if (!ep || !ep->ring || !ep->sideband || ep->sideband != sb)
 		return NULL;
 
-	return xhci_ring_to_sgtable(sb, ep->ring);
+	return xhci_ring_to_sgtable(ep->ring);
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_get_endpoint_buffer);
 
@@ -276,7 +255,7 @@ xhci_sideband_get_event_buffer(struct xhci_sideband *sb)
 	if (!sb || !sb->ir)
 		return NULL;
 
-	return xhci_ring_to_sgtable(sb, sb->ir->event_ring);
+	return xhci_ring_to_sgtable(sb->ir->event_ring);
 }
 EXPORT_SYMBOL_GPL(xhci_sideband_get_event_buffer);
 
-- 
2.43.0

Re: [PATCH 07/14] usb: xhci: sideband: fix ring sg table for sub-page TRB segments

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: xhci: OOB array access and scatterlist corruption via sgt->sgl[i] direct indexing
- [High] usb: xhci: DMA API violation and potential panic via virt_to_page() on coherent DMA memory
- [High] usb: xhci: sideband: incomplete fix as qc_audio_offload ignores sg->offset
--

commit 94c9b1944dfef1bf09409dc3dc3f51bb0547b9a2
Author: Wesley Cheng <wesley.cheng@oss.qualcomm.com>

usb: xhci: sideband: fix ring sg table for sub-page TRB segments

This patch modifies xhci_ring_to_sgtable() to directly build an sg_table
instead of relying on dma_get_sgtable() and sg_alloc_table_from_pages(),
which only operate on whole PAGE_SIZE chunks. The aim is to support kernels
with larger page sizes (16K/64K) where multiple TRB segments share a page.

> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index a5deeee4d5dce..beb637407e47e 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
[ ... ]
> @@ -9,57 +9,42 @@
>   */
>  
>  #include <linux/usb/xhci-sideband.h>
> -#include <linux/dma-direct.h>
>  
>  #include "xhci.h"
>  
>  /* sideband internal helpers */
>  static struct sg_table *
> -xhci_ring_to_sgtable(struct xhci_sideband *sb, struct xhci_ring *ring)
> +xhci_ring_to_sgtable(struct xhci_ring *ring)
>  {
[ ... ]
> +	if (sg_alloc_table(sgt, ring->num_segs, GFP_KERNEL)) {
> +		kfree(sgt);
>  		return NULL;
>  	}
>  
>  	for (i = 0; i < ring->num_segs; i++) {
> -		dma_get_sgtable(dev, sgt, seg->trbs, seg->dma,
> -				TRB_SEGMENT_SIZE);
> -		pages[i] = sg_page(sgt->sgl);
> -		sg_free_table(sgt);
> +		if (is_vmalloc_addr(seg->trbs))
> +			page = vmalloc_to_page(seg->trbs);
> +		else
> +			page = virt_to_page(seg->trbs);

[Severity: High]
Is it safe to use virt_to_page() on coherent DMA memory here?

The xHCI ring segments (seg->trbs) are allocated via dma_pool_zalloc(), which
uses dma_alloc_coherent(). The DMA API explicitly warns that memory returned
by dma_alloc_coherent() is not guaranteed to be backed by a struct page.

On architectures where DMA memory resides outside the linear direct-map
(e.g., ARM/MIPS platforms using IOMEM or CMA reserved pools), virt_to_page()
computes an invalid pointer. The previous code safely handled this by
delegating to dma_get_sgtable(), which uses architecture-specific logic.

> +
> +		sg_set_page(&sgt->sgl[i], page, TRB_SEGMENT_SIZE,
                            ^^^^^^^^^^^^

[Severity: High]
Can this array indexing corrupt the scatterlist and cause an out-of-bounds
write?

The function sg_alloc_table() returns a chained scatterlist when the
requested number of entries (ring->num_segs) exceeds SG_MAX_SINGLE_ALLOC
(typically 128).

When i reaches 127 (the chain pointer index), sg_set_page() overwrites the
chain link, severing the rest of the list. On subsequent iterations
(e.g., i = 128), &sgt->sgl[i] writes into unallocated out-of-bounds heap
memory.

Should this iterate over the allocated sg_table using the for_each_sg() macro
or sg_next() instead of flat array indexing?

[Severity: High]
Does this fix leave the issue unresolved for existing callers?

While this patch adds the correct offset to the sg_table for sub-page TRB
segments, downstream consumers currently ignore it.

For example, in sound/usb/qcom/qc_audio_offload.c:uaudio_endpoint_setup():

	pg = sg_page(sgt->sgl);
	tr_pa = page_to_phys(pg);

And similarly in uaudio_event_ring_setup():

	pg = sg_page(sgt->sgl);
	er_pa = page_to_phys(pg);

These calls extract the page via sg_page() and use page_to_phys(), which
completely strips the sg->offset. On systems with PAGE_SIZE > 4096, this maps
the start of the kernel page instead of the actual segment offset, which
could cause the audio offload hardware to read from the wrong physical
location.

Will the consumers need updates to use sg_phys() instead?

> +			    offset_in_page(seg->trbs));
>  		seg = seg->next;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=7

Re: [PATCH 07/14] usb: xhci: sideband: fix ring sg table for sub-page TRB segments

[Mathias Nyman]

On 10/9/26 13:15, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] usb: xhci: OOB array access and scatterlist corruption via sgt->sgl[i] direct indexing
> - [High] usb: xhci: DMA API violation and potential panic via virt_to_page() on coherent DMA memory
> - [High] usb: xhci: sideband: incomplete fix as qc_audio_offload ignores sg->offset
> --
> 
> commit 94c9b1944dfef1bf09409dc3dc3f51bb0547b9a2
> Author: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
> 
> usb: xhci: sideband: fix ring sg table for sub-page TRB segments
> 
> This patch modifies xhci_ring_to_sgtable() to directly build an sg_table
> instead of relying on dma_get_sgtable() and sg_alloc_table_from_pages(),
> which only operate on whole PAGE_SIZE chunks. The aim is to support kernels
> with larger page sizes (16K/64K) where multiple TRB segments share a page.
> 
>> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
>> index a5deeee4d5dce..beb637407e47e 100644
>> --- a/drivers/usb/host/xhci-sideband.c
>> +++ b/drivers/usb/host/xhci-sideband.c
> [ ... ]
>> @@ -9,57 +9,42 @@
>>    */
>>   
>>   #include <linux/usb/xhci-sideband.h>
>> -#include <linux/dma-direct.h>
>>   
>>   #include "xhci.h"
>>   
>>   /* sideband internal helpers */
>>   static struct sg_table *
>> -xhci_ring_to_sgtable(struct xhci_sideband *sb, struct xhci_ring *ring)
>> +xhci_ring_to_sgtable(struct xhci_ring *ring)
>>   {
> [ ... ]
>> +	if (sg_alloc_table(sgt, ring->num_segs, GFP_KERNEL)) {
>> +		kfree(sgt);
>>   		return NULL;
>>   	}
>>   
>>   	for (i = 0; i < ring->num_segs; i++) {
>> -		dma_get_sgtable(dev, sgt, seg->trbs, seg->dma,
>> -				TRB_SEGMENT_SIZE);
>> -		pages[i] = sg_page(sgt->sgl);
>> -		sg_free_table(sgt);
>> +		if (is_vmalloc_addr(seg->trbs))
>> +			page = vmalloc_to_page(seg->trbs);
>> +		else
>> +			page = virt_to_page(seg->trbs);
> 
> [Severity: High]
> Is it safe to use virt_to_page() on coherent DMA memory here?
> 
> The xHCI ring segments (seg->trbs) are allocated via dma_pool_zalloc(), which
> uses dma_alloc_coherent(). The DMA API explicitly warns that memory returned
> by dma_alloc_coherent() is not guaranteed to be backed by a struct page.
> 
> On architectures where DMA memory resides outside the linear direct-map
> (e.g., ARM/MIPS platforms using IOMEM or CMA reserved pools), virt_to_page()
> computes an invalid pointer. The previous code safely handled this by
> delegating to dma_get_sgtable(), which uses architecture-specific logic.
> 
>> +
>> +		sg_set_page(&sgt->sgl[i], page, TRB_SEGMENT_SIZE,
>                              ^^^^^^^^^^^^
> 

Not sure, not familiar with this. Needs to be investigated


> [Severity: High]
> Can this array indexing corrupt the scatterlist and cause an out-of-bounds
> write?
> 
> The function sg_alloc_table() returns a chained scatterlist when the
> requested number of entries (ring->num_segs) exceeds SG_MAX_SINGLE_ALLOC
> (typically 128).
> 
> When i reaches 127 (the chain pointer index), sg_set_page() overwrites the
> chain link, severing the rest of the list. On subsequent iterations
> (e.g., i = 128), &sgt->sgl[i] writes into unallocated out-of-bounds heap
> memory.
> 
> Should this iterate over the allocated sg_table using the for_each_sg() macro
> or sg_next() instead of flat array indexing?

sg_next() could be better, but this isn't a high priority issue as we are very
far from any chained scatter-gather list limit.

xhci transfer rings usually have 2 segments. Can possibly be 4 or 8 in future if
we add ring expansion support to offloaded endpoints.

> 
> [Severity: High]
> Does this fix leave the issue unresolved for existing callers?
> 
> While this patch adds the correct offset to the sg_table for sub-page TRB
> segments, downstream consumers currently ignore it.
> 
> For example, in sound/usb/qcom/qc_audio_offload.c:uaudio_endpoint_setup():
> 
> 	pg = sg_page(sgt->sgl);
> 	tr_pa = page_to_phys(pg);
> 
> And similarly in uaudio_event_ring_setup():
> 
> 	pg = sg_page(sgt->sgl);
> 	er_pa = page_to_phys(pg);
> 
> These calls extract the page via sg_page() and use page_to_phys(), which
> completely strips the sg->offset. On systems with PAGE_SIZE > 4096, this maps
> the start of the kernel page instead of the actual segment offset, which
> could cause the audio offload hardware to read from the wrong physical
> location.
> 

There are other issues with PAGE_SIZE > 4096 than this.

Wesley has a series that addresses these issues together with this client side change

I'll drop this for now.

We can take this patch later together with the rest of the > 4096 page size + custom
dma pool series.

Wesley, any idea about the virt_to_page(seg->trbs) issue?

Thanks
Mathias

[PATCH 08/14] usb: xhci-pci: Add TUSB73x0 definitions

[Mathias Nyman]

From: Fabio Estevam <festevam@gmail.com>

Instead of hard-coding the TUSB73X0 PCI ID, USB_CTRL register address
and the PWRON_POLARITY, introduce definitions for them to make the code
easier to read.

No functional change.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-pci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index a8889081ae82..c580e0b86011 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -28,6 +28,9 @@
 #define SPARSE_DISABLE_BIT	17
 #define SPARSE_CNTL_ENABLE	0xC12C
 
+#define TUSB73X0_USB_CTRL			0xe0
+#define TUSB73X0_PWRON_POLARITY			BIT(22)
+
 /* Device for a quirk */
 #define PCI_VENDOR_ID_FRESCO_LOGIC		0x1b73
 #define PCI_DEVICE_ID_FRESCO_LOGIC_PDK		0x1000
@@ -95,6 +98,8 @@
 #define PCI_DEVICE_ID_ASMEDIA_3042_XHCI			0x3042
 #define PCI_DEVICE_ID_ASMEDIA_3242_XHCI			0x3242
 
+#define PCI_DEVICE_ID_TI_TUSB73X0			0x8241
+
 static const char hcd_name[] = "xhci_hcd";
 
 static struct hc_driver __read_mostly xhci_pci_hc_driver;
@@ -479,7 +484,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	    pdev->device == PCI_DEVICE_ID_ASMEDIA_3042_XHCI)
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
 
-	if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
+	if (pdev->vendor == PCI_VENDOR_ID_TI &&
+	    pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)
 		xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
 
 	if ((pdev->vendor == PCI_VENDOR_ID_BROADCOM ||
@@ -678,7 +684,8 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	dma_set_max_seg_size(&dev->dev, UINT_MAX);
 
 	if (device_property_read_bool(&dev->dev, "ti,pwron-active-high"))
-		pci_clear_and_set_config_dword(dev, 0xE0, 0, 1 << 22);
+		pci_clear_and_set_config_dword(dev, TUSB73X0_USB_CTRL, 0,
+					       TUSB73X0_PWRON_POLARITY);
 
 	return 0;
 
-- 
2.43.0

Re: [PATCH 08/14] usb: xhci-pci: Add TUSB73x0 definitions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb: incomplete refactoring of TUSB73x0 PCI ID magic number
--

commit 876cce743788b879e20245be68d1112387a5be14
Author: Fabio Estevam <festevam@gmail.com>

usb: xhci-pci: Add TUSB73x0 definitions

This patch introduces definitions for the TUSB73X0 PCI ID, USB_CTRL register
address, and PWRON_POLARITY. This replaces the hard-coded values in the
code to improve readability.

> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index a8889081ae824..c580e0b860118 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
[ ... ]
> @@ -95,6 +98,8 @@
>  #define PCI_DEVICE_ID_ASMEDIA_3042_XHCI			0x3042
>  #define PCI_DEVICE_ID_ASMEDIA_3242_XHCI			0x3242
>  
> +#define PCI_DEVICE_ID_TI_TUSB73X0			0x8241

[Severity: Low]
Does this code leave an incomplete refactoring of the magic number 0x8241?

In drivers/usb/host/pci-quirks.c:quirk_usb_handoff_xhci(), the TI TUSB73X0 PCI
device ID is still hard-coded:

drivers/usb/host/pci-quirks.c:quirk_usb_handoff_xhci() {
    ...
    if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
    ...
}

Should PCI_DEVICE_ID_TI_TUSB73X0 be placed in a shared header so it can replace
the hard-coded value in both files?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=8

Re: [PATCH 08/14] usb: xhci-pci: Add TUSB73x0 definitions

[Mathias Nyman]

On 10/9/26 13:07, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] usb: incomplete refactoring of TUSB73x0 PCI ID magic number
> --
> 
> commit 876cce743788b879e20245be68d1112387a5be14
> Author: Fabio Estevam <festevam@gmail.com>
> 
> usb: xhci-pci: Add TUSB73x0 definitions
> 
> This patch introduces definitions for the TUSB73X0 PCI ID, USB_CTRL register
> address, and PWRON_POLARITY. This replaces the hard-coded values in the
> code to improve readability.
> 
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index a8889081ae824..c580e0b860118 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
> [ ... ]
>> @@ -95,6 +98,8 @@
>>   #define PCI_DEVICE_ID_ASMEDIA_3042_XHCI			0x3042
>>   #define PCI_DEVICE_ID_ASMEDIA_3242_XHCI			0x3242
>>   
>> +#define PCI_DEVICE_ID_TI_TUSB73X0			0x8241
> 
> [Severity: Low]
> Does this code leave an incomplete refactoring of the magic number 0x8241?
> 
> In drivers/usb/host/pci-quirks.c:quirk_usb_handoff_xhci(), the TI TUSB73X0 PCI
> device ID is still hard-coded:
> 
> drivers/usb/host/pci-quirks.c:quirk_usb_handoff_xhci() {
>      ...
>      if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
>      ...
> }
> 
> Should PCI_DEVICE_ID_TI_TUSB73X0 be placed in a shared header so it can replace
> the hard-coded value in both files?
> 

Sure, dropping this patch for now. It only added some definitions

Thanks
Mathias

[PATCH 09/14] usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

In this case we know that the xHC has released ownership of all missed
TDs, we only don't know which were missed and which were queued later.

URBs are queued atomically, so we can safely give back all TDs of the
currently executing URB. Unlike the previous policy, this does actually
ensure that the class driver will learn about the error and won't see
all of its URBs still in progress when all TDs are missed on xHCI 1.0.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-ring.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7f480db2983e..8b915a1d5b25 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2649,6 +2649,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	unsigned int slot_id;
 	int ep_index;
 	struct xhci_td *td = NULL;
+	struct urb *missed_urb = NULL;
 	dma_addr_t ep_trb_dma;
 	union xhci_trb *ep_trb;
 	int status = -EINPROGRESS;
@@ -2868,26 +2869,31 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 					return 0;
 
 				/*
-				 * TD was missed, skip it. Core already initialized frame->status
-				 * to -EXDEV and frame->actual_length to 0, nothing more to do.
+				 * If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB
+				 * pointer is zero again. All missed TDs can be given back, but we
+				 * don't know which were missed and which were queued after the xrun
+				 * occurred. We can safely give back the first pending URB.
 				 */
-				xhci_dequeue_td(xhci, td, ep_ring, 0);
+				if (ring_xrun_event) {
+					if (!missed_urb)
+						missed_urb = td->urb;
 
-				if (!list_empty(&ep_ring->td_list)) {
-					if (ring_xrun_event) {
-						/*
-						 * If we are here, we are on xHCI 1.0 host with no
-						 * idea how many TDs were missed or where the xrun
-						 * occurred. New TDs may have been added after the
-						 * xrun, so skip only one TD to be safe.
-						 */
-						xhci_dbg(xhci, "Skipped one TD for slot %u ep %u",
+					if (td->urb != missed_urb) {
+						xhci_dbg(xhci, "Skipped one URB for slot %u ep %u",
 								slot_id, ep_index);
 						return 0;
 					}
-					continue;
 				}
 
+				/*
+				 * TD was missed, skip it. Core already initialized frame->status
+				 * to -EXDEV and frame->actual_length to 0, nothing more to do.
+				 */
+				xhci_dequeue_td(xhci, td, ep_ring, 0);
+
+				if (!list_empty(&ep_ring->td_list))
+					continue;
+
 				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
 					 slot_id, ep_index);
 				ep->skip = false;
-- 
2.43.0

Re: [PATCH 09/14] usb: xhci: Guarantee URB giveback on Ring Underrun/Overrun

[sashiko-bot]

> From: Michal Pecio <michal.pecio@gmail.com>
> 
> In this case we know that the xHC has released ownership of all missed
> TDs, we only don't know which were missed and which were queued later.
> 
> URBs are queued atomically, so we can safely give back all TDs of the
> currently executing URB. Unlike the previous policy, this does actually
> ensure that the class driver will learn about the error and won't see
> all of its URBs still in progress when all TDs are missed on xHCI 1.0.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=9

[PATCH 10/14] usb: xhci: Don't set the skip flag on non-isoc endpoints

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

These events are unique to isochronous endpoints, ignore them otherwise.
Update debug messages to reflect new policies. We could also log invalid
events as errors, but it seems nobody has ever had problems with that,
so don't bother.

This allows dropping the isoc check when skipping TDs.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-ring.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 8b915a1d5b25..2dd11732bb87 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2778,16 +2778,18 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		 * Set skip flag of the ep_ring; Complete the missed tds as
 		 * short transfer when process the ep_ring next time.
 		 */
-		ep->skip = true;
+		if (ep_ring->type == TYPE_ISOC)
+			ep->skip = true;
 		xhci_dbg(xhci,
-			 "Miss service interval error for slot %u ep %u, set skip flag%s\n",
-			 slot_id, ep_index, ep_trb_dma ? ", skip now" : "");
+			 "Missed Service Error for slot %u ep %u, skip %d, try now %d\n",
+			 slot_id, ep_index, ep->skip, !!ep_trb_dma);
 		break;
 	case COMP_NO_PING_RESPONSE_ERROR:
-		ep->skip = true;
+		if (ep_ring->type == TYPE_ISOC)
+			ep->skip = true;
 		xhci_dbg(xhci,
-			 "No Ping response error for slot %u ep %u, Skip one Isoc TD\n",
-			 slot_id, ep_index);
+			 "No Ping response error for slot %u ep %u, skip %d\n",
+			 slot_id, ep_index, ep->skip);
 		return 0;
 
 	case COMP_INCOMPATIBLE_DEVICE_ERROR:
@@ -2863,7 +2865,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		/* Is this TRB not part of the currently executing TD? */
 		if (!trb_in_td(td, ep_trb_dma)) {
 
-			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
+			if (ep->skip) {
 				/* this event is unlikely to match any TD, don't skip them all */
 				if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
 					return 0;
-- 
2.43.0

Re: [PATCH 10/14] usb: xhci: Don't set the skip flag on non-isoc endpoints

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: xhci: invalid completion of URB with -EINPROGRESS on COMP_MISSED_SERVICE_ERROR
--

commit dfab6709cb637a348bd004519bdfe39df717cdc4
Author: Michal Pecio <michal.pecio@gmail.com>

usb: xhci: Don't set the skip flag on non-isoc endpoints

These events are unique to isochronous endpoints, ignore them otherwise.
Update debug messages to reflect new policies. We could also log invalid
events as errors, but it seems nobody has ever had problems with that,
so don't bother.

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 8b915a1d5b256..2dd11732bb87c 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2778,16 +2778,18 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>  		 * Set skip flag of the ep_ring; Complete the missed tds as
>  		 * short transfer when process the ep_ring next time.
>  		 */
> -		ep->skip = true;
> +		if (ep_ring->type == TYPE_ISOC)
> +			ep->skip = true;
>  		xhci_dbg(xhci,
> -			 "Miss service interval error for slot %u ep %u, set skip flag%s\n",
> -			 slot_id, ep_index, ep_trb_dma ? ", skip now" : "");
> +			 "Missed Service Error for slot %u ep %u, skip %d, try now %d\n",
> +			 slot_id, ep_index, ep->skip, !!ep_trb_dma);
>  		break;

[Severity: High]
In handle_tx_event() in drivers/usb/host/xhci-ring.c, for non-isochronous
endpoints, does using break instead of return 0 here cause the event to
improperly fall through into the rest of the handler?

If the event's DMA matches the current TD, it appears it is passed to
process_bulk_intr_td() or process_ctrl_td(). Since those functions do not
handle this completion code, does the URB's status remain initialized to
-EINPROGRESS, completing the URB with an illegal status code?

Additionally, if the event DMA does not match the current TD, does the code
fall back to the debug_finding_td error path, fatally shutting down the host
controller with -ESHUTDOWN?

>  	case COMP_NO_PING_RESPONSE_ERROR:
> -		ep->skip = true;
> +		if (ep_ring->type == TYPE_ISOC)
> +			ep->skip = true;
>  		xhci_dbg(xhci,
> -			 "No Ping response error for slot %u ep %u, Skip one Isoc TD\n",
> -			 slot_id, ep_index);
> +			 "No Ping response error for slot %u ep %u, skip %d\n",
> +			 slot_id, ep_index, ep->skip);
>  		return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=10

Re: [PATCH 10/14] usb: xhci: Don't set the skip flag on non-isoc endpoints

[Mathias Nyman]

On 10/9/26 13:16, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] usb: xhci: invalid completion of URB with -EINPROGRESS on COMP_MISSED_SERVICE_ERROR
> --
> 
> commit dfab6709cb637a348bd004519bdfe39df717cdc4
> Author: Michal Pecio <michal.pecio@gmail.com>
> 
> usb: xhci: Don't set the skip flag on non-isoc endpoints
> 
> These events are unique to isochronous endpoints, ignore them otherwise.
> Update debug messages to reflect new policies. We could also log invalid
> events as errors, but it seems nobody has ever had problems with that,
> so don't bother.
> 
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 8b915a1d5b256..2dd11732bb87c 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -2778,16 +2778,18 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>>   		 * Set skip flag of the ep_ring; Complete the missed tds as
>>   		 * short transfer when process the ep_ring next time.
>>   		 */
>> -		ep->skip = true;
>> +		if (ep_ring->type == TYPE_ISOC)
>> +			ep->skip = true;
>>   		xhci_dbg(xhci,
>> -			 "Miss service interval error for slot %u ep %u, set skip flag%s\n",
>> -			 slot_id, ep_index, ep_trb_dma ? ", skip now" : "");
>> +			 "Missed Service Error for slot %u ep %u, skip %d, try now %d\n",
>> +			 slot_id, ep_index, ep->skip, !!ep_trb_dma);
>>   		break;
> 
> [Severity: High]
> In handle_tx_event() in drivers/usb/host/xhci-ring.c, for non-isochronous
> endpoints, does using break instead of return 0 here cause the event to
> improperly fall through into the rest of the handler?
> 
> If the event's DMA matches the current TD, it appears it is passed to
> process_bulk_intr_td() or process_ctrl_td(). Since those functions do not
> handle this completion code, does the URB's status remain initialized to
> -EINPROGRESS, completing the URB with an illegal status code?

Not an issue,

MISSED_SERVICE_ERROR is completion code only applies to isoch endpoints

see xhci spec section 4.10.3.2 Missed Service Error
"This error only applies to Isochronous endpoints."

> 
> Additionally, if the event DMA does not match the current TD, does the code
> fall back to the debug_finding_td error path, fatally shutting down the host
> controller with -ESHUTDOWN?

Not an issue

This completion code is exclusive to isoch endpoints, meaning td->skip
flag will always be set on this error, thus never taking the debug_finding_td
error path.

Keeping this patch, will send as part of v2


-Mathias

[PATCH 11/14] usb: xhci: Shorten the TD skipping loop

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

Half of this loop is code which only executes once to deal with cases
where no TD matches the event and then it returns. This code needs not
to be in any kind of loop, so get it out.

Optimize conditionals remaining in the loop body.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-ring.c | 68 +++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 2dd11732bb87..7597ef8105c6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2862,10 +2862,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
 				      td_list);
 
-		/* Is this TRB not part of the currently executing TD? */
-		if (!trb_in_td(td, ep_trb_dma)) {
+		if (ep->skip) {
 
-			if (ep->skip) {
+			if (!trb_in_td(td, ep_trb_dma)) {
 				/* this event is unlikely to match any TD, don't skip them all */
 				if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
 					return 0;
@@ -2903,38 +2902,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 				goto check_endpoint_halted;
 			}
 
-			/* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
-			if (ring_xrun_event)
-				return 0;
-
-			/*
-			 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
-			 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
-			 * pointer still at the previous TRB of the current TD. The previous TRB
-			 * maybe a Link TD or the last TRB of the previous TD. The command
-			 * completion handle will take care the rest.
-			 */
-			if (trb_comp_code == COMP_STOPPED ||
-			    trb_comp_code == COMP_STOPPED_LENGTH_INVALID) {
-				return 0;
-			}
-
-			/*
-			 * Some hosts give a spurious success event after a short
-			 * transfer or error on last TRB. Ignore it.
-			 */
-			if (xhci_spurious_success_tx_event(xhci, ep_ring)) {
-				xhci_dbg(xhci, "Spurious event dma %pad, comp_code %u after %u\n",
-					 &ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code);
-				ep_ring->old_trb_comp_code = 0;
-				return 0;
-			}
-
-			/* HC is busted, give up! */
-			goto debug_finding_td;
-		}
-
-		if (ep->skip) {
 			xhci_dbg(xhci,
 				 "Found td. Clear skip flag for slot %u ep %u.\n",
 				 slot_id, ep_index);
@@ -2949,6 +2916,37 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	 */
 	} while (ep->skip);
 
+	/* Handle events not referencing the current TD */
+	if (!trb_in_td(td, ep_trb_dma)) {
+		/* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
+		if (ring_xrun_event)
+			return 0;
+
+		/*
+		 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
+		 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
+		 * pointer still at the previous TRB of the current TD. The previous TRB
+		 * maybe a Link TD or the last TRB of the previous TD. The command
+		 * completion handle will take care the rest.
+		 */
+		if (trb_comp_code == COMP_STOPPED || trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
+			return 0;
+
+		/*
+		 * Some hosts give a spurious success event after a short
+		 * transfer or error on last TRB. Ignore it.
+		 */
+		if (xhci_spurious_success_tx_event(xhci, ep_ring)) {
+			xhci_dbg(xhci, "Spurious event dma %pad, comp_code %u after %u\n",
+					&ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code);
+			ep_ring->old_trb_comp_code = 0;
+			return 0;
+		}
+
+		/* HC is busted, give up! */
+		goto debug_finding_td;
+	}
+
 	ep_ring->old_trb_comp_code = trb_comp_code;
 
 	/* Get out if a TD was queued at enqueue after the xrun occurred */
-- 
2.43.0

Re: [PATCH 11/14] usb: xhci: Shorten the TD skipping loop

[sashiko-bot]

> From: Michal Pecio <michal.pecio@gmail.com>
> 
> Half of this loop is code which only executes once to deal with cases
> where no TD matches the event and then it returns. This code needs not
> to be in any kind of loop, so get it out.
> 
> Optimize conditionals remaining in the loop body.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=11

[PATCH 12/14] usb: xhci: Rework and improve the TD matching and skipping logic

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

Matching events with TDs and giving back missed TDs is carried out
by a complicated loop. Replace it with a simpler linear logic:

0. Having verified that 'td_list' isn't empty,
1. Scan it to find the matching TD and count missed TDs,
2. Perform necessary adjustments for corner cases,
3. Give back missed TDs, if applicable, using a short and tidy loop,
4. Check if the event refers to the expected TD and proceed as usual.

Besides cleaning up the code, this provides a few improvements:
- when the skip flag is set, no TD is given back unless we found a match
  or otherwise know how many TDs should be given back
- when the skip flag is clear, we know if the event refers to a "future"
  TD so we can log this in the Scary Error Message to aid debugging.

While altering the error message, drop a pointless goto.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>
---
 drivers/usb/host/xhci-ring.c | 137 ++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 76 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7597ef8105c6..243b1fd2b2f6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -127,11 +127,16 @@ static bool link_trb_toggles_cycle(union xhci_trb *trb)
 	return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
 }
 
-static bool last_td_in_urb(struct xhci_td *td)
+static int num_tds_not_done(struct urb *urb)
 {
-	struct urb_priv *urb_priv = td->urb->hcpriv;
+	struct urb_priv *urb_priv = urb->hcpriv;
 
-	return urb_priv->num_tds_done == urb_priv->num_tds;
+	return urb_priv->num_tds - urb_priv->num_tds_done;
+}
+
+static bool last_td_in_urb(struct xhci_td *td)
+{
+	return !num_tds_not_done(td->urb);
 }
 
 static bool unhandled_event_trb(struct xhci_ring *ring)
@@ -2624,14 +2629,19 @@ static bool xhci_spurious_success_tx_event(struct xhci_hcd *xhci,
 	}
 }
 
-static struct xhci_td *find_td_by_dma(struct xhci_ring *ep_ring, dma_addr_t dma)
+static struct xhci_td *find_td_by_dma(struct xhci_ring *ep_ring, int *missed_tds, dma_addr_t dma)
 {
 	struct xhci_td *td;
 
-	if (dma)
+	if (dma) {
 		list_for_each_entry(td, &ep_ring->td_list, td_list)
 			if (trb_in_td(td, dma))
 				return td;
+			else
+				(*missed_tds)++;
+	}
+
+	*missed_tds = 0;
 	return NULL;
 }
 
@@ -2648,8 +2658,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	struct xhci_ring *ep_ring;
 	unsigned int slot_id;
 	int ep_index;
-	struct xhci_td *td = NULL;
-	struct urb *missed_urb = NULL;
+	struct xhci_td *td;
+	int missed_tds = 0;
 	dma_addr_t ep_trb_dma;
 	union xhci_trb *ep_trb;
 	int status = -EINPROGRESS;
@@ -2832,13 +2842,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		xhci_dequeue_td(xhci, td, ep_ring, td->status);
 	}
 
-	/*
-	 * We don't know how many TDs were missed when ep_trb_dma is zero (as permitted by
-	 * xHCI 1.0) or bogus. Bail out leaving ep->skip set, next event will sort it out.
-	 */
-	if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !find_td_by_dma(ep_ring, ep_trb_dma))
-		return 0;
-
 	if (list_empty(&ep_ring->td_list)) {
 		/*
 		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
@@ -2858,66 +2861,50 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		goto check_endpoint_halted;
 	}
 
-	do {
-		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
-				      td_list);
-
-		if (ep->skip) {
-
-			if (!trb_in_td(td, ep_trb_dma)) {
-				/* this event is unlikely to match any TD, don't skip them all */
-				if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID)
-					return 0;
-
-				/*
-				 * If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB
-				 * pointer is zero again. All missed TDs can be given back, but we
-				 * don't know which were missed and which were queued after the xrun
-				 * occurred. We can safely give back the first pending URB.
-				 */
-				if (ring_xrun_event) {
-					if (!missed_urb)
-						missed_urb = td->urb;
-
-					if (td->urb != missed_urb) {
-						xhci_dbg(xhci, "Skipped one URB for slot %u ep %u",
-								slot_id, ep_index);
-						return 0;
-					}
-				}
-
-				/*
-				 * TD was missed, skip it. Core already initialized frame->status
-				 * to -EXDEV and frame->actual_length to 0, nothing more to do.
-				 */
-				xhci_dequeue_td(xhci, td, ep_ring, 0);
+	td = find_td_by_dma(ep_ring, &missed_tds, ep_trb_dma);
 
-				if (!list_empty(&ep_ring->td_list))
-					continue;
+	if (ep->skip) {
+		if (!td) {
+			/*
+			 * xHCI 1.0 allowed MSE events to have zero TRB pointers. Some old chips
+			 * also generate bogus non-zero pointers. We know, don't bother warning.
+			 * Missed TDs will be given back by the next event with a valid pointer.
+			 */
+			if (trb_comp_code == COMP_MISSED_SERVICE_ERROR &&
+			    xhci->hci_version <= 0x100)
+				return 0;
+			/*
+			 * If skip flag is still set at xrun, we are on xHCI 1.0 and our TRB pointer
+			 * is zero again. All missed TDs can be given back, but we don't know which
+			 * were missed and which were queued after the xrun occurred. We can safely
+			 * give back the first pending URB to let the class driver know.
+			 */
+			if (ring_xrun_event)
+				missed_tds = num_tds_not_done(list_first_entry(&ep_ring->td_list,
+								struct xhci_td, td_list)->urb);
+			/* In other cases missed_tds is zero */
+		}
 
-				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
-					 slot_id, ep_index);
-				ep->skip = false;
-				td = NULL;
-				goto check_endpoint_halted;
-			}
+		/*
+		 * Give back missed TDs. Core already initialized their frame->status to -EXDEV
+		 * and frame->actual_length to 0, nothing more to do.
+		 */
+		for (int i = 0; i < missed_tds; i++)
+			xhci_dequeue_td(xhci,
+				list_first_entry(&ep_ring->td_list, struct xhci_td, td_list),
+				ep_ring, 0);
 
-			xhci_dbg(xhci,
-				 "Found td. Clear skip flag for slot %u ep %u.\n",
-				 slot_id, ep_index);
+		/* the list may become empty on ring_xrun_event */
+		if (td || list_empty(&ep_ring->td_list))
 			ep->skip = false;
-		}
 
-	/*
-	 * If ep->skip is set, it means there are missed tds on the
-	 * endpoint ring need to take care of.
-	 * Process them as short transfer until reach the td pointed by
-	 * the event.
-	 */
-	} while (ep->skip);
+		xhci_dbg(xhci, "Skipped %d TDs on slot %u ep %u comp_code %u, TD found %d, skip flag %d\n",
+				missed_tds, slot_id, ep_index, trb_comp_code, !!td, ep->skip);
+		missed_tds = 0;
+	}
 
 	/* Handle events not referencing the current TD */
-	if (!trb_in_td(td, ep_trb_dma)) {
+	if (!td || missed_tds) {
 		/* TD was queued after xrun, maybe xrun was on a link, don't panic yet */
 		if (ring_xrun_event)
 			return 0;
@@ -2944,7 +2931,13 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		}
 
 		/* HC is busted, give up! */
-		goto debug_finding_td;
+		td = list_first_entry(&ep_ring->td_list, struct xhci_td, td_list);
+		xhci_err(xhci, "Event dma %pad for ep %d comp_code %u not part of TD at %016llx - %016llx, missed %d\n",
+				&ep_trb_dma, ep_index, trb_comp_code,
+				(u64)xhci_trb_virt_to_dma(td->start_seg, td->start_trb),
+				(u64)xhci_trb_virt_to_dma(td->end_seg, td->end_trb),
+				missed_tds);
+		return -ESHUTDOWN;
 	}
 
 	ep_ring->old_trb_comp_code = trb_comp_code;
@@ -2982,14 +2975,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 	return 0;
 
-debug_finding_td:
-	xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %016llx - %016llx\n",
-		 &ep_trb_dma, ep_index, trb_comp_code,
-		 (unsigned long long)xhci_trb_virt_to_dma(td->start_seg, td->start_trb),
-		 (unsigned long long)xhci_trb_virt_to_dma(td->end_seg, td->end_trb));
-
-	return -ESHUTDOWN;
-
 err_out:
 	xhci_err(xhci, "@%016llx %08x %08x %08x %08x\n",
 		 (unsigned long long) xhci_trb_virt_to_dma(
-- 
2.43.0

Re: [PATCH 12/14] usb: xhci: Rework and improve the TD matching and skipping logic

[sashiko-bot]

> From: Michal Pecio <michal.pecio@gmail.com>
> 
> Matching events with TDs and giving back missed TDs is carried out
> by a complicated loop. Replace it with a simpler linear logic:
> 
> 0. Having verified that 'td_list' isn't empty,
> 1. Scan it to find the matching TD and count missed TDs,
> 2. Perform necessary adjustments for corner cases,
> 3. Give back missed TDs, if applicable, using a short and tidy loop,
> 4. Check if the event refers to the expected TD and proceed as usual.
> 
> Besides cleaning up the code, this provides a few improvements:
> - when the skip flag is set, no TD is given back unless we found a match
>   or otherwise know how many TDs should be given back
> - when the skip flag is clear, we know if the event refers to a "future"
>   TD so we can log this in the Scary Error Message to aid debugging.
> 
> While altering the error message, drop a pointless goto.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=12

[PATCH 13/14] usb: xhci: Fix bounce buffer overflow

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

High-speed devices with out of spec 1024 byte bulk endpoints exist and
are allowed by USB core, but xhci-hcd always sets packet size to 512.
The exact nature of these devices isn't documented, commit fb5ee84ea72c
("USB: Accept bulk endpoints with 1024-byte maxpacket") only states
that they "don't work with xHCI host controllers", whatever it means.

But somebody (or a malicious device) can try, and then the driver will
allocate a 512 byte bounce buffer for this endpoint and may write up to
1024 bytes into it if particular scatter-gather URBs are used, because
xhci_align_td() obtains packet size from the descriptor. Fix this.

As a side effect, TRBs will be aligned to the packet size chosen by the
driver on all endpoints of all speeds. Alignment serves the xHC, not
device, so this is fine. Only out of spec devices are affected anyway.

Reported-by: co+fd80bc5967eb22c3@bugs.sh
Link: https://lore.kernel.org/linux-usb/D4tcSGerkYkIV1DmaUo1t8TaR5qQElDLkidn@bugs.sh/
Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
Cc: stable@vger.kernel.org
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 243b1fd2b2f6..c23434001e9c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3546,15 +3546,13 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int transferred,
 
 
 static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 enqd_len,
-			 u32 *trb_buff_len, struct xhci_segment *seg)
+			 u32 *trb_buff_len, struct xhci_segment *seg, u32 max_pkt)
 {
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
 	unsigned int unalign;
-	unsigned int max_pkt;
 	u32 new_buff_len;
 	size_t len;
 
-	max_pkt = xhci_usb_endpoint_maxp(urb->dev, urb->ep);
 	unalign = (enqd_len + *trb_buff_len) % max_pkt;
 
 	/* we got lucky, last normal TRB data on segment is packet aligned */
@@ -3699,9 +3697,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		if (enqd_len + trb_buff_len < full_len) {
 			field |= TRB_CHAIN;
 			if (trb_is_link(ring->enqueue + 1)) {
-				if (xhci_align_td(xhci, urb, enqd_len,
-						  &trb_buff_len,
-						  ring->enq_seg)) {
+				if (xhci_align_td(xhci, urb, enqd_len, &trb_buff_len,
+						ring->enq_seg, ring->bounce_buf_len)) {
 					send_addr = ring->enq_seg->bounce_dma;
 					/* TD bounced at least, and last on this seg */
 					td->bounce_seg = ring->enq_seg;
-- 
2.43.0

Re: [PATCH 13/14] usb: xhci: Fix bounce buffer overflow

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: xhci: inconsistent TD Size calculation for out-of-spec bulk devices
--

commit f7283e43a6da5355da08e764e6aa96a4a118927d
Author: Michal Pecio <michal.pecio@gmail.com>
Subject: usb: xhci: Fix bounce buffer overflow

High-speed devices with out of spec 1024 byte bulk endpoints exist and
are allowed by USB core, but xhci-hcd always sets packet size to 512.
This commit fixes a bounce buffer overflow for these devices by allocating
a 512 byte bounce buffer and using the correctly clamped packet size
in xhci_align_td().

Link: https://lore.kernel.org/linux-usb/D4tcSGerkYkIV1DmaUo1t8TaR5qQElDLkidn@bugs.sh/

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 243b1fd2b2f68..c23434001e9cb 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
[ ... ]
> @@ -3699,9 +3697,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  		if (enqd_len + trb_buff_len < full_len) {
>  			field |= TRB_CHAIN;
>  			if (trb_is_link(ring->enqueue + 1)) {
> -				if (xhci_align_td(xhci, urb, enqd_len,
> -						  &trb_buff_len,
> -						  ring->enq_seg)) {
> +				if (xhci_align_td(xhci, urb, enqd_len, &trb_buff_len,
> +						ring->enq_seg, ring->bounce_buf_len)) {

[Severity: Medium]
While xhci_align_td() is correctly updated here to use the clamped
ring->bounce_buf_len, does the xhci_td_remainder() call further down in
xhci_queue_bulk_tx() also need to be updated?

xhci_td_remainder() still relies on the raw 1024-byte max packet size from the
descriptor:

drivers/usb/host/xhci-ring.c:xhci_td_remainder() {
	...
	maxp = xhci_usb_endpoint_maxp(urb->dev, urb->ep);
	total_packet_count = DIV_ROUND_UP(td_total_len, maxp);

	/* Queueing functions don't count the current TRB into transferred */
	return (total_packet_count - ((transferred + trb_buff_len) / maxp));
}

Since the endpoint context and bounce buffer use a clamped 512-byte size, could
this mismatch cause the driver to write an incorrectly small TD Size into the
TRB? If the xHC hardware is using 512 bytes, might this mismatch lead to a
Transfer Ring Expansion Error, babble, or endpoint halt when queueing a bulk
transfer greater than 1024 bytes for a high-speed device with an out-of-spec
1024-byte max packet size?

>  					send_addr = ring->enq_seg->bounce_dma;
>  					/* TD bounced at least, and last on this seg */
>  					td->bounce_seg = ring->enq_seg;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=13

[PATCH 14/14] xhci: Prevent invalid vdev dereference during sideband unregister

[Mathias Nyman]

Offloaded USB audio devices using the xhci-sideband API store a pointer to
the xhci virtual device (vdev) in the sideband structure when registering.

This pointer typically remains valid throughout the lifetime of the USB
device. If a configured offloaded device requires a reset, the USB core
usually unbinds or notifies the audio driver beforehand, ensuring that the
sideband is unregistered before the vdev is freed.

An exception occurs when the USB core resets a device to recover from a
failed resume, but a subsequent 'address device' request also fails. To
recover in this specific scenario, the xHCI driver disables and re-enables
the slot, which frees and re-allocates the vdev.

xhci_sideband_unregister() later dereferences the stale, previously freed
vdev pointer during disconnect, triggering a kernel oops:

 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
  usb_disable_device+0x84/0x190
  usb_disconnect+0xe8/0x338
  hub_event+0xbd8/0x19ac

Fix this by preventing the reallocation of the vdev in this specific error
path if the device is registered for sideband use.

Just propagate the error directly to the USB core. It should either retry
enumeration or detect the disconnected device, and handle it accordingly.

Debugging this issue to the dangling vdev pointer, reporting it, testing,
and initial patch with different solution by Lianqin Hu

Reported-by: Lianqin Hu <hulianqin@vivo.com>
Closes: https://lore.kernel.org/linux-usb/TYUPR06MB6217000B59003EDF233D7246D2B22@TYUPR06MB6217.apcprd06.prod.outlook.com/
Tested-by: Lianqin Hu <hulianqin@vivo.com>
Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9564dde8bb34..30ac1bc4559b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4435,10 +4435,13 @@ 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_and_free_slot(xhci, udev->slot_id);
-		if (!ret) {
-			if (xhci_alloc_dev(hcd, udev) == 1)
-				xhci_setup_addressable_virt_dev(xhci, udev);
+
+		if (!virt_dev->sideband) {
+			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);
+			}
 		}
 		kfree(command->completion);
 		kfree(command);
-- 
2.43.0

Re: [PATCH 14/14] xhci: Prevent invalid vdev dereference during sideband unregister

[sashiko-bot]

> Offloaded USB audio devices using the xhci-sideband API store a pointer to
> the xhci virtual device (vdev) in the sideband structure when registering.
> 
> This pointer typically remains valid throughout the lifetime of the USB
> device. If a configured offloaded device requires a reset, the USB core
> usually unbinds or notifies the audio driver beforehand, ensuring that the
> sideband is unregistered before the vdev is freed.
> 
> An exception occurs when the USB core resets a device to recover from a
> failed resume, but a subsequent 'address device' request also fails. To
> recover in this specific scenario, the xHCI driver disables and re-enables
> the slot, which frees and re-allocates the vdev.
> 
> xhci_sideband_unregister() later dereferences the stale, previously freed
> vdev pointer during disconnect, triggering a kernel oops:
> [ ... ]
> Closes: https://lore.kernel.org/linux-usb/TYUPR06MB6217000B59003EDF233D7246D2B22@TYUPR06MB6217.apcprd06.prod.outlook.com/
> Tested-by: Lianqin Hu <hulianqin@vivo.com>
> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009095834.561578-1-mathias.nyman@linux.intel.com?part=14

Re: [PATCH 00/14] xhci features and fixes for usb-next

[Greg KH]

On Fri, Oct 09, 2026 at 12:58:20PM +0300, Mathias Nyman wrote:
> Hi Greg
> 
> xhci improvements and non-urgent fixes for usb-next.
> 
> One series by Michal to improve transfer event handling, otherwise
> smaller scattered patches for dbc, early dbc, sideband and generic
> xhci cleanups.

You you want me to take all the ones here that were "clean" from the
sashiko review, and ignore the ones that were not, or do you want me to
take them all now and those issues (if they are real, at least one I
noticed is not), will be worried about later?

thanks,

greg k-h

Re: [PATCH 00/14] xhci features and fixes for usb-next

[Mathias Nyman]

On 10/9/26 13:50, Greg KH wrote:
> On Fri, Oct 09, 2026 at 12:58:20PM +0300, Mathias Nyman wrote:
>> Hi Greg
>>
>> xhci improvements and non-urgent fixes for usb-next.
>>
>> One series by Michal to improve transfer event handling, otherwise
>> smaller scattered patches for dbc, early dbc, sideband and generic
>> xhci cleanups.
> 
> You you want me to take all the ones here that were "clean" from the
> sashiko review, and ignore the ones that were not, or do you want me to
> take them all now and those issues (if they are real, at least one I
> noticed is not), will be worried about later?
> 

Let me take a closer look, I'll send a new series today.

Some could be dropped, cleanup up, and submitted later, like [PATCH 1/14]

Others like [PATCH 10/14] is mid series, and don't want to blindly drop it
without checking if sashiko issue is valid

Thanks
Mathias

Re: [PATCH 00/14] xhci features and fixes for usb-next

[Michal Pecio]

On Fri, 9 Oct 2026 14:00:21 +0300, Mathias Nyman wrote:
> On 10/9/26 13:50, Greg KH wrote:
> > On Fri, Oct 09, 2026 at 12:58:20PM +0300, Mathias Nyman wrote:  
> >> Hi Greg
> >>
> >> xhci improvements and non-urgent fixes for usb-next.
> >>
> >> One series by Michal to improve transfer event handling, otherwise
> >> smaller scattered patches for dbc, early dbc, sideband and generic
> >> xhci cleanups.  
> > 
> > You you want me to take all the ones here that were "clean" from the
> > sashiko review, and ignore the ones that were not, or do you want me to
> > take them all now and those issues (if they are real, at least one I
> > noticed is not), will be worried about later?
> >   
> 
> Let me take a closer look, I'll send a new series today.
> 
> Some could be dropped, cleanup up, and submitted later, like [PATCH 1/14]
> 
> Others like [PATCH 10/14] is mid series, and don't want to blindly drop it
> without checking if sashiko issue is valid

Oh cool, more AI slop on linux-usb.

I don't like wasting time on that, but I understand this bot has just
been deployed and it's two weeks to merge window, so...

 [PATCH 13/14] usb: xhci: Fix bounce buffer overflow

Yes, -EPOROTO or -EOVERFLOW is roughly what one can expect when the
host and device are set up for different packet size. The patch only
tries to ensure that we don't also get memory corruption then.

We could actually try to support 1024 byte bulk high-speed endpoints
(provided that the HC doesn't reject or mishandle them), I tested this
on FX2 with appropriate FW and mentioned such possibility to Mathias,
but in absence of genuine user demand he showed no enthusiasm.

 [PATCH 10/14] usb: xhci: Don't set the skip flag on non-isoc endpoints

This is a pre-existing (non-)issue actually introduced by bfa845994282
last year. I see Mathias has just responded that this is limited to
out-of-spec host controllers which, may I add, aren't known to exist,
though other forms of bogus or mangled transfer events are known.

The code, again, only tries to prevent completely crazy things like
giving back multiple pending URBs with zero reason to believe that the
hardware will not attempt to DMA them in a moment.

Regards,
Michal