Re: [PATCH] ACPI/PPTT: fix off-by-one error

Jeremy Linton
2025-05-07T17:35:29Z

Hi,

On 5/7/25 12:01 PM, Rafael J. Wysocki wrote:
> On Wed, May 7, 2025 at 6:41 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>>
>> On 5/7/25 11:38 AM, Jeremy Linton wrote:
>>> On 5/7/25 11:31 AM, Jeremy Linton wrote:
>>>> On 5/7/25 11:12 AM, Rafael J. Wysocki wrote:
>>>>> On Wed, May 7, 2025 at 5:51 PM Jeremy Linton <jeremy.linton@arm.com>
>>>>> wrote:
>>>>>>
>>>>>> On 5/7/25 10:42 AM, Rafael J. Wysocki wrote:
>>>>>>> On Wed, May 7, 2025 at 5:25 PM Jeremy Linton
>>>>>>> <jeremy.linton@arm.com> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 5/6/25 8:13 AM, Heyne, Maximilian wrote:
>>>>>>>>> Commit 7ab4f0e37a0f ("ACPI PPTT: Fix coding mistakes in a couple of
>>>>>>>>> sizeof() calls") corrects the processer entry size but unmasked a
>>>>>>>>> longer
>>>>>>>>> standing bug where the last entry in the structure can get
>>>>>>>>> skipped due
>>>>>>>>> to an off-by-one mistake if the last entry ends exactly at the
>>>>>>>>> end of
>>>>>>>>> the ACPI subtable.
>>>>>>>>>
>>>>>>>>> The error manifests for instance on EC2 Graviton Metal instances
>>>>>>>>> with
>>>>>>>>>
>>>>>>>>> ACPI PPTT: PPTT table found, but unable to locate core 63 (63)
>>>>>>>>> [...]
>>>>>>>>> ACPI: SPE must be homogeneous
>>>>>>>>>
>>>>>>>>> Fixes: 2bd00bcd73e5 ("ACPI/PPTT: Add Processor Properties
>>>>>>>>> Topology Table parsing")
>>>>>>>>> Cc: stable@vger.kernel.org
>>>>>>>>> Signed-off-by: Maximilian Heyne <mheyne@amazon.de>
>>>>>>>>> ---
>>>>>>>>> drivers/acpi/pptt.c | 4 ++--
>>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>>>>>>>>> index f73ce6e13065d..4364da90902e5 100644
>>>>>>>>> --- a/drivers/acpi/pptt.c
>>>>>>>>> +++ b/drivers/acpi/pptt.c
>>>>>>>>> @@ -231,7 +231,7 @@ static int acpi_pptt_leaf_node(struct
>>>>>>>>> acpi_table_header *table_hdr,
>>>>>>>>> sizeof(struct acpi_table_pptt));
>>>>>>>>> proc_sz = sizeof(struct acpi_pptt_processor);
>>>>>>>>
>>>>>>>> This isn't really right, it should be struct acpi_subtable_header,
>>>>>>>> then
>>>>>>>> once the header is safe, pull the length from it.
>>>>>>>>
>>>>>>>> But then, really if we are trying to fix the original bug that the
>>>>>>>> table
>>>>>>>> could be shorter than the data in it suggests, the struct
>>>>>>>> acpi_pptt_processor length plus its resources needs to be checked
>>>>>>>> once
>>>>>>>> the subtype is known to be a processor node.
>>>>>>>>
>>>>>>>> Otherwise the original sizeof * change isn't really fixing anything.
>>>>>>>
>>>>>>> Sorry, what sense did it make to do
>>>>>>>
>>>>>>> proc_sz = sizeof(struct acpi_pptt_processor *);
>>>>>>>
>>>>>>> here? As much as proc_sz = 0 I suppose?
>>>>>>
>>>>>> No, I agree, I think the original checks were simplified along the way
>>>>>> to that. It wasn't 'right' either.
>>>>>>
>>>>>> The problem is that there are three subtypes of which processor is only
>>>>>> one, and that struct acpi_pptt_processor doesn't necessarily reflect
>>>>>> the
>>>>>> actual size of the processor structure in the table because it has
>>>>>> optional private resources tagged onto the end.
>>>>>
>>>>> Right.
>>>>>
>>>>>> So if the bug being fixed is that the length check is validating that
>>>>>> the table length is less than the data in the table, that's still a
>>>>>> problem because its only validating the processor node without
>>>>>> resources.
>>>>>
>>>>> Admittedly, it is not my code, but I understand this check as a
>>>>> termination condition for the loop: If there's not enough space in the
>>>>> table to hold a thing that I'm looking for, I may as well bail out.
>>>>>
>>>>>> AKA the return is still potentially returning a pointer to a structure
>>>>>> which may not be entirely contained in the table.
>>>>>
>>>>> Right, but this check should be made anyway before comparing
>>>>> cpu_node->parent to node_entry, when it is known to be a CPU entry
>>>>> because otherwise why bother.
>>>>
>>>> Right, but then there is a clarity because really its walking the
>>>> table+subtypes looking for the cpu node. Exiting early because its not
>>>> big enough for a cpu node makes sense but you still need the cpu node
>>>> check to avoid a variation on the original bug.
>>>>
>>>>
>>>>
>>>>>
>>>>> Roughly something like this:
>>>>>
>>>>> proc_sz = sizeof(struct acpi_pptt_processor);
>>>>>
>>>>> while ((unsigned long)entry + entry->length <= table_end) {
>>>>
>>>> Here your reading the entry, without knowing its long enough. For the
>>>> leaf check just using struct acpi_pptt_processor is fine, but for the
>>>> acpi_find_processor_node():
>>>>
>>>> proc_sz = sizeof(struct acpi_subtable_type);
>>>
>>> Although, maybe I just wrote code that justifies using
>>> acpi_pptt_processor here because the entry->num_of_priv_resources length
>>> check isn't being made without it. So ok, use proc_sz = sizeof(struct
>>> acpi_subtable_type) and assume that we don't care if the subtable type
>>> is less than proc_sz.
>>
>> Sorry for the noise, scratch that, a better solution is just to swap the
>> length checking in the 'if' statement. Then its clear its iterating
>> subtable types not processor nodes.
>
> Do you mean something like this (modulo GMail-induced whitespace damage):
>
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -231,16 +231,22 @@
> sizeof(struct acpi_table_pptt));
> proc_sz = sizeof(struct acpi_pptt_processor);
>
> - while ((unsigned long)entry + proc_sz < table_end) {
> - cpu_node = (struct acpi_pptt_processor *)entry;
> - if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
> - cpu_node->parent == node_entry)
> - return 0;
> + while ((unsigned long)entry + proc_sz <= table_end) {
> + if ((unsigned long)entry + entry->length <= table_end &&
> + entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
> + entry->length == proc_sz +
> + entry->number_of_priv_resources * sizeof(u32)) {
> + cpu_node = (struct acpi_pptt_processor *)entry;
> +
> + if (cpu_node->parent == node_entry)
> + return 0;
> + }
> +
> if (entry->length == 0)
> return 0;
> +
> entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
> entry->length);
> -
> }
> return 1;
> }
>

