[PATCH v2 0/2] PCI/CXL: Distinguish CXL capabilities in MCAP

[PATCH v2 0/2] PCI/CXL: Distinguish CXL capabilities in MCAP

[penn]

From: Penn <engguopeng@buaa.edu.cn>

When MMPT is enabled, a device may expose PCI-SIG-defined MMPT
capabilities alongside CXL-defined capabilities in the same MMIO
Capabilities Register Block (MCAP).

MCAP capability IDs are scoped by Vendor ID, but the CXL capability
parser currently matches entries using only their capability IDs. This
can cause MMPT entries to be interpreted as CXL register blocks.

Add the generic PCI MCAP register definitions, then update the CXL
parser to reject capabilities carrying a non-zero Vendor ID other than
PCI_VENDOR_ID_CXL, while preserving compatibility with legacy CXL
capability headers.

Patch 1 adds the PCI MCAP array and capability header definitions.
Patch 2 filters non-CXL capabilities before interpreting their IDs
while continuing to accept legacy capability headers whose Vendor ID
field reads as zero.

The fix has been tested on CXL 1.1 and CXL 3.0 devices. On the
MMPT-enabled device, it prevents both Mailbox and Status capabilities
from being misidentified.

Changes in v2:
- Update the MCAP specification citation to PCIe r7.0 sec 6.35.
- Accept a zero Vendor ID for compatibility with legacy CXL devices.
- Move the u16 declaration above the u32 declaration.
- Document testing on CXL 1.1 and CXL 3.0 devices.

Penn (2):
  PCI: Add MMIO Capabilities Register Block definitions
  cxl/core: Skip non-CXL capabilities in MCAP

 drivers/cxl/core/regs.c       | 12 +++++++++++-
 include/uapi/linux/pci_regs.h | 22 ++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v2 1/2] PCI: Add MMIO Capabilities Register Block definitions

[penn]

From: Penn <engguopeng@buaa.edu.cn>

PCIe r7.0 sec 6.35 defines the MMIO Capabilities Register Block
(MCAP). It consists of an array header followed by capability headers
that identify the version, location, size, and vendor of each MMIO
capability.

Define the MCAP array-header and capability-header registers and their
fields. These definitions allow subsystem drivers to use the Vendor ID
along with the Capability ID when capabilities from multiple vendors
share the same MCAP.

Signed-off-by: Penn <engguopeng@buaa.edu.cn>
---
Changes in v2:
- Update the MCAP specification citation to PCIe r7.0 sec 6.35.
- Document that capability header indices are one-based.

 include/uapi/linux/pci_regs.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 14f634ab9350..4dd22cde9e71 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1338,6 +1338,28 @@
 #define  PCI_IDE_SEL_ADDR_3(x)		(28 + (x) * PCI_IDE_SEL_ADDR_BLOCK_SIZE)
 #define PCI_IDE_SEL_BLOCK_SIZE(nr_assoc)  (20 + PCI_IDE_SEL_ADDR_BLOCK_SIZE * (nr_assoc))
 
+/* PCIe r7.0, sec 6.35: MMIO Capabilities Register Block (MCAP) */
+#define PCI_MCAP_ARRAY_1		0x00
+#define  PCI_MCAP_ARRAY_ID		__GENMASK(15, 0)
+#define  PCI_MCAP_ARRAY_VERSION		__GENMASK(23, 16)
+#define  PCI_MCAP_ARRAY_TYPE		__GENMASK(27, 24)
+#define  PCI_MCAP_ARRAY_TYPE_CLASS_CODE	0x0
+#define PCI_MCAP_ARRAY_2		0x04
+#define  PCI_MCAP_ARRAY_COUNT		__GENMASK(15, 0)
+
+/* Capability header indices are one-based. */
+#define PCI_MCAP_HDR_SIZEOF		0x10
+#define PCI_MCAP_HDR_BASE(n)		((n) * PCI_MCAP_HDR_SIZEOF)
+#define PCI_MCAP_HDR_REG_1		0x00
+#define  PCI_MCAP_CAP_ID		__GENMASK(15, 0)
+#define  PCI_MCAP_CAP_VERSION		__GENMASK(23, 16)
+#define PCI_MCAP_HDR_REG_2		0x04
+#define  PCI_MCAP_HDR_OFFSET		__GENMASK(31, 0)
+#define PCI_MCAP_HDR_REG_3		0x08
+#define  PCI_MCAP_HDR_LENGTH		__GENMASK(31, 0)
+#define PCI_MCAP_HDR_REG_4		0x0c
+#define  PCI_MCAP_HDR_VENDOR_ID		__GENMASK(15, 0)
+
 /*
  * Compute Express Link (CXL r4.0, sec 8.1)
  *
-- 
2.43.0

Re: [PATCH v2 1/2] PCI: Add MMIO Capabilities Register Block definitions

[sashiko-bot]

> From: Penn <engguopeng@buaa.edu.cn>
> 
> PCIe r7.0 sec 6.35 defines the MMIO Capabilities Register Block
> (MCAP). It consists of an array header followed by capability headers
> that identify the version, location, size, and vendor of each MMIO
> capability.
> 
> Define the MCAP array-header and capability-header registers and their
> fields. These definitions allow subsystem drivers to use the Vendor ID
> along with the Capability ID when capabilities from multiple vendors
> share the same MCAP.
> 
> Signed-off-by: Penn <engguopeng@buaa.edu.cn>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817075854.17207-1-engguopeng@buaa.edu.cn?part=1

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

[penn]

From: Penn <engguopeng@buaa.edu.cn>

When MMPT is enabled, a CXL device may expose CXL-defined capabilities
and PCIe Management Message Passthrough (MMPT) capabilities in the same
MMIO Capabilities Register Block (MCAP).

The CXL capability parser currently identifies entries using only the
capability ID. Since capability IDs are scoped by Vendor ID, a
PCI-SIG-defined capability may have the same ID as a CXL-defined
capability. This causes non-CXL entries, including MMPT and MMIO Mailbox,
to be interpreted as CXL register blocks.

The MMPT register block may be interpreted as a CXL mailbox. This
causes mailbox initialization to fail with:

    cxl_pci 0000:3b:00.0: Mailbox is too small (64b)

Check the MCAP Vendor ID before interpreting an entry as a CXL-defined
capability. Skip capabilities with a non-zero Vendor ID other than
PCI_VENDOR_ID_CXL.

In legacy CXL capability headers, the field now used for the MCAP
Vendor ID was reserved and reads as zero. Continue to accept zero to
preserve compatibility with those devices.

The fix has been tested on CXL 1.1 and CXL 3.0 devices. On the
MMPT-enabled device, it prevents both Mailbox and Status capabilities
from being misidentified.

Fixes: 8adaf747c9f0 ("cxl/mem: Find device capabilities")
Suggested-by: Johnny <johnny.li@montage-tech.com>
Signed-off-by: Penn <engguopeng@buaa.edu.cn>
---
Changes in v2:
- Accept a zero Vendor ID for compatibility with legacy CXL capability
  headers.
- Move the u16 declaration above the u32 declaration to follow the
  reverse Christmas tree convention.
- Document testing on CXL 1.1 and CXL 3.0 devices.

 drivers/cxl/core/regs.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 93710cf4f0a6..c7c14089f2c6 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -133,8 +133,18 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 
 	for (cap = 1; cap <= cap_count; cap++) {
 		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));
-- 
2.43.0

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

[sashiko-bot]

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 <engguopeng@buaa.edu.cn>

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/20260817075854.17207-1-engguopeng@buaa.edu.cn?part=2