Re: [PATCH v3 08/10] ACPI: APEI: GHES: Validate memory error section length before payload access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing payload length bounds checks for PCIe error sections before payload access in `ghes_handle_aer()`.
- [High] Missing payload length validation in vendor-specific HiSilicon PCIe error handler causes out-of-bounds read.
- [High] The `queued` status for synchronous errors is blindly overwritten during multi-section generic status blocks.
- [High] Unconditional out-of-bounds read in `cper_mem_err_pack()` for 73-byte legacy memory error records.
- [High] Short memory error records with manipulated validation bits will cause consumers to act on out-of-bounds data.
--

--- Patch [8]: [PATCH v3 08/10] ACPI: APEI: GHES: Validate memory error section length before payload access ---
commit 1aac797f010b3f5d2062204450ea2f731ee9a1d5
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: APEI: GHES: Validate memory error section length before payload access

This commit adds a payload length validation for memory error sections
in ghes_do_proc() before allowing report chain consumers to access the
data. The check ensures the length is at least the size of the older
UEFI 2.1/2.2 layout to prevent out-of-bounds reads on truncated
records.

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

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index b8dbd99da47ef..1d2966a437bdb 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -940,6 +940,17 @@ static void ghes_do_proc(struct ghes *ghes,
>  		if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) {
>  			struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>  
> +			/*
> +			 * Several consumers below dereference the record, so
> +			 * check the length once here. Bound against the shorter
> +			 * UEFI 2.1/2.2 layout that older firmware still emits;
> +			 * the extra fields are only used under validation bits
> +			 * such records leave clear.
> +			 */

[Severity: High]
This isn't a bug introduced by this patch, but it looks like short memory
error records with manipulated validation bits could cause consumers to act
on out-of-bounds data.

The commit message states that trailing fields are only acted on under
validation bits that a short record leaves clear. However, does the length
check in ghes_do_proc() enforce this correlation?

If firmware provides a 73-byte record but sets the validation bits for the
extended fields (e.g., CPER_MEM_VALID_MODULE_HANDLE), will consumers like
ghes_edac and the mc_event tracepoint trust the validation bits and actively
consume the out-of-bounds data packed by cper_mem_err_pack()?

> +			if (gdata->error_data_length <
> +			    sizeof(struct cper_sec_mem_err_old))
> +				continue;

[Severity: High]
This isn't a bug introduced by this patch, but explicitly allowing 73-byte
legacy memory error records appears to cause an unconditional out-of-bounds
read downstream.

The payload is passed to ghes_report_chain consumers, such as ghes_edac.
When ghes_edac_report_mem_error() calls cper_mem_err_pack():

drivers/firmware/efi/cper.c:cper_mem_err_pack() {
    ...
    cmem->extended = mem->extended;
    cmem->rank = mem->rank;
    cmem->mem_array_handle = mem->mem_array_handle;
    cmem->mem_dev_handle = mem->mem_dev_handle;
    ...
}

Does this unconditionally copy the trailing 7 bytes? If firmware provides a
73-byte record as permitted by this new check, does this cause an
unconditional out-of-bounds read past the end of the CPER section?

> +
>  			atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err);
>  
>  			arch_apei_report_mem_error(sev, mem_err);

[Severity: High]
This isn't a bug introduced by this patch, but the queued status for
synchronous errors seems to be blindly overwritten during multi-section
generic status blocks.

In ghes_do_proc(), right after handling the memory error:

drivers/acpi/apei/ghes.c:ghes_do_proc() {
    ...
        queued = ghes_handle_memory_failure(gdata, sev, sync);
    } else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
        queued = ghes_handle_arm_hw_error(gdata, sev, sync);
    ...
}

If a generic status block contains both a CPER_SEC_PLATFORM_MEM and a
CPER_SEC_PROC_ARM section, does the ARM handler's return value overwrite the
MEM handler's return value? If the MEM error cannot be queued, but the ARM
error is successfully queued, queued becomes true. Could this incorrectly
bypass the force_sig(SIGBUS) fallback for unhandled synchronous errors,
potentially causing an infinite exception loop upon returning to userspace?

[Severity: High]
This isn't a bug introduced by this patch, but missing payload length bounds
checks for PCIe error sections before payload access in ghes_handle_aer() can
lead to out-of-bounds reads.

When ghes_handle_aer() accesses the payload:

drivers/acpi/apei/ghes.c:ghes_handle_aer() {
    ...
    struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
    
    if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID && ...
    ...
}

Does this read pcie_err->validation_bits without checking
gdata->error_data_length? If the firmware provides a truncated PCIe section,
could this read past the bounds? If the out-of-bounds memory contains
valid-looking bits, could it copy 96 bytes of aer_info from out-of-bounds
memory into the AER recovery queue, potentially leaking kernel pool memory or
corrupting AER state?

[Severity: High]
This isn't a bug introduced by this patch, but there is a missing payload
length validation in the vendor-specific HiSilicon PCIe error handler.

When hisi_pcie_notify_error() accesses the payload:

drivers/pci/controller/pcie-hisi-error.c:hisi_pcie_notify_error() {
    ...
    const struct hisi_pcie_error_data *error_data = acpi_hest_get_payload(gdata);
    ...
    if (error_data->socket_id != socket)
        return NOTIFY_DONE;
    ...
}

Does this cast the raw payload and read error_data->socket_id without any
length validation? If the section is empty or truncated, could this read past
the end of the pool allocation? If socket_id randomly matches, could it
further read up to 132 bytes of out-of-bounds memory based on out-of-bounds
val_bits, leaking kernel pool memory?

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