Re: [PATCH v2 1/2] PCI/AER: Fix memory leak in aer_recover_queue() on kfifo buffer overflow

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Preexisting memory leak of `aer_regs` in `aer_recover_work_func()` when the associated PCI device is not found.
- [High] Out-of-bounds memory read and ABI layout mismatch when copying and parsing CPER AER payload.
--

--- Patch [1]: [PATCH v2 1/2] PCI/AER: Fix memory leak in aer_recover_queue() on kfifo buffer overflow ---
commit b967f56b707635c965827309db44195cb8cee5ff
Author: Priyank Rathod <rathodpriyank@google.com>

PCI/AER: Fix memory leak in aer_recover_queue() on kfifo buffer overflow

When ACPI APEI/GHES processes PCIe AER error records, it allocates memory
for aer_capability_regs from ghes_estatus_pool. If kfifo_in_spinlocked()
fails due to a buffer overflow, the allocated memory is not freed. This
commit fixes the leak by calling ghes_estatus_pool_region_free() on the
error path.

> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index c4fd9c0b2a548..967cde9dd5191 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1274,9 +1274,12 @@ void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
>  	if (kfifo_in_spinlocked(&aer_recover_ring, &entry, 1,
>  				 &aer_recover_ring_lock))
>  		schedule_work(&aer_recover_work);
> -	else
> +	else {
>  		pr_err("buffer overflow in recovery for %04x:%02x:%02x.%x\n",
>  		       domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> +		ghes_estatus_pool_region_free((unsigned long)aer_regs,
> +					    sizeof(struct aer_capability_regs));
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but while you are fixing memory
leaks of aer_regs from ghes_estatus_pool, there appears to be another
pre-existing leak in the worker function aer_recover_work_func().

If pci_get_domain_bus_and_slot() fails to find the PCI device, it executes
a continue statement:

drivers/pci/pcie/aer.c:aer_recover_work_func() {
        ...
		pdev = pci_get_domain_bus_and_slot(entry.domain, entry.bus,
						   entry.devfn);
		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;
		}
        ...
        ghes_estatus_pool_region_free((unsigned long)entry.regs,
					    sizeof(struct aer_capability_regs));
}

Does this continue permanently leak entry.regs by skipping the
ghes_estatus_pool_region_free() call at the bottom of the loop?

[Severity: High]
This is a pre-existing issue, but since this patch interacts with
struct aer_capability_regs allocation, I noticed a potential struct layout
mismatch that could lead to an out-of-bounds read and corrupted hardware
error log parsing.

In drivers/acpi/apei/ghes.c:ghes_handle_aer(), the CPER payload is copied
using the size of the software structure:

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

However, in include/linux/cper.h, pcie_err->aer_info is strictly a 96-byte
array. struct aer_capability_regs in include/linux/aer.h incorporates
struct pcie_tlp_log (60 bytes), expanding the struct size to 100 bytes and
shifting subsequent hardware fields (such as root_command).

Does this memcpy read 4 bytes past the end of the GHES payload? When
pci_print_aer() subsequently calls pcie_print_tlp_log(), could it read the
hardware Root Error Command register as if it were a TLP prefix, outputting
garbage data?

>  }
>  EXPORT_SYMBOL_GPL(aer_recover_queue);
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-b4-fix-aer-memleaks-v2-0-fd199b0171fd@google.com?part=1