Re: [PATCH v4 07/13] ACPI: extlog: Fix CONFIG_ACPI_APEI_PCIEAER guard typo

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `cxl_rch_get_aer_info()` performs a MMIO read that exceeds the physical footprint of the AER capability, leading to potential out-of-bounds physical MMIO accesses.
- [High] Memory leak from the fixed-size `ghes_estatus_pool` when the AER recovery queue is full.
- [Medium] Activating the AER handler causes corrupted diagnostic output because `struct aer_capability_regs` no longer maps 1:1 to the hardware AER capability layout.
- [Medium] Concurrent lockless updates to u64 AER stats can lead to dropped counts.
--

--- Patch [7]: [PATCH v4 07/13] ACPI: extlog: Fix CONFIG_ACPI_APEI_PCIEAER guard typo ---
commit 6574c6d62d519f2f7bce0f053a2b55a5d05016ce
Author: Dave Jiang <dave.jiang@intel.com>
ACPI: extlog: Fix CONFIG_ACPI_APEI_PCIEAER guard typo

This commit fixes a typo in the preprocessor guard by adding the missing
CONFIG_ prefix. This change enables the previously compiled-out extlog
PCIe aer handling code.

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 3aec73187b519..ebedf3b136a88 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -136,7 +136,7 @@ static int print_extlog_rcd(const char *pfx,
>  static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>  			      int severity, u32 len)
>  {
> -#ifdef ACPI_APEI_PCIEAER
> +#ifdef CONFIG_ACPI_APEI_PCIEAER

[Severity: Medium]
This isn't a bug introduced by this patch, but activating this aer handler
exposes a preexisting issue. Does the memcpy() in this block overwrite
misaligned software fields?

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

Since struct aer_capability_regs contains struct pcie_tlp_log which was
expanded with software-only fields (header_len, flit), this pushes the
offset of subsequent fields beyond their hardware offsets. 

Copying the 96-byte hardware aer capability directly into the structure
misaligns these later fields, which could lead to corrupted diagnostic
output when pci_print_aer() reads them.

>  	struct aer_capability_regs aer_regs = {};
>  	struct pci_dev *pdev;
>  	unsigned int devfn;

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds mmio
read execution in the cxl core related to these aer structures.

drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_info() {
    ...
    aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
    ...
}

Can this loop read past the physical footprint of the aer capability?

Because sizeof(struct aer_capability_regs) grew due to the addition of
header_len and flit, reading this size from the aer_base mmio address
could fetch past the end of the physical aer capability registers.

This might cause bus hangs or trigger kasan violations.

[Severity: Medium]
This is a pre-existing issue, but I noticed potential lockless increment
operations on a 64-bit statistic in the aer core.

drivers/pci/pcie/aer.c:pci_dev_aer_stats_incr() {
    ...
    aer_info->dev_total_cor_errs++;
    ...
}

Can concurrent lockless updates to these u64 aer stats lead to dropped
counts? 

Since this function is called locklessly from multiple contexts (such as
aer interrupt handlers and nmi-context ghes/extlog handlers), concurrent
read-modify-write operations on u64 variables without locking or atomics
could lead to dropped counts or torn reads/writes on 32-bit architectures.

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a memory
leak in the aer core failure path when the recovery queue is full.

drivers/pci/pcie/aer.c:aer_recover_queue() {
    ...
    if (!kfifo_in_spinlocked(&aer_recover_ring, &entry, 1,
                             &aer_recover_ring_lock)) {
        pr_err("buffer overflow in recovery for %04x:%02x:%02x.%x\n",
               ...);
        return;
    }
    ...
}

Does this failure path drop allocated memory without freeing it?

ghes_handle_aer() allocates memory from the fixed-size ghes_estatus_pool
and passes it to aer_recover_queue(). 

If kfifo_in_spinlocked() fails, the function prints an error and returns. 

Since the pool is small and fixed-size, repeated overflow events could
permanently exhaust it, causing future hardware errors to be silently dropped.

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