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

[Mathias Nyman <mathias.nyman@linux.intel.com>]

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