Re:Re: [PATCH] lib/firmware_table: validate subtable bounds

[jiale yao]

At 2026-10-19 23:29:57, "Dave Jiang" <dave.jiang@xxxxxxxxx> wrote:
>
>
>On 10/16/26 4:15 PM, Andrew Morton wrote:
>> On Fri, 16 Oct 2026 22:41:54 +0800 Jiale Yao <yaojiale02@xxxxxxx> wrote:
>> 
>>> The generic firmware table walker dispatches matching entries after only
>>> checking that a generic subtable header is present. A record whose
>>> declared length is shorter than its header or extends past the table can
>>> therefore reach a handler before its bounds have been established.
>>>
>>> Validate the complete generic header and the declared record range
>>> before looking up or invoking a handler. This also rejects zero-length
>>> records and prevents malformed lengths from advancing the cursor outside
>>> the table.
>> 
>> Thanks.
>> 
>> get_maintainer is quite unreliable with lib/ files.  Please always use
>> git-blame when determining who should be cc'ed.
>> 
>> Dave, can you please take a look?
>> 
>>> --- a/lib/fw_table.c
>>> +++ b/lib/fw_table.c
>>> @@ -172,7 +172,20 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>>>  	    ((unsigned long)table_header + table_size);
>>>  	subtable_len = acpi_get_subtable_header_length(&entry);
>>>  
>>> -	while (((unsigned long)entry.hdr) + subtable_len < table_end) {
>>> +	while ((unsigned long)entry.hdr < table_end) {
>>> +		if (table_end - (unsigned long)entry.hdr < subtable_len) {
>>> +			pr_err("[%4.4s] Invalid subtable header\n", id);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		entry_len = acpi_get_entry_length(&entry);
>>> +		if (entry_len < subtable_len ||
>>> +		    entry_len > table_end - (unsigned long)entry.hdr) {
>>> +			pr_err("[%4.4s:0x%02lx] Invalid length %lu\n", id,
>>> +			       acpi_get_entry_type(&entry), entry_len);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>>  		for (i = 0; i < proc_num; i++) {
>>>  			if (acpi_get_entry_type(&entry) != proc[i].id)
>>>  				continue;
>>> @@ -186,16 +199,6 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>>>  			break;
>>>  		}
>>>  
>>> -		/*
>>> -		 * If entry->length is 0, break from this loop to avoid
>>> -		 * infinite loop.
>>> -		 */
>>> -		entry_len = acpi_get_entry_length(&entry);
>>> -		if (entry_len == 0) {
>>> -			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, proc->id);
>>> -			return -EINVAL;
>>> -		}
>>> -
>>>  		entry.hdr = (union acpi_subtable_headers *)
>>>  		    ((unsigned long)entry.hdr + entry_len);
>>>  	}
>>> -- 
>>> 2.34.1
>
>Can this turn a previously accepted table into a fatal parse error? My
>main concern is breaking previously tolerated behavior and we end up
>breaking deployed systems. Given this code has been around a long time.
>Maybe Rafael or Len can comment.
>
>The old loop stopped quietly when subtable_len or fewer bytes remained
>after the last entry. Here subtable_len is sizeof() the subtable header
>structure, 2 bytes for MADT or 4 for CDAT, not the entry's length field.
>
>Now fewer than subtable_len leftover bytes return -EINVAL. Exactly
>subtable_len leftover bytes are read as a header, and fail the entry_len
>check below unless they happen to hold a valid one. A table with a few
>bytes of padding after the last entry now fails the whole parse.
>
>On x86 that error is not contained to one entry type:
>
>early_acpi_process_madt()
>  early_acpi_parse_madt_lapic_addr_ovr()
>    acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC_OVERRIDE, ...)
>      -> -EINVAL
>
>	if (error == -EINVAL) {
>		...
>		pr_err("Invalid BIOS MADT, disabling ACPI\n");
>		disable_acpi();
>	}
>
>Would it be safer to keep tolerating leftover bytes too small to hold a
>subtable header, and reject only entries whose declared length is
>invalid? The loop condition can end the walk in that case without a
>separate check:
>
>	while ((unsigned long)entry.hdr + subtable_len <= table_end) {
>		entry_len = acpi_get_entry_length(&entry);
>		if (entry_len < subtable_len ||
>		    entry_len > table_end - (unsigned long)entry.hdr) {
>			...
>			return -EINVAL;
>		}
Thanks for your review,
how about this one, https://lore.kernel.org/all/20261021150159.399043-1-yaojiale02@xxxxxxx

Jiale