Re: [PATCH v4 03/13] ACPI: extlog: Validate elog record length before walking sections

On 8/24/26 3:28 PM, Jonathan Cameron wrote:
> On Mon, 24 Aug 2026 10:49:26 -0700
> Dave Jiang <[email protected]> wrote:
> 
>> extlog_print() copies a fixed ELOG_ENTRY_LEN (4096) bytes from the elog
>> record into elog_buf, then walks the sections using the firmware-controlled
>> data_length. Nothing keeps data_length inside the buffer, so a malformed
>> record walks the section pointer past elog_buf and reads adjacent memory.
>> Unlike the GHES paths, extlog never calls cper_estatus_check().
>>
>> Reject a record longer than ELOG_ENTRY_LEN and run cper_estatus_check()
>> before walking the sections. The length test alone is not enough: a wrapped
>> length reads back short and passes it, which cper_estatus_check() catches
>> via the header check added earlier. Drop a malformed record with
>> NOTIFY_DONE and without MCE_HANDLED_EXTLOG, since extlog did not consume
>> it.
>>
>> Reported-by: [email protected]
>> Closes: https://sashiko.dev/#/patchset/[email protected]?part=6
>> Fixes: f6ec01da40e4 ("ACPI: extlog: Handle multiple records")
>> Reviewed-by: Alison Schofield <[email protected]>
>> Reviewed-by: Shuai Xue <[email protected]>
>> Assisted-by: Claude:claude-opus-4-8
>> Signed-off-by: Dave Jiang <[email protected]>
>> ---
>>  drivers/acpi/acpi_extlog.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
>> index 7ad3b36013cc..9ad0052aa20c 100644
>> --- a/drivers/acpi/acpi_extlog.c
>> +++ b/drivers/acpi/acpi_extlog.c
>> @@ -208,6 +208,10 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>>  
>>  	tmp = (struct acpi_hest_generic_status *)elog_buf;
>>  
>> +	/* Keep the firmware-controlled data_length inside elog_buf. */
>> +	if (cper_estatus_len(tmp) > ELOG_ENTRY_LEN || cper_estatus_check(tmp))
> 
> Why this order?  To me checking if we are in crazy world (overflow) before
> doing anything with the overflowed value makes more sense. So that would be swapping
> the two conditions.

Swapping it lets cper_estatus_check() walk the sections with data_length unbounded relative to elog_buf. elog_buf is a 4k buffer via kmalloc(ELOG_ENTRY_LEN). cper_estatus_check() iterates sections bounded by data_length, over a fixed kmalloc(ELOG_ENTRY_LEN) of 4096 bytes, reading gdata->revision at +20 and gdata->error_data_length at +24 each time. In the current order it only runs once cper_estatus_len() <= 4096 has passed, which caps data_length at 4076. Swapped, the only thing standing before the walk is cper_estatus_check_header(), and that admits data_length up to 4294967275.

Expand the comment to:

	/*
	 * Bound the length before cper_estatus_check() walks the sections: it
	 * iterates over data_length, which is not yet known to fit elog_buf.
	 * cper_estatus_check_header() then rejects a length that wrapped, which
	 * the bound cannot see.
	 */

DJ

> 
>> +		return NOTIFY_DONE;
>> +
>>  	if (!ras_userspace_consumers()) {
>>  		print_extlog_rcd(NULL, tmp, cpu);
>>  		goto out;
>