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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1911d3b6-f328-40a6-aa03-cde3d79554de@arm.com>
Date: Wed, 7 May 2025 10:51:38 -0500
From: Jeremy Linton <jeremy.linton@....com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: "Heyne, Maximilian" <mheyne@...zon.de>,
 "stable@...r.kernel.org" <stable@...r.kernel.org>,
 Len Brown <lenb@...nel.org>, Sudeep Holla <sudeep.holla@....com>,
 Ard Biesheuvel <ardb@...nel.org>, Catalin Marinas <catalin.marinas@....com>,
 "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ACPI/PPTT: fix off-by-one error

On 5/7/25 10:42 AM, Rafael J. Wysocki wrote:
> On Wed, May 7, 2025 at 5:25 PM Jeremy Linton <jeremy.linton@....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@...r.kernel.org
>>> Signed-off-by: Maximilian Heyne <mheyne@...zon.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.

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.

AKA the return is still potentially returning a pointer to a structure 
which may not be entirely contained in the table.


> 
>>>
>>> -     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->type == ACPI_PPTT_TYPE_PROCESSOR &&
> 
> And this checks if the current entry is a CPU one and goes to the next
> one otherwise, so it clearly looks for a CPU entry.
> 
> So the size check is logically correct now: It checks if there's
> enough space in the table to hold a CPU entry that's being looked for.
> The only problem with it is the assumption that the size of a CPU
> entry must be greater than sizeof(struct acpi_pptt_processor).
> 
> Previously, it didn't make sense at all.
> 
>>>                    cpu_node->parent == node_entry)
>>> @@ -273,7 +273,7 @@ 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) {