Re: [PATCH v3 04/10] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but ghes_handle_aer() completely misses the fix applied in this patch. It reads 100 bytes from a 96-byte array, triggering a FORTIFY_SOURCE panic, and leaves software-only fields uncleared, leading to a heap information leak.
- [High] This problem wasn't introduced by this patch, but cxl_rch_get_aer_info() calculates MMIO reads using the inflated software struct size, causing an out-of-bounds MMIO read and populating a stack-allocated struct with garbage, driving a stack information leak.
- [High] This problem wasn't introduced by this patch, but aer_recover_queue() fails to free the dynamically allocated aer_capability_regs when the kfifo is full, permanently leaking memory from the critical ghes_estatus_pool.
--

--- Patch [4]: [PATCH v3 04/10] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer ---
commit 61009f4d553f47557d961ce2fe251ac8b1878dfd
Author: Dave Jiang <[email protected]>
ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

This patch resolves an out-of-bounds read by copying the raw aer_info
buffer into a local struct and clearing the software-only fields.
It prevents hardware data from overwriting header_len and flit.

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;
>  	struct pci_dev *pdev;
>  	unsigned int devfn;
[ ... ]
>  	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));
> +	aer_regs.header_log.header_len = 0;
> +	aer_regs.header_log.flit = false;
> +	aer = &aer_regs;
> +
>  	domain = pcie_err->device_id.segment;
>  	bus = pcie_err->device_id.bus;
>  	devfn = PCI_DEVFN(pcie_err->device_id.device,

While reviewing this change, I noticed a few related pre-existing issues in
other files that might need similar attention.

drivers/acpi/apei/ghes.c:ghes_handle_aer() {
[ ... ]
		aer_info = (void *)gen_pool_alloc(ghes_estatus_pool,
						  sizeof(struct aer_capability_regs));
		if (!aer_info)
			return;
		memcpy(aer_info, pcie_err->aer_info, sizeof(struct aer_capability_regs));
[ ... ]
}

[Severity: High]
This isn't a bug introduced by this patch, but ghes_handle_aer() completely
misses the fix applied in this patch.

Does this read 100 bytes from the 96-byte pcie_err->aer_info array?

Could this trigger a strict fortify source buffer overflow panic?

Also, since the software-only fields aren't cleared, could
pcie_print_tlp_log() read out of bounds from the heap-allocated dw array,
leaking adjacent ghes_estatus_pool memory into the system log?


drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_info() {
[ ... ]
	int read_cnt = sizeof(struct aer_capability_regs) / sizeof(u32);
	u32 *aer_regs_buf = (u32 *)aer_regs;
	int n;
[ ... ]
	/* Use readl() to guarantee 32-bit accesses */
	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 cxl_rch_get_aer_info() calculates
mmio reads using the inflated software struct size.

Could this cause an out-of-bounds unmapped mmio read, potentially triggering
a machine check exception or unsupported request error?

Additionally, since the aer_regs struct is allocated on the stack in
cxl_handle_rdport_errors(), couldn't reading unmapped mmio fill the flit
and header_len fields with garbage?

If passed to pcie_print_tlp_log(), this might drive an out-of-bounds loop
that leaks kernel stack frames into the system log.


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));
}

[Severity: High]
This is a pre-existing issue, but aer_recover_queue() fails to free the
dynamically allocated aer_capability_regs when the kfifo is full.

Since ghes_handle_aer() allocates this buffer from ghes_estatus_pool and
hands ownership to aer_recover_queue(), does dropping it on the kfifo
overflow error path permanently leak memory from the pool?

Could a burst of pcie aer errors exhaust the pool and break subsequent
ras handling?

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