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

[Jeremy Linton <jeremy.linton@arm.com>]

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;
                 }