| 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
| ||
|
Message-ID: <89d1df2f-29d7-4442-9d92-9c29e7276b08@arm.com>
Date: Wed, 7 May 2025 12:59:30 -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 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@....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@....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@....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.
>>>>>>
>>>>>> 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;
> }
>
>
>
Powered by blists - more mailing lists