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

On 8/24/26 2:57 PM, Jonathan Cameron wrote:
> On Mon, 24 Aug 2026 10:49:24 -0700
> Dave Jiang <[email protected]> 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: [email protected]
>> Closes: https://sashiko.dev/#/patchset/[email protected]?part=1
>> Fixes: 45b14a4ffcc1 ("efi: cper: Fix possible out-of-bounds access")
>> Assisted-by: Claude:claude-opus-4-8
>> Signed-off-by: Dave Jiang <[email protected]>
> 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

Not late at all.

> 
> 		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.
> 

Yup I'll do that.

DJ

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