diff mbox series

ACPI: PPTT: Fix table length check when parsing processor nodes

Message ID 20250507035124.28071-1-yangyicong@huawei.com
State New
Headers show
Series ACPI: PPTT: Fix table length check when parsing processor nodes | expand

Commit Message

Yicong Yang May 7, 2025, 3:51 a.m. UTC 
From: Yicong Yang <yangyicong@hisilicon.com>

Below error is met on my board and QEMU VM on SMT or non-SMT machine:
  ACPI PPTT: PPTT table found, but unable to locate core 31 (31)

This is because the processor node is found by iterating the PPTT
table under condition (for both acpi_find_processor_node() and
acpi_pptt_leaf_node()):
  while (entry + proc_sz < table_end)
    [parse the processor node]

If the last processor node is happened to be the last node in the
PPTT table, above condition will always be false since
entry + proc_sz == table_end. Thus the last CPU is not parsed.
Fix the loop condition to resolve the issue.

This issue is exposed by [1] but the root cause is explained above.
Before [1] entry + proc_sz is always smaller than table_end.

[1] 7ab4f0e37a0f ("ACPI PPTT: Fix coding mistakes in a couple of sizeof() calls")
Fixes: 2bd00bcd73e5 ("ACPI/PPTT: Add Processor Properties Topology Table parsing")
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/acpi/pptt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Sudeep Holla May 7, 2025, 11:47 a.m. UTC | #1 
On Wed, May 07, 2025 at 01:44:26PM +0200, Rafael J. Wysocki wrote:
> On Wed, May 7, 2025 at 1:40 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, May 07, 2025 at 11:51:24AM +0800, Yicong Yang wrote:
> > > From: Yicong Yang <yangyicong@hisilicon.com>
> > >
> > > Below error is met on my board and QEMU VM on SMT or non-SMT machine:
> > >   ACPI PPTT: PPTT table found, but unable to locate core 31 (31)
> > >
> > > This is because the processor node is found by iterating the PPTT
> > > table under condition (for both acpi_find_processor_node() and
> > > acpi_pptt_leaf_node()):
> > >   while (entry + proc_sz < table_end)
> > >     [parse the processor node]
> > >
> > > If the last processor node is happened to be the last node in the
> > > PPTT table, above condition will always be false since
> > > entry + proc_sz == table_end. Thus the last CPU is not parsed.
> > > Fix the loop condition to resolve the issue.
> > >
> > > This issue is exposed by [1] but the root cause is explained above.
> > > Before [1] entry + proc_sz is always smaller than table_end.
> > >
> >
> > Another thread [1]  with similar patch.
> 
> OK, so is this a correct fix?

While it may fix the issue on the surface, I just want to be sure there
are no other issues with the PPTT table presented from the firmware.
I will asked some questions on that thread before I can agree on the solution.
Sudeep Holla May 7, 2025, 11:55 a.m. UTC | #2 
On Wed, May 07, 2025 at 01:51:58PM +0200, Rafael J. Wysocki wrote:
> On Wed, May 7, 2025 at 1:47 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, May 07, 2025 at 01:44:26PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, May 7, 2025 at 1:40 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Wed, May 07, 2025 at 11:51:24AM +0800, Yicong Yang wrote:
> > > > > From: Yicong Yang <yangyicong@hisilicon.com>
> > > > >
> > > > > Below error is met on my board and QEMU VM on SMT or non-SMT machine:
> > > > >   ACPI PPTT: PPTT table found, but unable to locate core 31 (31)
> > > > >
> > > > > This is because the processor node is found by iterating the PPTT
> > > > > table under condition (for both acpi_find_processor_node() and
> > > > > acpi_pptt_leaf_node()):
> > > > >   while (entry + proc_sz < table_end)
> > > > >     [parse the processor node]
> > > > >
> > > > > If the last processor node is happened to be the last node in the
> > > > > PPTT table, above condition will always be false since
> > > > > entry + proc_sz == table_end. Thus the last CPU is not parsed.
> > > > > Fix the loop condition to resolve the issue.
> > > > >
> > > > > This issue is exposed by [1] but the root cause is explained above.
> > > > > Before [1] entry + proc_sz is always smaller than table_end.
> > > > >
> > > >
> > > > Another thread [1]  with similar patch.
> > >
> > > OK, so is this a correct fix?
> >
> > While it may fix the issue on the surface, I just want to be sure there
> > are no other issues with the PPTT table presented from the firmware.
> > I will asked some questions on that thread before I can agree on the solution.
> 
> Yeah, it looks like table_end points to the last byte of the table
> instead of pointing to the first byte after the end of the table.

