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

[sashiko-bot@kernel.org]

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