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

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

Pre-existing issues:
- [High] The function blindly trusts the hardware-provided `cap_count` without validating it against the mapped register block size, leading to out-of-bounds MMIO reads.
- [High] Hardware-provided capability `offset` and `length` are not validated against the mapped register block bounds, allowing arbitrary physical memory mapping.
--

--- Patch [2]: [PATCH v2 2/2] cxl/core: Skip non-CXL capabilities in MCAP ---
commit f423958d8928ff9878ca7f5a9766da9989945b54
Author: Penn <[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. This prevents non-CXL entries from being
misinterpreted as CXL register blocks when MMPT is enabled.

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

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to trust the
hardware-provided cap_count without validating it against the mapped register
block size?

A malicious or faulty CXL device could present a large cap_count (up to
65535) but advertise a small PCI BAR. Since cxl_probe_device_regs() iterates
up to cap_count without bounding the loop by the mapped size, the capability
header reads could fall outside the ioremapped memory region and cause a
kernel oops/panic due to a page fault.

>  		struct cxl_reg_map *rmap;
> +		u16 cap_id, vendor_id;
>  		u32 offset, length;
> -		u16 cap_id;
> +
> +		vendor_id = FIELD_GET(PCI_MCAP_HDR_VENDOR_ID,
> +				      readl(base + PCI_MCAP_HDR_BASE(cap) +
> +					    PCI_MCAP_HDR_REG_4));
> +		/*
> +		 * The Vendor ID field is reserved and reads as zero in legacy
> +		 * CXL capability headers. See CXL r3.2, Table 8-44.
> +		 */
> +		if (vendor_id && vendor_id != PCI_VENDOR_ID_CXL)
> +			continue;
>  
>  		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 does this code properly validate the
untrusted, hardware-provided offset and length?

In cxl_map_device_regs(), these values are used to compute 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);
...
}

Because they are never checked against the CXL register block's actual size
(map->max_size), a device providing arbitrarily large offsets could cause
the kernel to map unrequested physical address space, potentially leading to
unintended MMIO interactions and system instability.

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