Indeed and also we should have private resources like L1 cache described
after the initial 20 bytes of the node. So I am bit worried if this will
just hide other problems while it may solve this problem by looks of it.
This example doesn't look like a proper PPTT matching real systems.
Jonathan Cameron May 7, 2025, 2:35 p.m. UTC | #3 
On Wed, 7 May 2025 12:55:00 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

> On Wed, May 07, 2025 at 01:51:58PM +0200, Rafael J. Wysocki wrote:
> > On Wed, May 7, 2025 at 1:47 PM Sudeep Holla <sudeep.holla@arm.com> wrote:  
> > >
> > > On Wed, May 07, 2025 at 01:44:26PM +0200, Rafael J. Wysocki wrote:  
> > > > On Wed, May 7, 2025 at 1:40 PM Sudeep Holla <sudeep.holla@arm.com> wrote:  
> > > > >
> > > > > On Wed, May 07, 2025 at 11:51:24AM +0800, Yicong Yang wrote:  
> > > > > > From: Yicong Yang <yangyicong@hisilicon.com>
> > > > > >
> > > > > > Below error is met on my board and QEMU VM on SMT or non-SMT machine:
> > > > > >   ACPI PPTT: PPTT table found, but unable to locate core 31 (31)
> > > > > >
> > > > > > This is because the processor node is found by iterating the PPTT
> > > > > > table under condition (for both acpi_find_processor_node() and
> > > > > > acpi_pptt_leaf_node()):
> > > > > >   while (entry + proc_sz < table_end)
> > > > > >     [parse the processor node]
> > > > > >
> > > > > > If the last processor node is happened to be the last node in the
> > > > > > PPTT table, above condition will always be false since
> > > > > > entry + proc_sz == table_end. Thus the last CPU is not parsed.
> > > > > > Fix the loop condition to resolve the issue.
> > > > > >
> > > > > > This issue is exposed by [1] but the root cause is explained above.
> > > > > > Before [1] entry + proc_sz is always smaller than table_end.
> > > > > >  
> > > > >
> > > > > Another thread [1]  with similar patch.  
> > > >
> > > > OK, so is this a correct fix?  
> > >
> > > While it may fix the issue on the surface, I just want to be sure there
> > > are no other issues with the PPTT table presented from the firmware.
> > > I will asked some questions on that thread before I can agree on the solution.  
> > 
> > Yeah, it looks like table_end points to the last byte of the table
> > instead of pointing to the first byte after the end of the table.  
> 
> Indeed and also we should have private resources like L1 cache described
> after the initial 20 bytes of the node. So I am bit worried if this will
> just hide other problems while it may solve this problem by looks of it.
> This example doesn't look like a proper PPTT matching real systems.
> 

Assuming I'm understanding the bug correctly...

SMT systems will hit this. There will typically be no private resources
for a thread as the L1I/D shared by multiple threads (which are processor
nodes IIRC).  Note we are trying to improve the cache description in QEMU
at the moment as it would definitely be better to present caches in PPTT,
but that isn't the main issue here.

Jonathan
diff mbox series

Patch

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index f73ce6e13065..4364da90902e 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) {