Re: [PATCH v6 09/13] ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-Bounds Read in `cxl_cper_print_prot_err` when dumping firmware-controlled length
- [High] Mismatched validation check in `cxl_cper_sec_prot_err_valid` drops valid errors and accepts uninitialized agent types
- [High] Invalid pointer comparison in `match_memdev_by_parent` entirely breaks CPER log association for memdevs
--

commit 370881a728ea6e57fe2c8f02a8283fec779e22bb
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy

This commit extends cxl_cper_sec_prot_err_valid() to check the section length
before copying the RAS Capability block. It ensures the header, DVSEC, and RAS
Capability block fit within the reported length to prevent out-of-bounds access.

Link: https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=6
Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/

> diff --git a/drivers/acpi/apei/ghes_helpers.c b/drivers/acpi/apei/ghes_helpers.c
> index bc7111b740afb..df41b993f4130 100644
> --- a/drivers/acpi/apei/ghes_helpers.c
> +++ b/drivers/acpi/apei/ghes_helpers.c
> @@ -5,8 +5,15 @@
>  #include <linux/aer.h>
>  #include <cxl/event.h>
>  
> -int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err)
> +int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err, u32 len)
>  {
> +	if (len < sizeof(*prot_err)) {
> +		pr_err_ratelimited(FW_WARN
> +				   "CXL CPER prot err section too small (%u)\n",
> +				   len);
> +		return -EINVAL;
> +	}
> +
>  	if (!(prot_err->valid_bits & PROT_ERR_VALID_AGENT_ADDRESS)) {
>  		pr_err_ratelimited("CXL CPER invalid agent type\n");
>  		return -EINVAL;
>  	}

[Severity: High]
This is a pre-existing issue, but does this check use the wrong validation bit?

It checks for PROT_ERR_VALID_AGENT_ADDRESS, but the error message states
"invalid agent type", which suggests it should be checking for
PROT_ERR_VALID_AGENT_TYPE.

If the firmware provides a CPER record lacking PROT_ERR_VALID_AGENT_ADDRESS,
valid hardware errors might be silently dropped. If it lacks
PROT_ERR_VALID_AGENT_TYPE, could uninitialized garbage be processed as the
agent type?


While reviewing this patch, I also noticed a pre-existing issue in related
printing code:

drivers/firmware/efi/cper_cxl.c:cxl_cper_print_prot_err() {
    ...
		print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, (prot_err + 1),
			       prot_err->dvsec_len, 0);
    ...
		size_t size = sizeof(*prot_err) + prot_err->dvsec_len;
		struct cxl_ras_capability_regs *cxl_ras;
    ...
		cxl_ras = (struct cxl_ras_capability_regs *)((long)prot_err + size);
		pr_info("%s cxl_ras_uncor_status: 0x%08x", pfx,
			cxl_ras->uncor_status);
}

[Severity: High]
This is a pre-existing issue, but could this cause an out-of-bounds read?

The exact same vulnerability fixed by this patch in
cxl_cper_setup_prot_err_work_data() seems to exist here. The callers in
cper.c do not pass the bounding length to cxl_cper_print_prot_err(), so it uses
prot_err->dvsec_len unvalidated for print_hex_dump() and to calculate the
pointer to cxl_ras.

If firmware provides an artificially large dvsec_len, could this read
out-of-bounds kernel memory into dmesg or cause a panic?


I also noticed another pre-existing issue in the error handling path:

drivers/cxl/core/ras.c:match_memdev_by_parent() {
	if (is_cxl_memdev(dev) && dev->parent == uport)
		return 1;
}

... called from:

drivers/cxl/core/ras.c:cxl_cper_handle_prot_err() {
    ...
	struct device *mem_dev __free(put_device) = bus_find_device(
		&cxl_bus_type, NULL, pdev, match_memdev_by_parent);
    ...
}

[Severity: High]
This is a pre-existing issue, but will this search always fail?

In cxl_cper_handle_prot_err(), pdev (a struct pci_dev *) is passed as the match
data (uport). The match_memdev_by_parent() function compares dev->parent (a
struct device *) directly to uport (a struct pci_dev *).

Since struct device is embedded within struct pci_dev at a non-zero offset,
dev->parent will never equal pdev. Could this entirely break CPER log
association for memdevs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904172337.1409775-1-dave.jiang@intel.com?part=9