Re: [PATCH v3 03/10] ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy

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

Pre-existing issues:
- [High] Missing payload length validation for PCIe AER error records (`CPER_SEC_PCIE`).
- [High] The exact same out-of-bounds read vulnerability exists in `cxl_cper_print_prot_err()` when printing the CPER record.
- [High] Potential deadlock due to missing IRQ masking when acquiring `cxl_cper_work_lock` and `cxl_cper_prot_err_work_lock` in process context.
- [High] The `#ifdef ACPI_APEI_PCIEAER` guard in `drivers/acpi/acpi_extlog.c` is missing the `CONFIG_` prefix.
--

--- Patch [3]: [PATCH v3 03/10] ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy ---
commit d9ae66d42856680b3ac8a462d2465e800e436efc
Author: Dave Jiang <[email protected]>

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

sashiko-bot flagged an out-of-bounds read driven by an unvalidated
firmware dvsec_len.

cxl_cper_setup_prot_err_work_data() locates the RAS Capability block at
prot_err + sizeof(*prot_err) + dvsec_len and copies it, but dvsec_len is
firmware controlled and never validated.

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/[email protected]/

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 7ad3b36013cc6..06a944dadbc15 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -165,12 +165,12 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>  
>  static void
>  extlog_cxl_cper_handle_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> -				int severity)
> +				int severity, u32 len)
>  {
>  #ifdef ACPI_APEI_PCIEAER

[Severity: High]
This isn't a bug introduced by this patch, but does this #ifdef evaluate to
false because it is missing the CONFIG_ prefix?

If so, does that cause the entire CXL protocol error handling and PCIe AER
logging in extlog_print_pcie() to be compiled as dead code?

>  	struct cxl_cper_prot_err_work_data wd;
>  
> -	if (cxl_cper_sec_prot_err_valid(prot_err))
> +	if (cxl_cper_sec_prot_err_valid(prot_err, len))
>  		return;

[ ... ]

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index a752e152a5a0b..17e4ef5552925 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -753,12 +753,12 @@ static DEFINE_SPINLOCK(cxl_cper_prot_err_work_lock);
>  struct work_struct *cxl_cper_prot_err_work;
>  
>  static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> -				   int severity)
> +				   int severity, u32 len)
>  {
>  #ifdef CONFIG_ACPI_APEI_PCIEAER
>  	struct cxl_cper_prot_err_work_data wd;
>  
> -	if (cxl_cper_sec_prot_err_valid(prot_err))
> +	if (cxl_cper_sec_prot_err_valid(prot_err, len))
>  		return;
>  
>  	guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);

[Severity: High]
This is a pre-existing issue, but while looking at how this lock is used,
I noticed a potential IRQ inversion deadlock.

In drivers/acpi/apei/ghes.c:cxl_cper_register_prot_err_work() and
cxl_cper_register_work(), the locks are acquired from process context using
guard(spinlock), which does not disable interrupts:

    guard(spinlock)(&cxl_cper_prot_err_work_lock);

If a hardware error occurs on the same CPU, the NMI/IRQ handler will call
cxl_cper_post_prot_err() which attempts to acquire the exact same spinlock
using guard(spinlock_irqsave):

    guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);

Could this cause a deadlock during initialization?

[ ... ]

> @@ -950,7 +950,8 @@ static void ghes_do_proc(struct ghes *ghes,
>  		} else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) {
>  			struct cxl_cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata);
>  
> -			cxl_cper_post_prot_err(prot_err, gdata->error_severity);
> +			cxl_cper_post_prot_err(prot_err, gdata->error_severity,
> +					       gdata->error_data_length);

[Severity: High]
This is a pre-existing issue, but I noticed similar validation might be
missing for PCIe AER error records.

When ghes_do_proc() calls ghes_handle_aer() for CPER_SEC_PCIE, it casts
gdata to struct cper_sec_pcie * and reads pcie_err->validation_bits without
first verifying that gdata->error_data_length is at least
sizeof(struct cper_sec_pcie).

Could malicious or buggy firmware supplying an undersized error data length
cause an out-of-bounds read here?

[ ... ]

> diff --git a/drivers/acpi/apei/ghes_helpers.c b/drivers/acpi/apei/ghes_helpers.c
> index bc7111b740afb..d625ec98a24c7 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)
>  {

[Severity: High]
This is a pre-existing issue, but I noticed another location that might need
similar bounds checking.

In drivers/firmware/efi/cper_cxl.c:cxl_cper_print_prot_err(), the function
uses the unvalidated prot_err->dvsec_len to calculate the offset to the RAS
capability registers:

    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);

Since the function isn't passed the section length to perform this check,
could a malformed payload cause print_hex_dump() and subsequent register
accesses to read out of bounds?

> +	if (len < sizeof(*prot_err)) {
> +		pr_err_ratelimited(FW_WARN
> +				   "CXL CPER prot err section too small (%u)\n",
> +				   len);
> +		return -EINVAL;
> +	}
> +

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