Right, I think we are largely on the same page, I flipflopped around
about using subtable vs processor but the processor size assumption does
remove an extra check. The version that compiles that I was about to
test (and this will take me hours) looks like:

@@ -231,7 +231,8 @@ static int acpi_pptt_leaf_node(struct
acpi_table_header *table_hdr,
sizeof(struct acpi_table_pptt));
proc_sz = sizeof(struct acpi_pptt_processor);

- while ((unsigned long)entry + proc_sz < table_end) {
+ /* ignore sub-table types that are smaller than a processor node */
+ while ((unsigned long)entry + proc_sz <= table_end) {
cpu_node = (struct acpi_pptt_processor *)entry;
if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
cpu_node->parent == node_entry)
@@ -273,15 +274,18 @@ static struct acpi_pptt_processor
*acpi_find_processor_node(struct acpi_table_he
proc_sz = sizeof(struct acpi_pptt_processor);

/* find the processor structure associated with this cpuid */
- while ((unsigned long)entry + proc_sz < table_end) {
+ while ((unsigned long)entry + proc_sz <= table_end) {
cpu_node = (struct acpi_pptt_processor *)entry;

if (entry->length == 0) {
pr_warn("Invalid zero length subtable\n");
break;
}
+ /* entry->length may not equal proc_sz, revalidate the
processor structure length */
if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
acpi_cpu_id == cpu_node->acpi_processor_id &&
+ (unsigned long)entry + entry->length <= table_end &&
+ entry->length == proc_sz +
cpu_node->acpi_processor_id * sizeof(u32) &&
acpi_pptt_leaf_node(table_hdr, cpu_node)) {
return (struct acpi_pptt_processor *)entry;
}