Re: [PATCH 2/3] cxl/ras: Clear software-only TLP log fields after RCH AER copy

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

Pre-existing issues:
- [High] cxl_rch_get_aer_info() and cxl_dport_map_rch_aer() use sizeof(struct aer_capability_regs) (bloated to 100 bytes) for MMIO mapping and reading, causing a 44-byte out-of-bounds MMIO read past the actual hardware AER capability.
- [High] ghes_handle_aer() suffers from a 4-byte out-of-bounds memory read when copying firmware-provided CPER records due to the expanded struct aer_capability_regs.
- [High] The out-of-bounds array read vulnerability fixed in CXL is left completely unpatched in ACPI GHES and EXTLOG drivers, which also pass uninitialized firmware metadata to pcie_print_tlp_log().
--

--- Patch [2]: [PATCH 2/3] cxl/ras: Clear software-only TLP log fields after RCH AER copy ---
commit 5075cbfef0d94b8fb5de2ea2f5d342f46661acbd
Author: Dave Jiang <[email protected]>

cxl/ras: Clear software-only TLP log fields after RCH AER copy

This commit clears the software-only header_len and flit fields in the
embedded pcie_tlp_log structure after copying the AER capability from
MMIO. This prevents pcie_print_tlp_log() from walking out of bounds and
leaking stack contents into the error log.

Reported-by: [email protected]
Closes: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
> index 0a8b3b9b63884..a847727686df5 100644
> --- a/drivers/cxl/core/ras_rch.c
> +++ b/drivers/cxl/core/ras_rch.c
> @@ -69,6 +69,13 @@ static bool cxl_rch_get_aer_info(void __iomem *aer_base,
>  	for (n = 0; n < read_cnt; n++)
>  		aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));

[Severity: High]
This is a pre-existing issue, but does using sizeof(struct aer_capability_regs)
cause an out-of-bounds MMIO read here?

The loop count is calculated earlier in cxl_rch_get_aer_info() as:

    int read_cnt = sizeof(struct aer_capability_regs) / sizeof(u32);

Since the software struct is bloated to 100 bytes with the embedded
pcie_tlp_log, but the hardware AER capability is at most 56 to 72 bytes,
won't this loop read undefined or overlapping MMIO registers past the end
of the capability? This can potentially trigger synchronous external aborts
or Machine Checks on some architectures during hardware error handling.

A similar regression appears in cxl_dport_map_rch_aer():

    devm_cxl_iomap_block(host, aer_phys, sizeof(struct aer_capability_regs));

Could this map unbacked RCRB memory or overlap with other capability
registers?

> +	/*
> +	 * header_len and flit are software-only metadata in struct
> +	 * pcie_tlp_log; Clear to remove garbage register contents.
> +	 */
> +	aer_regs->header_log.header_len = 0;
> +	aer_regs->header_log.flit = false;

While this patch addresses the stack read in cxl/ras, there are pre-existing
regressions in the ACPI GHES and EXTLOG drivers caused by this same struct
expansion.

[Severity: High]
This is a pre-existing issue, but does copying sizeof(struct aer_capability_regs)
cause an out-of-bounds memory read in ghes_handle_aer()?

In drivers/acpi/apei/ghes.c:ghes_handle_aer():

    memcpy(aer_info, pcie_err->aer_info, sizeof(struct aer_capability_regs));

Since pcie_err->aer_info is strictly defined as a 96-byte array, copying
100 bytes will read past the CPER payload. If the array sits at the end of
a mapped page, could this trigger a fatal page fault during a machine check
or NMI context?

[Severity: High]
This is a pre-existing issue, but are the software-only fields left
uninitialized when passed to pcie_print_tlp_log() from the ACPI drivers?

In drivers/acpi/apei/ghes.c:ghes_handle_aer(), the struct is populated
directly from the firmware payload:

    memcpy(aer_info, pcie_err->aer_info, sizeof(struct aer_capability_regs));

And in drivers/acpi/acpi_extlog.c:extlog_print_pcie():

    aer = (struct aer_capability_regs *)pcie_err->aer_info;
    ...
    pci_print_aer(pdev, aer_severity, aer);

If firmware provides arbitrary bytes that happen to set flit to true and
header_len to a large value, could pcie_print_tlp_log() iterate up to
255 times, reading out of bounds and leaking adjacent kernel memory?

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