ACPI: GTDT: Relax sanity checking on Platform Timers array count

PATCH Applied Oliver Upton 4 participants 8 messages Jan 28, 2025 – Feb 13, 2025

Subsystems (3)

Stable branch ACPI ACPI arm64

lore

Share & tools

lore

Oliver Upton

author

2025-01-28 00:18 UTC #0 lore raw

PATCH 1/1 ACPI: GTDT: Relax sanity checking on Platform Timers array count

Perhaps unsurprisingly there are some platforms where the GTDT isn't quite right and the Platforms Timer array overflows the length of the overall table.

While the recently-added sanity checking isn't wrong, it makes it impossible to boot the kernel on offending platforms. Try to hobble along and limit the Platform Timer count to the bounds of the table.

Cc Marc Zyngier <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Zheng Zengkai <[email protected]>

Cc [email protected]

Fixes 263e22d6bd1f ("ACPI: GTDT: Tighten the check for the array of platform timer structures")

Signed-off-by Oliver Upton <[email protected]>

---
 drivers/acpi/arm64/gtdt.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/acpi/arm64/gtdt.c +8 -4

index 3561553eff8b..70f8290b659d 100644
@@ -163,7 +163,7 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
 {
 	void *platform_timer;
 	struct acpi_table_gtdt *gtdt;
-	int cnt = 0;
+	u32 cnt = 0;
 
 	gtdt = container_of(table, struct acpi_table_gtdt, header);
 	acpi_gtdt_desc.gtdt = gtdt;

@@ -188,13 +188,17 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
 		cnt++;
 
 	if (cnt != gtdt->platform_timer_count) {
+		cnt = min(cnt, gtdt->platform_timer_count);
+		pr_err(FW_BUG "limiting Platform Timer count to %d\n", cnt);
+	}
+
+	if (!cnt) {
 		acpi_gtdt_desc.platform_timer = NULL;
-		pr_err(FW_BUG "invalid timer data.\n");
-		return -EINVAL;
+		return 0;
 	}
 
 	if (platform_timer_count)
-		*platform_timer_count = gtdt->platform_timer_count;
+		*platform_timer_count = cnt;
 
 	return 0;
 }

base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04

Signature · 1 line

2.48.1.262.g85cc9f2d1e-goog

Marc Zyngier

Acked-by on v1 1/1

2025-01-28 08:46 UTC #1 lore raw

Quoted 4 lines · Oliver Upton

Perhaps unsurprisingly there are some platforms where the GTDT isn't quite right and the Platforms Timer array overflows the length of the overall table.

Colour me shocked! Broken ACPI tables? [insert REM song here].

Quoted 11 lines

While the recently-added sanity checking isn't wrong, it makes it impossible to boot the kernel on offending platforms. Try to hobble along and limit the Platform Timer count to the bounds of the table.
Cc Marc Zyngier <[email protected]>
Cc Lorenzo Pieralisi <[email protected]>
Cc Zheng Zengkai <[email protected]>
Cc [email protected]
Fixes 263e22d6bd1f ("ACPI: GTDT: Tighten the check for the array of platform timer structures")
Signed-off-by Oliver Upton <[email protected]>

Acked-by Marc Zyngier <[email protected]>

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Lorenzo Pieralisi

maintainer Reviewed-by on v1 1/1

2025-01-28 10:51 UTC #2 lore raw

In reply to Oliver Upton

Quoted 1 line · Oliver Upton

Perhaps unsurprisingly there are some platforms where the GTDT isn't

https://lore.kernel.org/lkml/Zw6b3V5Mk2tIGmy5@lpieralisi

An accident waiting to happen :)

Quoted 31 lines

quite right and the Platforms Timer array overflows the length of the overall table.
While the recently-added sanity checking isn't wrong, it makes it impossible to boot the kernel on offending platforms. Try to hobble along and limit the Platform Timer count to the bounds of the table.
Cc Marc Zyngier <[email protected]>
Cc Lorenzo Pieralisi <[email protected]>
Cc Zheng Zengkai <[email protected]>
Cc [email protected]
Fixes 263e22d6bd1f ("ACPI: GTDT: Tighten the check for the array of platform timer structures")
Signed-off-by Oliver Upton <[email protected]>
---
 drivers/acpi/arm64/gtdt.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/acpi/arm64/gtdt.c +1 -1
