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

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

Pre-existing issues:
- [High] Missing bounds check before reading section header fields in cper_estatus_check(), leading to an out-of-bounds read.
--

--- Patch [2]: [PATCH v4 02/13] efi/cper: Reject an error status block length that wraps a u32 ---
commit 50e02caac680576dbb8a9d80c6f2e251318a9c08
Author: Dave Jiang <[email protected]>

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

This commit prevents an integer overflow in cper_estatus_len() by adding
bounds checks to cper_estatus_check_header(). The new checks ensure that a
firmware-controlled data length cannot wrap a u32, avoiding infinite loops
in callers like bert_print_all().

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index ec092729caccb..ea1c999089bc1 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;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(cper_estatus_check_header);

[Severity: High]
This is a pre-existing issue, but there appears to be a missing bounds
check in cper_estatus_check() that could lead to an out-of-bounds read.

The function iterates over error status sections:

drivers/firmware/efi/cper.c:cper_estatus_check() {
    ...
    apei_estatus_for_each_section(estatus, gdata) {
        u64 record_size;

        if (acpi_hest_get_size(gdata) > data_len)
            return -EINVAL;
    ...
}

The loop condition in apei_estatus_for_each_section() only checks that the
next section starts within the total data_length. If a malformed record
leaves a remaining data_len greater than 0 but less than
sizeof(struct acpi_hest_generic_data), the loop will still process it.

Inside the loop, the very first operation is acpi_hest_get_size(gdata). This
internally evaluates gdata->revision via acpi_hest_get_version():

include/acpi/ghes.h:acpi_hest_get_version() {
    return gdata->revision >> 8;
}

Since gdata->revision is at offset 20, reading it when the remaining
data_len is too small (for example, if only 10 bytes remain) reads memory
beyond the end of estatus->data_length.

Could this cause a kernel panic if the read crosses into an unmapped page?

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