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

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

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.


> 
> 
>>
>> while ((unsigned long)entry + proc_sz <= table_end) {
>>   if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
>>   entry->length == sizeof(struct acpi_pptt_processor) +
>>      entry->number_of_priv_resources * sizeof(u32) &&
>>   entry + entry->length <= table_end &&
>>   acpi_pptt_leaf_node(...))
>>      return (...)entry;
>>
>>
>> Although at this point the while loops entry + proc_sz could just be < 
>> table_end under the assumption that entry->length will be > 0 but 
>> whichever makes more sense.
>>
>>
>>
>