Re: [PATCH v4 01/13] efi/cper: Reject CPER records with an out-of-range error_data_length

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 24 Aug 2026 10:49:24 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> cper_estatus_check() sizes each section with acpi_hest_get_record_size(),
> which adds the firmware-controlled u32 error_data_length to the header size
> as a signed int (see <acpi/ghes.h>). A value in the top sizeof(*gdata)
> bytes of the u32 range wraps the sum small rather than large, so it slips
> past the "record_size > data_len" check: against the 72-byte v300 header,
> 0xffffffb9 sizes the record at 1 and acpi_hest_get_next() walks it a byte
> at a time, off the end. 0xffffffb8 sizes it at 0 and loops forever.
> 
> Sum in u64 so the check sees the real size, and reject a size the int
> helpers cannot carry, since the walk advances by their return value.
> 
> This is the per-section upper bound the later "len < sizeof(*foo)" guards
> rely on; they are lower bounds only.
> 
> Reported-by: sashiko-bot@kernel.org
> Closes: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=1
> Fixes: 45b14a4ffcc1 ("efi: cper: Fix possible out-of-bounds access")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
I know I'm late to the discussion but maybe it would just be simpler to
use check_add_overflow()?

> ---
> v4:
> - Do the arithmetic in u64 at the choke point instead of bounding the u32
>   error_data_length against data_len, so the check no longer depends on the
>   signed helpers in <acpi/ghes.h> behaving (Tony Luck). Bounding the u32
>   still left a window when data_length itself was within a header of
>   U32_MAX, and it did not stop acpi_hest_get_next() advancing by a wrapped
>   int. The v3 "< 0" arm was dead either way, since data_len is unsigned and
>   promoted the int back (Tony Luck, Shuai Xue).
> - Dropped Alison's and Shuai's Reviewed-by; the check was reworked after
>   they reviewed it.
> ---
>  drivers/firmware/efi/cper.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 06b4fdb59917..ec092729cacc 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -752,7 +752,7 @@ EXPORT_SYMBOL_GPL(cper_estatus_check_header);
>  int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>  {
>  	struct acpi_hest_generic_data *gdata;
> -	unsigned int data_len, record_size;
> +	unsigned int data_len;
>  	int rc;
>  
>  	rc = cper_estatus_check_header(estatus);
> @@ -762,11 +762,21 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>  	data_len = estatus->data_length;
>  
>  	apei_estatus_for_each_section(estatus, gdata) {
> +		u64 record_size;
> +
>  		if (acpi_hest_get_size(gdata) > data_len)
>  			return -EINVAL;
>  
> -		record_size = acpi_hest_get_record_size(gdata);
> -		if (record_size > data_len)
> +		/*
> +		 * acpi_hest_get_record_size() sums these as a signed int (see
> +		 * <acpi/ghes.h>), which wraps small for a huge
> +		 * error_data_length and slips past the check below. Sum in u64,
> +		 * and reject what those helpers cannot carry, since the walk
> +		 * advances by their return value.
> +		 */
> +		record_size = (u64)acpi_hest_get_size(gdata) +
> +			      gdata->error_data_length;

I'm late to the game obviously and what you have works but could this have
used some explicit overflow checking?  Something like

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

		if (record_size > data_len)
			return -EINVAL;

That uses the compiler __builtin_add_overflow() which checks if the infinite
precision result of the sum of the parameters would have wrapped when written
to the output one.

I think you could then drop the earlier check as well as
record_size is at least as big as acpi_hest_get_size() and we know there
was no wrap around.

	
> +		if (record_size > data_len || record_size > INT_MAX)
>  			return -EINVAL;
>  
>  		data_len -= record_size;