Re: [PATCH v3 09/10] ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Memory allocated for AER registers from the NMI-safe pool is leaked when the aer_recover_ring kfifo is full.
- [High] Memory allocated for AER registers from the NMI-safe pool is leaked when the target PCI device cannot be found during asynchronous recovery.
- [High] The CXL RCH driver reads past the physical AER capability into a stack-allocated struct, corrupting software metadata and triggering out-of-bounds stack memory accesses.
--

--- Patch [9]: [PATCH v3 09/10] ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata ---
commit 4a70c73fe1d51d5ff3adef3bd4739dd98b4c3b8d
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: APEI: GHES: Bound AER info copy and sanitize software metadata
    
Validate error_data_length, zero the destination, bound the copy to the
96-byte source, and clear header_len and flit. This mirrors the
extlog_print_pcie() fix.

Link: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=3

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 1d2966a437bdb..bd53509dcab3c 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c

[ ... ]

> @@ -664,13 +667,22 @@ static void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
>  						  sizeof(struct aer_capability_regs));
>  		if (!aer_info)
>  			return;
> -		memcpy(aer_info, pcie_err->aer_info, sizeof(struct aer_capability_regs));
> +
> +		/*
> +		 * aer_info is a fixed 96-byte buffer, smaller than struct
> +		 * aer_capability_regs, so bound the copy to the source. Clear
> +		 * the software-only header_len and flit fields afterwards so
> +		 * firmware bytes cannot drive the pcie_print_tlp_log() loop over
> +		 * dw[] out of bounds.
> +		 */
> +		memset(aer_info, 0, sizeof(struct aer_capability_regs));
> +		memcpy(aer_info, pcie_err->aer_info, sizeof(pcie_err->aer_info));
> +		aer_info->header_log.header_len = 0;
> +		aer_info->header_log.flit = false;

[Severity: High]
This isn't a bug introduced by this patch, but given the sizing fixes here
regarding struct aer_capability_regs, does the CXL RCH driver have a similar
out-of-bounds access?

In cxl_rch_get_aer_info(), the loop reads from MMIO based on the software
struct size rather than the hardware capability:

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

Reading past the capability can return 0xFFFFFFFF, which would overwrite
header_len to 0xFF and flit to true in the stack-allocated aer_regs.

When passed to pci_print_aer() in cxl_handle_rdport_errors(), could this
corrupted header_len cause pcie_print_tlp_log() to read far out-of-bounds
on the stack?

>  		aer_recover_queue(pcie_err->device_id.segment,
>  				  pcie_err->device_id.bus,
> -				  devfn, aer_severity,
> -				  (struct aer_capability_regs *)
> -				  aer_info);
> +				  devfn, aer_severity, aer_info);

[Severity: High]
This is a pre-existing issue, but does this code leak the memory allocated
from ghes_estatus_pool if the aer_recover_ring kfifo is full?

If the kfifo insertion fails in aer_recover_queue():

drivers/pci/pcie/aer.c:aer_recover_queue() {
    ...
    if (kfifo_in_spinlocked(&aer_recover_ring, &entry, 1,
                             &aer_recover_ring_lock))
        schedule_work(&aer_recover_work);
    else
        pr_err("buffer overflow in recovery for %04x:%02x:%02x.%x\n",
               domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
}

The function returns void without freeing the aer_regs pointer. Could this
deplete the NMI-safe ghes_estatus_pool during a burst of AER events?

[Severity: High]
This is also a pre-existing issue, but does this same allocation leak
during asynchronous recovery if the PCI device is not found?

When the worker processes the queue in aer_recover_work_func():

drivers/pci/pcie/aer.c:aer_recover_work_func() {
    ...
    if (!pdev) {
        pr_err_ratelimited("%04x:%02x:%02x.%x: no pci_dev found\n",
                           entry.domain, entry.bus,
                           PCI_SLOT(entry.devfn),
                           PCI_FUNC(entry.devfn));
        continue;
    }
    ...
}

If the device lookup fails, the continue statement bypasses the
ghes_estatus_pool_region_free() call at the end of the loop, which
would leave the memory chunk unreleased.

>  	}
>  #endif
>  }

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