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

[Jonathan Cameron <jic23@kernel.org>]

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

> cper_estatus_len() sums the firmware-controlled data_length (or
> raw_data_offset plus raw_data_length) into a u32. A data_length of
> 0xffffffec wraps that sum to 0, and a length that reads back short defeats
> every bound built on it: bert_print_all() passes its "remain <
> estatus_len" check, then advances "estatus += estatus_len" by zero and
> loops forever. GHES survives only because __ghes_check_estatus() rejects a
> length below sizeof(*estatus) first.
> 
> Reject a length that cannot be expressed in a u32 in
> cper_estatus_check_header(), which both of today's callers reach: GHES via
> __ghes_check_estatus() and BERT via cper_estatus_check(). extlog reaches
> neither yet; a later patch routes it through cper_estatus_check(), whose
> ELOG_ENTRY_LEN bound needs this to hold.
> 
> Reported-by: sashiko-bot@kernel.org
> 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")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Seems fine to me and to use the overflow.h stuff here we'd have to invent some
local variables which rather outweighs their documentation benefit.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

> ---
> v4:
> - New patch. sashiko-bot's review of v4 pointed out that the u32 sum in
>   cper_estatus_len() wraps, which bypasses the bounds added by the
>   surrounding patches.
> ---
>  drivers/firmware/efi/cper.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index ec092729cacc..ea1c999089bc 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);