Re: [PATCH v2 3/7] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

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

Pre-existing issues:
- [High] GHES AER handling suffers from the exact same unvalidated hardware data bug being fixed, plus a 4-byte out-of-bounds buffer read.
- [High] cxl_rch_get_aer_info() reads raw MMIO past the hardware capability boundary and populates software struct fields directly, triggering an out-of-bounds stack read.
- [High] Missing bounds check on gdata->error_data_length before processing CPER_SEC_CXL_PROT_ERR and CPER_SEC_PCIE payloads.
- [Medium] The patch maps the hardware AER capability buffer to struct aer_capability_regs via memcpy, but structural misalignment causes hardware registers to be logged erroneously as TLP Prefixes.
--

--- Patch [3]: [PATCH v2 3/7] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer ---
commit abdd5781d57340e81525247f53a52f27296139bd
Author: Dave Jiang <[email protected]>
ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

This commit copies the AER info hardware buffer into a zeroed local struct
to avoid treating raw hardware bytes as software-only metadata. It then
explicitly clears the header_len and flit fields so only known-safe values
reach the TLP log printing routine.

Link: https://lore.kernel.org/linux-cxl/[email protected]/

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 06a944dadbc15..fbc88c584c067 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -137,6 +137,7 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>  			      int severity)
>  {
>  #ifdef ACPI_APEI_PCIEAER
> +	struct aer_capability_regs aer_regs = {};
>  	struct aer_capability_regs *aer;

[ ... ]

> @@ -149,7 +150,12 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>  		return;
>  
>  	aer_severity = cper_severity_to_aer(severity);
> -	aer = (struct aer_capability_regs *)pcie_err->aer_info;
> +
> +	memcpy(&aer_regs, pcie_err->aer_info, sizeof(pcie_err->aer_info));

[Severity: Medium]
This is a pre-existing issue, but the structural misalignment here appears to
cause hardware registers to be logged erroneously as TLP Prefixes.

The software struct embeds a struct pcie_tlp_log that is 60 bytes long,
whereas the actual hardware Header Log is only 16 bytes. Does mapping the
hardware AER capability buffer directly to struct aer_capability_regs cause
the Root Error Command and Root Error Status registers to map into the
aer_regs.header_log.prefix array?

Since flit is set to false below, pcie_print_tlp_log() falls back to
printing the prefix array if prefix[0] is non-zero. Could this erroneously
print raw hardware control registers as End-to-End TLP Prefixes, corrupting
logs?

> +	aer_regs.header_log.header_len = 0;
> +	aer_regs.header_log.flit = false;
> +	aer = &aer_regs;

I noticed a few other related pre-existing issues in the codebase that might
warrant a look:

[Severity: High]
This is a pre-existing issue, but is there a missing bounds check on the
error data length before processing payloads?

In extlog_print(), the code iterates over ACPI HEST generic data sections
and processes them based on section type:

drivers/acpi/acpi_extlog.c:extlog_print() {
    ...
    extlog_print_pcie(pcie_err, gdata->error_severity);
    ...
}

If a malformed or truncated payload is received from the hardware, could this
access out-of-bounds memory when downstream functions dereference the payload
fields, since gdata->error_data_length isn't validated to be at least
sizeof(struct cper_sec_pcie)?


[Severity: High]
This is a pre-existing issue, but does ghes_handle_aer() suffer from the
same unvalidated hardware data bug being fixed here, along with an
out-of-bounds read?

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

This memcpy copies 100 bytes (size of the struct) from a 96-byte array,
reading past the payload buffer. Does this also populate the software-only
flit and header_len fields with unvalidated hardware data, potentially
causing pcie_print_tlp_log() to iterate past the end of the dynamically
allocated buffer?


[Severity: High]
This is a pre-existing issue, but does cxl_rch_get_aer_info() read raw MMIO
past the hardware capability boundary?

drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_info() {
    ...
    int read_cnt = sizeof(struct aer_capability_regs) / sizeof(u32);
    ...
    for (n = 0; n < read_cnt; n++)
        aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
    ...
}

This loop reads 100 bytes from the MMIO address, which exceeds standard AER
capability sizes (56 to 72 bytes). 

Later, in cxl_handle_rdport_errors(), the polluted struct is allocated on
the stack:

drivers/cxl/core/ras_rch.c:cxl_handle_rdport_errors() {
    ...
    struct aer_capability_regs aer_regs;
    ...
}

Since the raw MMIO read pollutes the flit and header_len fields, could
pcie_print_tlp_log() use these unvalidated fields to iterate past the bounds
of the dw array and leak kernel stack memory?

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