Re: [PATCH v5 02/13] efi/cper: Reject an error status block length that wraps a u32

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double-fetch (TOCTOU) vulnerability in GHES error record parsing leading to out-of-bounds reads in the kernel heap.
--

commit ddc6acba60cbd7b0aee24cf35b70654e2a951cb0
Author: Dave Jiang <dave.jiang@intel.com>

efi/cper: Reject an error status block length that wraps a u32

This commit adds bounds checking to cper_estatus_check_header() to prevent
a u32 wrap-around when calculating the total length of the error status
block. It rejects lengths that cannot be expressed in a u32 to protect
callers like GHES and BERT from processing short values and looping forever.

Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=1
Fixes: 06d65deade9a ("ACPI, APEI, UEFI Common Platform Error Record (CPER) header")

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 332c78f2d5617..3dcef618ad39f 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -745,6 +745,17 @@ int cper_estatus_check_header(const struct acpi_hest_generic_status *estatus)
>  	    estatus->raw_data_offset < sizeof(*estatus) + estatus->data_length)
>  		return -EINVAL;
>  
> +	/*
> +	 * cper_estatus_len() sums these into a u32, and a wrapped sum reads
> +	 * back smaller than the record. Reject a length that cannot be
> +	 * expressed so no caller is handed the short value.
> +	 */
> +	if ((u64)sizeof(*estatus) + estatus->data_length > U32_MAX)
> +		return -EINVAL;
> +	if (estatus->raw_data_length &&
> +	    (u64)estatus->raw_data_offset + estatus->raw_data_length > U32_MAX)
> +		return -EINVAL;
> +

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing 
double-fetch (TOCTOU) vulnerability that these new checks do not mitigate.

The GHES driver fetches the error status header from shared physical memory
twice. The first fetch in ghes_in_nmi_queue_one_entry() determines the
buffer allocation size:

drivers/acpi/apei/ghes.c:ghes_in_nmi_queue_one_entry() {
        ...
        rc = __ghes_peek_estatus(ghes, &tmp_header, &buf_paddr, fixmap_idx);
        ...
        len = cper_estatus_len(&tmp_header);

The second fetch copies exactly len bytes from shared memory, overwriting
the previously validated header:

        if (__ghes_read_estatus(estatus, buf_paddr, fixmap_idx, len)) {
        ...
}

If the data_length field in physical memory is maliciously or erroneously
increased between these two reads, the newly copied header will contain a
data_length that exceeds the originally allocated buffer size (len).

The new untrusted data_length is then passed to cper_estatus_check():

drivers/firmware/efi/cper.c:cper_estatus_check() {
        ...
        data_len = estatus->data_length;
        apei_estatus_for_each_section(estatus, gdata) {
        ...
}

Because the checks here in cper_estatus_check_header() do not verify if
the new data_length exceeds the actual copied buffer size (len), the loop
in cper_estatus_check() will read out of bounds into the kernel heap.

This can lead to an out-of-bounds memory disclosure to dmesg, or a kernel
panic if it crosses into unmapped memory during NMI or IRQ context.

Does it make sense to update the caller to pass the allocated buffer size
into cper_estatus_check_header() so we can ensure the newly fetched length
does not exceed the buffer?

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