Re: [PATCH] cxl/regs: Reject register blocks in an unassigned BAR

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing validation of CXL register locator BIR and resource type allows mapping non-memory or bridge window resources, leading to memory corruption.
- [High] Insufficient bounds validation allows out-of-bounds MMIO reads during CXL capabilities probing, leading to kernel panics.
--

commit 1001d63b6bc0f0959235c2291ef3e197cc4ca650
Author: Junjie Cao <junjie.cao@intel.com>

cxl/regs: Reject register blocks in an unassigned BAR

cxl_decode_regblock() only checks that the Register Locator offset fits
pci_resource_len(). A BAR the PCI core could not place is reset to zero
start, end and flags while config space keeps the firmware value, so a
zero offset passes and the block is mapped at physical address 0.

Link: https://github.com/tianocore/edk2/issues/13104

> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 20c2d9fbcfe7d..41416fa2ce4a4 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -277,6 +277,20 @@ static bool cxl_decode_regblock(struct pci_dev *pdev, u32 reg_lo, u32 reg_hi,
>  	u64 offset = ((u64)reg_hi << 32) |
>  		     (reg_lo & PCI_DVSEC_CXL_REG_LOCATOR_BLOCK_OFF_LOW);
>  
> +	if (reg_type == CXL_REGLOC_RBI_EMPTY)
> +		return false;
> +
> +	/*
> +	 * A BAR the PCI core could not place is reset to zero; decoding it
> +	 * would map the block at physical address 0.
> +	 */
> +	if (!pci_resource_len(pdev, bar) ||
> +	    (pci_resource_flags(pdev, bar) & IORESOURCE_UNSET)) {

[Severity: High]
This isn't a bug introduced by this patch, but since bar is extracted from the
untrusted CXL DVSEC register locator before this check, is there anything
verifying that it falls within the standard endpoint BAR range (0-5)?

Additionally, could a malicious device bypass this validation by configuring a
standard BAR as an assigned I/O BAR (IORESOURCE_IO)? This code does not
verify if pci_resource_flags(pdev, bar) & IORESOURCE_MEM is true. 

If an I/O port address bypasses this check and is passed to ioremap(), could
it map low physical RAM on x86 and corrupt memory during register setup?

> +		dev_warn(&pdev->dev, "BAR%d: not assigned (type: %d)\n", bar,
> +			 reg_type);
> +		return false;
> +	}
> +
>  	if (offset > pci_resource_len(pdev, bar)) {

[Severity: High]
This is a pre-existing issue, but this bounds check allows offset to be
arbitrarily close or equal to the resource length without ensuring enough
space remains for the capabilities array.

If this computes a very small max_size (such as 4 bytes), wouldn't the driver
subsequently perform out-of-bounds MMIO reads during capabilities probing:

drivers/cxl/core/regs.c:cxl_probe_device_regs() {
    ...
    for (cap = 1; cap <= cap_count; cap++) {
        cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
                           readl(base + cap * 0x10));
    ...
}

and read past the end of the mapped pages, causing a kernel panic?

>  		dev_warn(&pdev->dev,
>  			 "BAR%d: %pr: too small (offset: %pa, type: %d)\n", bar,
>  			 &pdev->resource[bar], &offset, reg_type);
>  		return false;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910093858.535969-1-junjie.cao@intel.com?part=1