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

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

[Heyne, Maximilian]

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) {
-- 
2.47.1




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

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

[Sudeep Holla]

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

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

[Heyne, Maximilian]

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

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Heyne, Maximilian]

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

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

[Sudeep Holla]

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 ?

-- 
Regards,
Sudeep

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

[Rafael J. Wysocki]

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.

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

[Heyne, Maximilian]

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

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

[Rafael J. Wysocki]

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.

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

[1] https://lore.kernel.org/all/20250507035124.28071-1-yangyicong@huawei.com/

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Heyne, Maximilian]

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

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Jeremy Linton]

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.

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

[Heyne, Maximilian]

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

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

[Rafael J. Wysocki]

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?

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

[Heyne, Maximilian]

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

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

[Jeremy Linton]

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) {

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

[Rafael J. Wysocki]

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) {

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

[Jeremy Linton]

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.

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) {

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

[Rafael J. Wysocki]

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.

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;
...
}

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

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

[Jeremy Linton]

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.

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

[Jeremy Linton]

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.
> 
> 
> 

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

[Jeremy Linton]

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.
>>
>>
>>
> 

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

[Rafael J. Wysocki]

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;
 }

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

[Jeremy Linton]

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;
                 }

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

[Jeremy Linton]

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;
>                  }
> 
> 
> 

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

[Sudeep Holla]

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;
                }

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

[Sudeep Holla]

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)

-- 
Regards,
Sudeep