Re: [PATCH] ACPI: GTDT: Relax sanity checking on Platform Timers array count

Lorenzo Pieralisi
2025-01-28T10:51:00Z

On Tue, Jan 28, 2025 at 12:17:49AM +0000, Oliver Upton wrote:
> Perhaps unsurprisingly there are some platforms where the GTDT isn't

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

An accident waiting to happen :)

> 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 <maz@kernel.org>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Zheng Zengkai <zhengzengkai@huawei.com>
> Cc: stable@vger.kernel.org
> Fixes: 263e22d6bd1f ("ACPI: GTDT: Tighten the check for the array of platform timer structures")
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
> drivers/acpi/arm64/gtdt.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
> index 3561553eff8b..70f8290b659d 100644
> --- a/drivers/acpi/arm64/gtdt.c
> +++ b/drivers/acpi/arm64/gtdt.c
> @@ -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);

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.

> + 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 <lpieralisi@kernel.org>

>
> return 0;
> }
>
> base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
> --
> 2.48.1.262.g85cc9f2d1e-goog
>