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);
- 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 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 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.
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 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 ?
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 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.
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?
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:42 PM Heyne, Maximilian mheyne@amazon.de 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.
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?
The code changes are fine, but the changelog is somewhat misleading.
The problem is that the original code assumed the presence of private resources at the end of every CPU entry, but in practice (due to emulation or otherwise) there are entries without them and if such an entry is located at the end of the table, it will not pass the sanity check after commit 2bd00bcd73e5. This issue was not evident previously because the code didn't work as designed.
On Wed, May 07, 2025 at 02:50:41PM +0200, Rafael J. Wysocki wrote:
On Wed, May 7, 2025 at 2:42 PM Heyne, Maximilian mheyne@amazon.de 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.
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?
The code changes are fine, but the changelog is somewhat misleading.
+1, may be look at the thread here[1], it make it clear that it is in QEMU which I missed clearly for a while. You need to emphasize the fact that this would happen only when the processor nodes are at the end of the PPTT and they have no private resources(which is generally the case in emulation as Rafaek explains below.
The problem is that the original code assumed the presence of private resources at the end of every CPU entry, but in practice (due to emulation or otherwise) there are entries without them and if such an entry is located at the end of the table, it will not pass the sanity check after commit 2bd00bcd73e5. This issue was not evident previously because the code didn't work as designed.
I completely agree.
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
On Wed, May 07, 2025 at 02:29:17PM +0000, Heyne, Maximilian wrote:
On Wed, May 07, 2025 at 01:56:53PM +0100, Sudeep Holla wrote:
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:
Ah, right I clearly missed considering multithreaded systems in my mind. I think SMTs processor nodes might have no private resource which I didn't consider before. However, your example made me open the spec and read about SMT and PPTT. There are couple of things I still don't follow below.
[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
Does the above node represent the container node for the threads within the core ? I assumes so.
[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
Is this the only thread as you mentioned the table ends here ? So it is single threaded core ? Just working what is the point in representing it in this way ? Also the ACPI Processor ID is unique, so I hope you have same number of container nodes as the threads. IOW, they ACPI Processor ID for the thread and the container node match only because it is single threaded core.
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.
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 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?
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?
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
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 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?
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) {
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 homogeneousFixes: 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.
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) {
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 homogeneousFixes: 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.
Roughly something like this:
proc_sz = sizeof(struct acpi_pptt_processor);
while ((unsigned long)entry + entry->length <= table_end) { if (entry->type == ACPI_PPTT_TYPE_PROCESSOR && entry->length >= proc_sz && cpu_node->parent == node_entry) return 0; ... }
On Wed, May 07, 2025 at 06:12:40PM +0200, Rafael J. Wysocki wrote:
On Wed, May 7, 2025 at 5:51 PM Jeremy Linton jeremy.linton@arm.com wrote:
[...]
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.
Roughly something like this:
proc_sz = sizeof(struct acpi_pptt_processor);
while ((unsigned long)entry + entry->length <= table_end) {
Yes, but in the last/termination run of the loop, entry will be > table_end, is it safe to access entry->length in that case. That's the point I was trying to make when I mentioned it is risky to use entry->length in this check. That location(outside of PPTT) might have a value that may result in entering the loop. We need to make sure the entry + offset(length) is within the table_end to access it.
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 homogeneousFixes: 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: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.
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 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; }
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; }
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; }
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)
linux-stable-mirror@lists.linaro.org
-
Heyne, Maximilian -
Jeremy Linton -
Rafael J. Wysocki -
Sudeep Holla