| Message ID | 20250506-draco-taped-15f475cd@mheyne-amazon |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI/PPTT: fix off-by-one error | expand |
On Tue, May 06, 2025 at 01:13:02PM +0000, 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. > Unless the firmware has populated an incorrect value for the header length, I don't see how this is possible. The table_end should point to the address immediately following the last byte of the table. None of the headers are only one byte long, so what am I missing that could explain this apparent off-by-one issue?.
On Tue, May 06, 2025 at 02:43:39PM +0100, Sudeep Holla wrote: > On Tue, May 06, 2025 at 01:13:02PM +0000, 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. > > > > Unless the firmware has populated an incorrect value for the header length, I > don't see how this is possible. The table_end should point to the address > immediately following the last byte of the table. None of the headers are only > one byte long, so what am I missing that could explain this apparent > off-by-one issue?. > > -- > Regards, > Sudeep Maybe calling it off-by-one is not very exact. You're right table_end points to the address following the last byte *but* (unsigned long)entry + proc_sz also points to this very byte if it's the last entry. And in this case the while condition is not taken which means we're ignoring the last processor node. For example, in our specific case the table has a length of 0xCBE and the last processor node entry is at 0xCAA with a length of 0x14 fitting exactly into the table but 0xCAA + 0x14 == 0xCBE which turns the condition false. Disclaimer: I'm not an expert in this topic but stumbled upon this bug when looking at the latest kernels. Amazon Web Services Development Center Germany GmbH Tamara-Danz-Str. 13 10243 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
On Tue, May 06, 2025 at 03:11:20PM -0500, Jeremy Linton wrote: > Hi, > > On 5/6/25 8:43 AM, Sudeep Holla wrote: > > On Tue, May 06, 2025 at 01:13:02PM +0000, 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. > > > > > > > Unless the firmware has populated an incorrect value for the header length, I > > don't see how this is possible. The table_end should point to the address > > immediately following the last byte of the table. None of the headers are only > > one byte long, so what am I missing that could explain this apparent > > off-by-one issue?. > > More likely its because the sizeof() fix was merged without proper review > and is wrong because the type isn't actually known on the object until the > header is checked. I agree that the type might not be known at this point but the condition proc_sz = sizeof(struct acpi_pptt_processor); while((unsigned long)entry + proc_sz <= table_end) would make sure that there could potentially be a node of type acpi_pptt_processor because there is at least space for it. If the entry can't be of that size because it would go over table_end then it can't be an acpi_pptt_processor. Therefore, I don't think the sizeof() fix is that wrong but we just need to adjust the while condition. Alternatively, we could at least make sure that we can safely access (without crossing table_end) the acpi_subtable_header to check the type. But the current approach seems cleaner to me. Amazon Web Services Development Center Germany GmbH Tamara-Danz-Str. 13 10243 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
On Wed, May 07, 2025 at 12:52:18PM +0100, Sudeep Holla wrote: > On Tue, May 06, 2025 at 08:08:47PM +0000, Heyne, Maximilian wrote: > > On Tue, May 06, 2025 at 02:43:39PM +0100, Sudeep Holla wrote: > > > On Tue, May 06, 2025 at 01:13:02PM +0000, 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. > > > > > > > > > > Unless the firmware has populated an incorrect value for the header length, I > > > don't see how this is possible. The table_end should point to the address > > > immediately following the last byte of the table. None of the headers are only > > > one byte long, so what am I missing that could explain this apparent > > > off-by-one issue?. > > > > > > -- > > > Regards, > > > Sudeep > > > > Maybe calling it off-by-one is not very exact. You're right table_end > > points to the address following the last byte *but* > > (unsigned long)entry + proc_sz > > also points to this very byte if it's the last entry. And in this case > > the while condition is not taken which means we're ignoring the last > > processor node. > > > > For example, in our specific case the table has a length of 0xCBE and > > the last processor node entry is at 0xCAA with a length of 0x14 fitting > > exactly into the table but 0xCAA + 0x14 == 0xCBE which turns the > > condition false. > > > > Just to understand, this node is absolutely processor node with no > private resources ? I find it hard to trust this as most of the CPUs > do have L1 I&D caches. If they were present the table can't abruptly end > like this. Yes looks like it. In our case the ACPI subtable has length 0x14 which is exactly sizeof(acpi_pptt_processor). Amazon Web Services Development Center Germany GmbH Tamara-Danz-Str. 13 10243 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
On Wed, May 07, 2025 at 01:59:12PM +0200, Rafael J. Wysocki wrote: > On Wed, May 7, 2025 at 1:53???PM Heyne, Maximilian <mheyne@amazon.de> wrote: > > > > On Tue, May 06, 2025 at 03:11:20PM -0500, Jeremy Linton wrote: > > > Hi, > > > > > > On 5/6/25 8:43 AM, Sudeep Holla wrote: > > > > On Tue, May 06, 2025 at 01:13:02PM +0000, 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. > > > > > > > > > > > > > Unless the firmware has populated an incorrect value for the header length, I > > > > don't see how this is possible. The table_end should point to the address > > > > immediately following the last byte of the table. None of the headers are only > > > > one byte long, so what am I missing that could explain this apparent > > > > off-by-one issue?. > > > > > > More likely its because the sizeof() fix was merged without proper review > > > and is wrong because the type isn't actually known on the object until the > > > header is checked. > > > > I agree that the type might not be known at this point but the condition > > > > proc_sz = sizeof(struct acpi_pptt_processor); > > while((unsigned long)entry + proc_sz <= table_end) > > > > would make sure that there could potentially be a node of type > > acpi_pptt_processor because there is at least space for it. If the entry > > can't be of that size because it would go over table_end then it can't > > be an acpi_pptt_processor. > > I don't follow. > > If it is an acpi_pptt_processor entry, the original condition would be > sufficient (assuming the correctness of the table header), wouldn't > it? On our hardware we have the following situation: For CPU 63, the acpi_pptt_processor starts at 0xCAA and it has a length of 0x14. The PPTT table has length 0xCBE. This is exactly the case I'm describing. You're traversing the entries in the table and arrive at the last entry but currently you exit the loop because 0xCAA + 0x14 == 0xCBE (it's not < 0xCBE which is table_end). So you skip over the last CPU node. > > > Therefore, I don't think the sizeof() fix is that wrong but we just need > > to adjust the while condition. > > The sizeof() fix is correct, it makes the code work as designed. > > > Alternatively, we could at least make sure that we can safely access > > (without crossing table_end) the acpi_subtable_header to check the type. > > Yes. > > > But the current approach seems cleaner to me. > > Why do you think so? Because the first line in the loop casts straight to acpi_pptt_processor. That's why I think it makes sense to check the entry at which you arrived can actually be a acpi_pptt_processor space-wise. Amazon Web Services Development Center Germany GmbH Tamara-Danz-Str. 13 10243 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
On Wed, May 7, 2025 at 2:31 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, May 07, 2025 at 11:56:48AM +0000, Heyne, Maximilian wrote: > > On Wed, May 07, 2025 at 12:52:18PM +0100, Sudeep Holla wrote: > > > > > > Just to understand, this node is absolutely processor node with no > > > private resources ? I find it hard to trust this as most of the CPUs > > > do have L1 I&D caches. If they were present the table can't abruptly end > > > like this. > > > > Yes looks like it. In our case the ACPI subtable has length 0x14 which is > > exactly sizeof(acpi_pptt_processor). > > > > OK, this seem like it is emulated platform with no private resources as > it is specified in the other similar patch clearly(QEMU/VM). So this > doesn't match real platforms. Your PPTT is wrong if it is real hardware > platform as you must have private resources. > > Anyways if we allow emulation to present CPUs without private resources > we may have to consider allowing this as the computed pointer will match > the table end. > > Rafael, > > If it is OK for QEMU to present cacheless CPUs, then we need to allow > this logic. What do you think ? I don't see why QEMU would be disallowed to do so.
On Wed, May 07, 2025 at 12:42:14PM +0000, Heyne, Maximilian wrote: > On Wed, May 07, 2025 at 01:30:53PM +0100, Sudeep Holla wrote: > > On Wed, May 07, 2025 at 11:56:48AM +0000, Heyne, Maximilian wrote: > > > On Wed, May 07, 2025 at 12:52:18PM +0100, Sudeep Holla wrote: > > > > > > > > Just to understand, this node is absolutely processor node with no > > > > private resources ? I find it hard to trust this as most of the CPUs > > > > do have L1 I&D caches. If they were present the table can't abruptly end > > > > like this. > > > > > > Yes looks like it. In our case the ACPI subtable has length 0x14 which is > > > exactly sizeof(acpi_pptt_processor). > > > > > > > OK, this seem like it is emulated platform with no private resources as > > it is specified in the other similar patch clearly(QEMU/VM). So this > > doesn't match real platforms. Your PPTT is wrong if it is real hardware > > platform as you must have private resources. > > > > Anyways if we allow emulation to present CPUs without private resources > > we may have to consider allowing this as the computed pointer will match > > the table end. > > Is there a need by the ACPI specification that the Cache information > must come after the processor information? Because on our platform there > is Cache and it's described but at a different location seemingly. It > looks like caches are described first and then the CPUs. > That is fine but you must have reference to those caches in the processor node and the length of the node won't be 0x14 in that case and you shouldn't hit this issue. So if this is real platform, then yes I am must say you PPTT is wrong especially if there are caches in the table as you say just that processor nodes are not pointing to them correctly then ? > I can try to drill even deeper here if you insist. As said I'm no > subject matter expert here. But is there something obviously wrong with > my patch or would it be ok to just take it? > Yes you much check your PPTT if it is real hardware platform. I am OK with the change in terms of QEMU or VM. You may need to reword commit message a bit. I will respond separately.
On Wed, May 07, 2025 at 01:56:53PM +0100, Sudeep Holla wrote: > On Wed, May 07, 2025 at 12:42:14PM +0000, Heyne, Maximilian wrote: > > On Wed, May 07, 2025 at 01:30:53PM +0100, Sudeep Holla wrote: > > > On Wed, May 07, 2025 at 11:56:48AM +0000, Heyne, Maximilian wrote: > > > > On Wed, May 07, 2025 at 12:52:18PM +0100, Sudeep Holla wrote: > > > > > > > > > > Just to understand, this node is absolutely processor node with no > > > > > private resources ? I find it hard to trust this as most of the CPUs > > > > > do have L1 I&D caches. If they were present the table can't abruptly end > > > > > like this. > > > > > > > > Yes looks like it. In our case the ACPI subtable has length 0x14 which is > > > > exactly sizeof(acpi_pptt_processor). > > > > > > > > > > OK, this seem like it is emulated platform with no private resources as > > > it is specified in the other similar patch clearly(QEMU/VM). So this > > > doesn't match real platforms. Your PPTT is wrong if it is real hardware > > > platform as you must have private resources. > > > > > > Anyways if we allow emulation to present CPUs without private resources > > > we may have to consider allowing this as the computed pointer will match > > > the table end. > > > > Is there a need by the ACPI specification that the Cache information > > must come after the processor information? Because on our platform there > > is Cache and it's described but at a different location seemingly. It > > looks like caches are described first and then the CPUs. > > > > That is fine but you must have reference to those caches in the processor > node and the length of the node won't be 0x14 in that case and you shouldn't > hit this issue. So if this is real platform, then yes I am must say you > PPTT is wrong especially if there are caches in the table as you say just > that processor nodes are not pointing to them correctly then ? The ACPI tables in our case describe a core first which references the cache as private resource and then a thread whose parent is the core but this doesn't have a private resource. This is how it looks like: [C8Eh 3214 1] Subtable Type : 00 [Processor Hierarchy Node] [C8Fh 3215 1] Length : 1C [C90h 3216 2] Reserved : 0000 [C92h 3218 4] Flags (decoded below) : 00000002 Physical package : 0 ACPI Processor ID valid : 1 Processor is a thread : 0 Node is a leaf : 0 Identical Implementation : 0 [C96h 3222 4] Parent : 000000A2 [C9Ah 3226 4] ACPI Processor ID : 0000003F [C9Eh 3230 4] Private Resource Number : 00000002 [CA2h 3234 4] Private Resource : 00000072 [CA6h 3238 4] Private Resource : 0000008A [CAAh 3242 1] Subtable Type : 00 [Processor Hierarchy Node] [CABh 3243 1] Length : 14 [CACh 3244 2] Reserved : 0000 [CAEh 3246 4] Flags (decoded below) : 0000000E Physical package : 0 ACPI Processor ID valid : 1 Processor is a thread : 1 Node is a leaf : 1 Identical Implementation : 0 [CB2h 3250 4] Parent : 00000C8E [CB6h 3254 4] ACPI Processor ID : 0000003F [CBAh 3258 4] Private Resource Number : 00000000 > > > I can try to drill even deeper here if you insist. As said I'm no > > subject matter expert here. But is there something obviously wrong with > > my patch or would it be ok to just take it? > > > > Yes you much check your PPTT if it is real hardware platform. I am OK > with the change in terms of QEMU or VM. You may need to reword commit > message a bit. I will respond separately. > > -- > Regards, > Sudeep Amazon Web Services Development Center Germany GmbH Tamara-Danz-Str. 13 10243 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
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. > > - 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 && > 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) {
On Wed, May 07, 2025 at 10:25:25AM -0500, Jeremy Linton 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. > Ah OK. Sorry I wasn't able to understand your point earlier. I get it now. But just for sake of argument here, accessing entry->length before doing some sanity check is also risky. So ideally we should be checking if entry + entry->length <= table_end right ? > 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. > Indeed. > Otherwise the original sizeof * change isn't really fixing anything. > How about extending the check for entry->length ? Do you think it will be any better ? The entry pointer is anyway updated to jump entry->length ahead at the end of the loop. Regards, Sudeep -->8 @@ -276,7 +276,7 @@ static struct acpi_pptt_processor *acpi_find_processor_node(struct acpi_table_he while ((unsigned long)entry + proc_sz <= table_end) { cpu_node = (struct acpi_pptt_processor *)entry; - if (entry->length == 0) { + if (!entry->length || entry->length < proc_sz) { pr_warn("Invalid zero length subtable\n"); break; }
On Wed, May 07, 2025 at 04:47:10PM +0100, Sudeep Holla wrote: > On Wed, May 07, 2025 at 10:25:25AM -0500, Jeremy Linton wrote: [...] > > > Otherwise the original sizeof * change isn't really fixing anything. > > > > How about extending the check for entry->length ? Do you think it will > be any better ? The entry pointer is anyway updated to jump entry->length > ahead at the end of the loop. > Scratch that, we will still end up reading an invalid node at the end if (entry + entry->length > table_end)
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); 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.
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. >> >> >> >
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; > } > > >
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); - 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 && 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) {
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(-)