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

Jeremy Linton
2025-05-07T17:59:34Z

On 5/7/25 12:35 PM, Jeremy Linton wrote:
> 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) &&

s/acpi_processor_id/number_of_priv_resources.

>                      acpi_pptt_leaf_node(table_hdr, cpu_node)) {
>                         return (struct acpi_pptt_processor *)entry;
>                 }
>
>
>