[PATCH] lib/firmware_table: validate subtable bounds

[PATCH] lib/firmware_table: validate subtable bounds

[Jiale Yao]

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.

Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 lib/fw_table.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/lib/fw_table.c b/lib/fw_table.c
index 16291814450e..84c2515edde7 100644
--- 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

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

[Andrew Morton]

On Fri, 16 Oct 2026 22:41:54 +0800 Jiale Yao <yaojiale02@163.com> 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

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

[jiale yao]

At 2026-10-16 22:41:54, "Jiale Yao" <yaojiale02@163.com> 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.
>

Fixes: 60574d1e05b0 ("acpi: Create subtable parsing infrastructure")

>Signed-off-by: Jiale Yao <yaojiale02@163.com>
>---
> lib/fw_table.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
>diff --git a/lib/fw_table.c b/lib/fw_table.c
>index 16291814450e..84c2515edde7 100644
>--- 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

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

[Dave Jiang]

On 10/16/26 4:15 PM, Andrew Morton wrote:
> On Fri, 16 Oct 2026 22:41:54 +0800 Jiale Yao <yaojiale02@163.com> 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;
		}

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

[jiale yao]

At 2026-10-19 23:29:57, "Dave Jiang" <dave.jiang@intel.com> wrote:
>
>
>On 10/16/26 4:15 PM, Andrew Morton wrote:
>> On Fri, 16 Oct 2026 22:41:54 +0800 Jiale Yao <yaojiale02@163.com> 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@163.com

Jiale

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

[Dave Jiang]

On 10/21/26 8:04 AM, jiale yao wrote:
> At 2026-10-19 23:29:57, "Dave Jiang" <dave.jiang@intel.com> wrote:
>>
>>
>> On 10/16/26 4:15 PM, Andrew Morton wrote:
>>> On Fri, 16 Oct 2026 22:41:54 +0800 Jiale Yao <yaojiale02@163.com> 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@163.com

LGTM
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> 
> Jiale