Re: [PATCH 2/2] cxl/core: Skip non-CXL capabilities in MCAP

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The strict Vendor ID check in `cxl_probe_device_regs` incorrectly rejects valid capabilities on legacy CXL 2.0/3.0 devices.

Pre-existing issues:
- [High] Missing bounds checking on untrusted device-provided capability count can lead to out-of-bounds MMIO reads.
- [High] Unvalidated device-provided capability offsets allow arbitrary physical memory mapping.
--

--- Patch [2]: [PATCH 2/2] cxl/core: Skip non-CXL capabilities in MCAP ---
commit 6cdac110087d8d1e0117e2bb4680d99d50fc28fe
Author: pguo <[email protected]>

cxl/core: Skip non-CXL capabilities in MCAP

This commit adds a check for the MCAP Vendor ID before interpreting an entry
as a CXL-defined capability. It intends to prevent non-CXL entries, such as
MMPT and MMIO Mailbox, from being misidentified as CXL register blocks.

> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 20c2d9fbcfe7d..854a8bb4c4f35 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -135,7 +135,13 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  	for (cap = 1; cap <= cap_count; cap++) {

[Severity: High]
This is a pre-existing issue, but does this loop lack bounds checking against
the mapped MMIO region size?

The cap_count is extracted from the device-provided capability array register
and can be as large as 65535. 

Since base points to an ioremap region bounded by the remaining PCIe BAR size,
a large untrusted cap_count could cause the offset calculations within the
loop to exceed the mapped boundaries, leading to out-of-bounds MMIO reads.

>  		struct cxl_reg_map *rmap;
>  		u32 offset, length;
> -		u16 cap_id;
> +		u16 cap_id, vendor_id;
> +
> +		vendor_id = FIELD_GET(PCI_MCAP_HDR_VENDOR_ID,
> +				      readl(base + PCI_MCAP_HDR_BASE(cap) +
> +					    PCI_MCAP_HDR_REG_4));
> +		if (vendor_id != PCI_VENDOR_ID_CXL)
> +			continue;

[Severity: High]
Does this strict check break legacy CXL 2.0 and 3.0 devices?

For pre-MCAP legacy devices, the offset for the Vendor ID in the capability
header is reserved and defined to read as zero. 

Because this check unconditionally rejects any capability that doesn't
explicitly match PCI_VENDOR_ID_CXL, it will skip all valid capabilities
whose Vendor ID evaluates to zero.

Will this cause device probing to fail entirely for legacy hardware?

>  
>  		cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
>  				   readl(base + cap * 0x10));
>  		offset = readl(base + cap * 0x10 + 0x4);
>  		length = readl(base + cap * 0x10 + 0x8);

[Severity: High]
This is a pre-existing issue, but are the offset and length values validated
against the PCIe BAR boundaries?

These fields are read directly from untrusted device MMIO. Later, in
cxl_map_device_regs(), they are used to calculate the physical address for
mapping:

cxl_map_device_regs()
    addr = phys_addr + mi->rmap->offset;
    length = mi->rmap->size;
    *(mi->addr) = devm_cxl_iomap_block(host, addr, length);

Could a crafted offset and length allow the device to force the host kernel
to map and interact with arbitrary physical memory outside the device's BAR?

>  
>  		rmap = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2