Re: [PATCH v5 05/13] ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer

[Dave Jiang <dave.jiang@intel.com>]

On 8/27/26 1:58 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Data loss of TLP Prefix Log / Extended Flit Log due to arbitrary payload truncation.

Yup. Will fix in v6.

DJ

> 
> Pre-existing issues:
> - [High] Out-of-bounds MMIO read and stack array bounds poisoning in `cxl_rch_get_aer_info()`.
> --
> 
> commit 99282f77735a9b69f6cccf74ca39147b80930bf8
> Author: Dave Jiang <dave.jiang@intel.com>
> ACPI: extlog: Avoid populating software AER metadata from raw hardware buffer
> 
> This commit addresses an issue in extlog_print_pcie() where casting the raw
> CPER aer_info buffer to a software struct populates software-only fields
> with raw hardware bytes. It copies only the leading registers and the
> Header Log DWORDs into a zeroed local struct to prevent out-of-bounds loop
> reads.
> 
> Link: https://lore.kernel.org/linux-cxl/20260709165457.8BA181F000E9@smtp.kernel.org/
> 
>> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
>> --- a/drivers/acpi/acpi_extlog.c
>> +++ b/drivers/acpi/acpi_extlog.c
> [ ... ]
>> @@ -149,7 +149,17 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>>  		return;
>>  
>>  	aer_severity = cper_severity_to_aer(severity);
>> -	aer = (struct aer_capability_regs *)pcie_err->aer_info;
>> +
>> +	/*
>> +	 * struct pcie_tlp_log is larger than the hardware layout, so only the
>> +	 * leading registers and the four Header Log DWORDs of aer_info map onto
>> +	 * the struct. Copy that much and leave the rest zero, which covers the
>> +	 * software-only header_len and flit.
>> +	 */
>> +	memcpy(&aer_regs, pcie_err->aer_info,
>> +	       offsetof(struct aer_capability_regs, header_log) +
>> +	       PCIE_STD_NUM_TLP_HEADERLOG * sizeof(u32));
> 
> [Severity: Medium]
> Will this truncated copy result in the permanent loss of the TLP Prefix Log?
> 
> The CPER aer_info array contains a raw 96-byte snapshot of the hardware AER
> capability, with the TLP Prefix Log located at offset 0x38 (56 bytes). By
> truncating the memcpy to 44 bytes, the remainder of the CPER payload is
> ignored, which silently drops the hardware-provided TLP Prefix Logs or
> Extended Flit Logs during PCIe error handling.
> 
> Would it be better to explicitly extract the prefix log from offset 56 in
> the CPER record into aer_regs.header_log.prefix to avoid data loss?
> 
> [ ... ]
> 
> This isn't a bug introduced by this patch, but I noticed a similar 
> pre-existing AER software struct issue in drivers/cxl/core/ras_rch.c.
> 
> 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;
>> ...
>> 	for (n = 0; n < read_cnt; n++)
>> 		aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
> 
> [Severity: High]
> Does this loop perform an out-of-bounds MMIO read by using the size of the
> software structure?
> 
> Using sizeof(struct aer_capability_regs) (104 bytes) instead of the actual
> hardware capability size directly maps MMIO data into software fields
> (header_len and flit at offset 84). Later, when pcie_print_tlp_log() runs,
> it uses the MMIO-poisoned header_len (which can be up to 255) as a loop
> bound over the 14-element stack array log->dw[]. This leads to a massive
> out-of-bounds stack read that can log arbitrary stack memory.
> 
> Additionally, in the same file:
> 
> drivers/cxl/core/ras_rch.c:cxl_rch_get_aer_severity()
>> 		if (aer_regs->uncor_status & PCI_ERR_ROOT_FATAL_RCV)
>> 			*severity = AER_FATAL;
> 
> [Severity: High]
> Does this code misinterpret the flow control protocol error bit?
> 
> It tests the Root Error Status flag PCI_ERR_ROOT_FATAL_RCV (0x40) against
> the uncor_status register. In the uncor_status register context, 0x40
> actually represents PCI_ERR_UNC_FCP, meaning the severity is incorrectly
> computed.
>