Re: [PATCH v6 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] Out-of-bounds read in CPER error status validation due to insufficient remaining length check
--

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

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

cper_estatus_len() sums the firmware-controlled data_length into a u32,
which can wrap to 0. This patch rejects a length that cannot be expressed in
a u32 in cper_estatus_check_header() to prevent callers from receiving a
short value.

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

> 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;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(cper_estatus_check_header);

[Severity: High]
This isn't a bug introduced by this patch, but while looking at validation
for estatus->data_length here, I noticed an out-of-bounds read issue in how
the sections are parsed by cper_estatus_check().

If firmware provides an estatus block where the data_length leaves between 1
and 63 bytes (smaller than sizeof(struct acpi_hest_generic_data)) after a
valid section, the apei_estatus_for_each_section() loop will erroneously
enter another iteration due to its boundary condition:

include/acpi/ghes.h:apei_estatus_for_each_section()
	for (section = (struct acpi_hest_generic_data *)(estatus + 1);	\
	     (void *)section - (void *)(estatus + 1) < estatus->data_length; \
	     section = acpi_hest_get_next(section))

The subsequent access to acpi_hest_get_size(gdata) and
gdata->error_data_length in cper_estatus_check() will then read past the
valid data_length.

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

		if (check_add_overflow(acpi_hest_get_size(gdata),
				       gdata->error_data_length, &record_size))
			return -EINVAL;

A heap out-of-bounds read here can cause a page fault in MCE or NMI context,
leading to a fatal machine check (kernel panic).

Could we tighten the apei_estatus_for_each_section loop boundary or add a
check in cper_estatus_check() to ensure the remaining data_length is at
least the size of a section header before allowing the next iteration?

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