index 3561553eff8b..70f8290b659d 100644
@@ -163,7 +163,7 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
 {
 	void *platform_timer;
 	struct acpi_table_gtdt *gtdt;
-	int cnt = 0;
+	u32 cnt = 0;
 
 	gtdt = container_of(table, struct acpi_table_gtdt, header);
 	acpi_gtdt_desc.gtdt = gtdt;

drivers/acpi/arm64/gtdt.c int __init acpi_gtdt_init(struct acpi_table_header *table, · line 191

Quoted 4 lines

drivers/acpi/arm64/gtdt.c +1 -0

@@ -188,13 +188,17 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
 		cnt++;
 
 	if (cnt != gtdt->platform_timer_count) {
+		cnt = min(cnt, gtdt->platform_timer_count);

Thank you for reporting this.

There is something I need to understand.

What's wrong cnt (because platform_timer_valid() fails for some reason on some entries whereas before the commit we are fixing was applied we *were* parsing those entries) or gtdt->platform_timer_count ?

I *guess* the issue is the following:

gtdt->platform_timer_count reports the number of GT blocks in the GTDT not including Arm generic watchdogs, whereas cnt counts both structure types (and that's what gtdt->platform_timer_count should report too if it was correct).

I am asking just to understand if platform_timer_valid() forced skipping some entries that we were parsing before the commit we are fixing was applied I doubt it but it is worth checking.

drivers/acpi/arm64/gtdt.c int __init acpi_gtdt_init(struct acpi_table_header *table, · line 201

Quoted 13 lines

drivers/acpi/arm64/gtdt.c +6 -3

+		pr_err(FW_BUG "limiting Platform Timer count to %d\n", cnt);
+	}`
+
+	if (!cnt) {
 		acpi_gtdt_desc.platform_timer = NULL;
-		pr_err(FW_BUG "invalid timer data.\n");
-		return -EINVAL;
+		return 0;
 	}
 
 	if (platform_timer_count)
-		*platform_timer_count = gtdt->platform_timer_count;
+		*platform_timer_count = cnt;

I think this should be fine as things stand (but see above).

It is used in:

gtdt_sbsa_gwdt_init() - just to check if there are platform timers entries

arch_timer_mem_acpi_init() - to create a temporary array to init arch mem timer
entries (the array is oversized because it includes watchdog entries in the count)

In both cases taking the

min(cnt, gtdt->platform_timer_count);

should work AFAICS (hard to grok though, we - as in ACPI maintainers - need to clean this up).

Reviewed-by Lorenzo Pieralisi <[email protected]>

Quoted 8 lines

drivers/acpi/arm64/gtdt.c +0 -0
 	return 0;
 }
 
base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
Signature · 1 line
2.48.1.262.g85cc9f2d1e-goog

Oliver Upton

author

2025-01-28 20:42 UTC #3 lore raw

Hi Lorenzo,

Quoted 15 lines · Lorenzo Pieralisi

drivers/acpi/arm64/gtdt.c int __init acpi_gtdt_init(struct acpi_table_header *table, · line 191
Quoted 5 lines
drivers/acpi/arm64/gtdt.c +1 -0
@@ -188,13 +188,17 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
 		cnt++;
 
 	if (cnt != gtdt->platform_timer_count) {
+		cnt = min(cnt, gtdt->platform_timer_count);
Thank you for reporting this.
There is something I need to understand.
What's wrong cnt (because platform_timer_valid() fails for some reason on some entries whereas before the commit we are fixing was applied we *were* parsing those entries) or gtdt->platform_timer_count ?

Quoted 6 lines

I *guess* the issue is the following:
gtdt->platform_timer_count reports the number of GT blocks in the GTDT not including Arm generic watchdogs, whereas cnt counts both structure types (and that's what gtdt->platform_timer_count should report too if it was correct).

I've seen two different issues so far:

- In one case, the offset of the platform timer array is entirely beyond the GTDT

- In another, the GTDT has a timer array of length 2, but only the first structure falls within the length of the overall GTDT

Since cnt is the result of doing a bounds-checked walk of the platform timer array, both of these issues cause the sanity check to fail.

Quoted 14 lines

drivers/acpi/arm64/gtdt.c int __init acpi_gtdt_init(struct acpi_table_header *table, · line 201
Quoted 3 lines
drivers/acpi/arm64/gtdt.c +1 -1
 	if (platform_timer_count)
-		*platform_timer_count = gtdt->platform_timer_count;
+		*platform_timer_count = cnt;
I think this should be fine as things stand (but see above).
It is used in:
gtdt_sbsa_gwdt_init() - just to check if there are platform timers entries
arch_timer_mem_acpi_init() - to create a temporary array to init arch mem timer
entries (the array is oversized because it includes watchdog entries in the count)

Quoted 5 lines

In both cases taking the
min(cnt, gtdt->platform_timer_count);
should work AFAICS

It was probably worth noting in the changelog that I did this to gracefully handle the reverse of this issue where we could dereference platform timer entries that are within the bounds of the GTDT but exceed gtdt->platform_timer_count.

Quoted 2 lines

(hard to grok though, we - as in ACPI maintainers - need to clean this up).

Heh, thought this smelled a little ripe ;-) Went for the minimal fix first.

Signature · 2 lines

Thanks,
Oliver

Lorenzo Pieralisi

maintainer

2025-01-30 17:37 UTC #4 lore raw

Quoted 25 lines · Oliver Upton

Hi Lorenzo,
Quoted 15 lines · Lorenzo Pieralisi
drivers/acpi/arm64/gtdt.c int __init acpi_gtdt_init(struct acpi_table_header *table, · line 191
Quoted 5 lines
drivers/acpi/arm64/gtdt.c +1 -0
@@ -188,13 +188,17 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
 		cnt++;
 
 	if (cnt != gtdt->platform_timer_count) {
+		cnt = min(cnt, gtdt->platform_timer_count);
Thank you for reporting this.
There is something I need to understand.
What's wrong cnt (because platform_timer_valid() fails for some reason on some entries whereas before the commit we are fixing was applied we *were* parsing those entries) or gtdt->platform_timer_count ?
Quoted 6 lines
I *guess* the issue is the following:
gtdt->platform_timer_count reports the number of GT blocks in the GTDT not including Arm generic watchdogs, whereas cnt counts both structure types (and that's what gtdt->platform_timer_count should report too if it was correct).

Quoted 4 lines

I've seen two different issues so far:
- In one case, the offset of the platform timer array is entirely beyond the GTDT

And we were parsing it before the commit you are fixing was applied because we weren't validating the first entry ?

That's how the loop was working before:

#define for_each_platform_timer(_g)
	for (_g = acpi_gtdt_desc.platform_timer; _g;
		_g = next_platform_timer(_g))

We were parsing the first entry and now we don't anymore (rightly so), even with this fix applied, correct ?

Quoted 2 lines

- In another, the GTDT has a timer array of length 2, but only the first structure falls within the length of the overall GTDT

Right.

Quoted 2 lines

Since cnt is the result of doing a bounds-checked walk of the platform timer array, both of these issues cause the sanity check to fail.

Yep.

Quoted 16 lines

Quoted 16 lines
drivers/acpi/arm64/gtdt.c int __init acpi_gtdt_init(struct acpi_table_header *table, · line 201
Quoted 3 lines
drivers/acpi/arm64/gtdt.c +1 -1
 	if (platform_timer_count)
-		*platform_timer_count = gtdt->platform_timer_count;
+		*platform_timer_count = cnt;
I think this should be fine as things stand (but see above).
It is used in:
gtdt_sbsa_gwdt_init() - just to check if there are platform timers entries
arch_timer_mem_acpi_init() - to create a temporary array to init arch mem timer
entries (the array is oversized because it includes watchdog entries in the count)
In both cases taking the

Quoted 8 lines

Quoted 3 lines
min(cnt, gtdt->platform_timer_count);
should work AFAICS
It was probably worth noting in the changelog that I did this to gracefully handle the reverse of this issue where we could dereference platform timer entries that are within the bounds of the GTDT but exceed gtdt->platform_timer_count.

Yes, that's strike three, where platform_timer_count is borked, I understand you have not hit this one though but it seems right to fix it the way you did.

I am just trying to understand given that we are sending the fix to stable whether this fix can affect other broken GTDTs (or better, whether other borked FW can affect this change) but I hope not, it should not.

Quoted 5 lines

Quoted 2 lines
(hard to grok though, we - as in ACPI maintainers - need to clean this up).
Heh, thought this smelled a little ripe ;-) Went for the minimal fix first.

I am sorry you have to deal with this, the kernel is not there to validate the firmware but on the other hand when it doesn't that's the outcome. Apologies.

Lorenzo

Lorenzo Pieralisi

maintainer

2025-02-12 10:33 UTC #5 lore raw

In reply to Oliver Upton

Quoted 52 lines · Oliver Upton

Hi Lorenzo,
Quoted 15 lines · Lorenzo Pieralisi
drivers/acpi/arm64/gtdt.c int __init acpi_gtdt_init(struct acpi_table_header *table, · line 191
Quoted 5 lines
drivers/acpi/arm64/gtdt.c +1 -0
@@ -188,13 +188,17 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
 		cnt++;
 
 	if (cnt != gtdt->platform_timer_count) {
+		cnt = min(cnt, gtdt->platform_timer_count);
Thank you for reporting this.
There is something I need to understand.
What's wrong cnt (because platform_timer_valid() fails for some reason on some entries whereas before the commit we are fixing was applied we *were* parsing those entries) or gtdt->platform_timer_count ?
Quoted 6 lines
I *guess* the issue is the following:
gtdt->platform_timer_count reports the number of GT blocks in the GTDT not including Arm generic watchdogs, whereas cnt counts both structure types (and that's what gtdt->platform_timer_count should report too if it was correct).
I've seen two different issues so far:
- In one case, the offset of the platform timer array is entirely beyond the GTDT
- In another, the GTDT has a timer array of length 2, but only the first structure falls within the length of the overall GTDT
Since cnt is the result of doing a bounds-checked walk of the platform timer array, both of these issues cause the sanity check to fail.
Quoted 16 lines
drivers/acpi/arm64/gtdt.c int __init acpi_gtdt_init(struct acpi_table_header *table, · line 201
Quoted 3 lines
drivers/acpi/arm64/gtdt.c +1 -1
 	if (platform_timer_count)
-		*platform_timer_count = gtdt->platform_timer_count;
+		*platform_timer_count = cnt;
I think this should be fine as things stand (but see above).
It is used in:
gtdt_sbsa_gwdt_init() - just to check if there are platform timers entries
arch_timer_mem_acpi_init() - to create a temporary array to init arch mem timer
entries (the array is oversized because it includes watchdog entries in the count)
In both cases taking the

Quoted 8 lines

Quoted 3 lines
min(cnt, gtdt->platform_timer_count);
should work AFAICS
It was probably worth noting in the changelog that I did this to gracefully handle the reverse of this issue where we could dereference platform timer entries that are within the bounds of the GTDT but exceed gtdt->platform_timer_count.

Hi Oliver,

I was about to ask Catalin/Will to pick this up, don't know if you have time to update the changelog and send a v2 - a Link: to this thread will be added anyway.

Thanks,
Lorenzo

Will Deacon

maintainer

2025-02-13 11:45 UTC #6 lore raw

Quoted 21 lines · Lorenzo Pieralisi

Quoted 12 lines
Quoted 12 lines
I think this should be fine as things stand (but see above).
It is used in:
gtdt_sbsa_gwdt_init() - just to check if there are platform timers entries
arch_timer_mem_acpi_init() - to create a temporary array to init arch mem timer
entries (the array is oversized because it includes watchdog entries in the count)
In both cases taking the
Quoted 8 lines
Quoted 3 lines
min(cnt, gtdt->platform_timer_count);
should work AFAICS
It was probably worth noting in the changelog that I did this to gracefully handle the reverse of this issue where we could dereference platform timer entries that are within the bounds of the GTDT but exceed gtdt->platform_timer_count.

Quoted 5 lines

Hi Oliver,
I was about to ask Catalin/Will to pick this up, don't know if you have time to update the changelog and send a v2 - a Link: to this thread will be added anyway.

I'll pick it up with the Link: tag, no need to resend.

Thanks, both!

Will

Will Deacon

maintainer Applied

2025-02-13 13:59 UTC #7 lore raw

In reply to Oliver Upton

Quoted 9 lines · Oliver Upton

Perhaps unsurprisingly there are some platforms where the GTDT isn't quite right and the Platforms Timer array overflows the length of the overall table.
While the recently-added sanity checking isn't wrong, it makes it impossible to boot the kernel on offending platforms. Try to hobble along and limit the Platform Timer count to the bounds of the table.
[...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] ACPI: GTDT: Relax sanity checking on Platform Timers array count

      https://git.kernel.org/arm64/c/f818227a2f3d

Cheers,

Signature · 5 lines

Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev