Re: [PATCH v3 05/10] ACPI: extlog: Validate PCIe error section length before payload access

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

Pre-existing issues:
- [High] The `extlog_print()` function fails to validate the firmware-provided `estatus->data_length` before iterating over its sections, leading to a heap out-of-bounds read.
- [High] Missing `CONFIG_` prefix in the preprocessor guard `#ifdef ACPI_APEI_PCIEAER` renders the error parsing functions and the newly added bounds check dead code.
- [High] The patch fixes an out-of-bounds read vulnerability for PCIe CPER sections in acpi_extlog.c, but leaves identical out-of-bounds reads unpatched in ghes.c.
--

--- Patch [5]: [PATCH v3 05/10] ACPI: extlog: Validate PCIe error section length before payload access ---
commit aa3cbf45758b1ffcfa8d2f4818d6c1ce97837a2a
Author: Dave Jiang <[email protected]>

ACPI: extlog: Validate PCIe error section length before payload access

This commit adds a missing section-length check before extlog_print_pcie()
touches the PCIe payload to prevent accessing stale adjacent bytes if the
section is too small.

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index fbc88c584c067..0c440d75d9a78 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -134,7 +134,7 @@ static int print_extlog_rcd(const char *pfx,
>  }
>  
>  static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
> -			      int severity)
> +			      int severity, u32 len)
>  {
>  #ifdef ACPI_APEI_PCIEAER

[Severity: High]
This is a pre-existing issue, but does this preprocessor guard lack the
CONFIG_ prefix?

If this should be CONFIG_ACPI_APEI_PCIEAER, this guard evaluates to false,
and both the newly added bounds check and the existing PCIe error parsing
logic will be stripped out as dead code. The CXL protocol error handler
in this file might also have a similar misspelled guard.

>  	struct aer_capability_regs aer_regs = {};
>  	struct aer_capability_regs *aer;
[ ... ]
>  	int aer_severity;
>  	int domain;
>  
> +	if (len < sizeof(*pcie_err))
> +		return;
> +

[Severity: High]
This is a pre-existing issue, but while this patch adds bounds checking
for PCIe CPER sections in acpi_extlog.c, do identical unpatched
out-of-bounds reads exist in drivers/acpi/apei/ghes.c?

Looking at ghes_handle_aer() and ghes_handle_memory_failure() in ghes.c,
they appear to unconditionally access validation_bits without verifying
if gdata->error_data_length is large enough for the struct:

drivers/acpi/apei/ghes.c:ghes_handle_aer() {
    ...
    if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID && ...
    ...
}

Could a short section payload from malicious or corrupted firmware cause
an out-of-bounds read when these GHES handlers are dispatched?

>  	if (!(pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
>  	      pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO))
>  		return;

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