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

Hi

On 7/20/26 22:12, Umang Jain wrote:
> Currently, the early xhci-dbc assumes that the extended capability
> can be mapped within the fixed boot time mappings dictated by
> NR_FIX_BTMAPS.
> 
> This patch iterates over the PCI BAR address size to find and map
> xhci-xdbc capability which could be out-of-bounds otherwise,
> in xdbc_map_pci_mmio(). The iterations map the maximum allowed
> boot time mappings (fixmap size) at a time and search for xhci-xdbc
> capability offset, till the end of the bar address size.
> 
> 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 <[email protected]>
> ---
> Link to earlier RFC:
> https://lore.kernel.org/all/[email protected]/
> ---
>   drivers/usb/early/xhci-dbc.c | 94 +++++++++++++++++++++++++++++++++---
>   drivers/usb/early/xhci-dbc.h |  1 +
>   2 files changed, 87 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
> index 41118bba9197..5e540b6bf962 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 */
> +#define XDBC_MAPPING_SIZE	56

I know spec says 56 bytes, but when looking at the Debug capability structure
in xhci section 7.6.8. it looks like 64 bytes.

Addresses 3F-00H from the dbc ext cap offset

I'd change this to 64 just to be sure

> +
> +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, mapped_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,72 @@ 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 mappings,
> +	 * hence iterate over the region one fixmap_size at a time,
> +	 * starting with XHCI_EXP_CAPS_DEBUG capability.
> +	 */
> +	base = early_ioremap(val64, fixmap_size);
> +	offset = xhci_find_next_ext_cap(base, 0, 0);
> +	mapped_size = fixmap_size;
> +
> +	while (mapped_size <= sz64) {

Do you know if it ever had to go past first fixmap_size (first iteration)
to find the XHCI_EXT_CAPS_DEBUG and the other needed capabilities?

If not then we could maybe skip the feature to iterate over entire xdbc.xhci_length.
Just check for the extended capabilities in first fixmap_size, and fail if not found there.

> +		val = readl(base + offset);

We should make sure offset < fixmap_size before reading 'base + offset'

> +		switch (XHCI_EXT_CAPS_ID(val)) {
> +		case XHCI_EXT_CAPS_DEBUG:
> +			if (offset + XDBC_MAPPING_SIZE > fixmap_size) {
> +				early_iounmap(base, fixmap_size);
> +				base = early_ioremap(val64 + offset, XDBC_MAPPING_SIZE);
> +
> +				mapped_size += offset;
> +				cap_flags = XDBC_CAP_FLAG_NONE;
> +			}
> +			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;
> +		}
> +
> +		/*
> +		 * Find offset to next xhci-ext capability, remap if the offset
> +		 * is out of bounds of the already mapped region.
> +		 */
> +		offset = xhci_find_next_ext_cap(base, offset, 0);
> +		if (!offset) {

xhci_find_next_ext_cap() only returns 0 if there are no extended capabilities left.
(in cases where ID is set then it returns 0 if no extended capabilities with that ID are left)

I think we need to do something like:

if (!offset) {
	break; /* exit, failed case */
} else if (offset > fixmap_size) {
	iounmap()
	ioremap(val64 + mapped_size, fixmap_size)
	...
	OR just break/exit/fail if we decide to just check first fixmap_size
}

Thanks
Mathias