Re: [PATCH v2 4/7] ACPI: extlog: Validate PCIe error section length before payload access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Other APEI/GHES CPER payload handlers lack error_data_length validation before accessing payload structs.
- [High] Hardware PCIe AER capability layout is blindly copied into a software structure, causing severe field misalignment and OOB reads.
--

--- Patch [4]: [PATCH v2 4/7] ACPI: extlog: Validate PCIe error section length before payload access ---
commit e17a8b52722d296c5c160626ff1eb6bdf1232531
Author: Dave Jiang <dave.jiang@intel.com>

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

extlog_print_pcie() reads pcie_err->validation_bits and device_id and
copies the fixed 96-byte pcie_err->aer_info buffer without first checking
that gdata->error_data_length is large enough to hold a struct
cper_sec_pcie.

> 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
[ ... ]
> @@ -145,6 +145,9 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>  	int aer_severity;
>  	int domain;
>  
> +	if (len < sizeof(*pcie_err))
> +		return;
> +
>  	if (!(pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
>  	      pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO))
>  		return;

[Severity: High]
This is a pre-existing issue, but does this code correctly handle the
hardware PCIe AER capability layout?

When copying the hardware aer_info (a 96-byte raw CPER array) into the
software struct aer_capability_regs in extlog_print_pcie(), it appears the
software structure expands header_log to 60 bytes to support 14 DWORDs and
prefix arrays, while the hardware TLP log is only 16 bytes.

drivers/acpi/acpi_extlog.c:extlog_print_pcie() {
    ...
    memcpy(&aer_regs, pcie_err->aer_info, sizeof(pcie_err->aer_info));
    ...
}

Does this cause hardware fields starting from root_command to overlap with
header_log.dw[4], losing the root status information and misinterpreting
them as TLP prefixes?

This same misalignment appears to occur in other places too:

drivers/acpi/apei/ghes.c:ghes_handle_aer() {
    ...
    memcpy(aer_info, pcie_err->aer_info, sizeof(struct aer_capability_regs));
    ...
}

In ghes_handle_aer(), does copying sizeof(struct aer_capability_regs)
(100 bytes) from a 96-byte array cause a 4-byte out-of-bounds read?

And also blindly casted in CPER:

drivers/firmware/efi/cper.c:cper_print_pcie() {
    ...
    aer = (struct aer_capability_regs *)pcie->aer_info;
    ...
}

[ ... ]
> @@ -247,7 +250,8 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>  		} else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
>  			struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
>  
> -			extlog_print_pcie(pcie_err, gdata->error_severity);
> +			extlog_print_pcie(pcie_err, gdata->error_severity,
> +					  gdata->error_data_length);

[Severity: High]
This isn't a bug introduced by this patch, but do other APEI/GHES CPER
payload handlers also lack error_data_length validation before accessing
payload structs?

If firmware provides a CPER payload with an artificially small
error_data_length, could we experience out-of-bounds reads in
ghes_handle_memory_failure()?

drivers/acpi/apei/ghes.c:ghes_handle_memory_failure() {
    ...
    struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);

    if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
    ...
}

Does this risk operating on garbage physical addresses or adjacent
uninitialized padding?

Similarly, in ghes_handle_aer() without a length check:

drivers/acpi/apei/ghes.c:ghes_handle_aer() {
    ...
    struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);

    if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID)
    ...
}

Would adding length validation similar to this patch be needed for those
other handlers as well?

>  		} else {
>  			void *err = acpi_hest_get_payload(gdata);
>

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