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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z5i2j9gFB2iyN9g4@lpieralisi>
Date: Tue, 28 Jan 2025 11:50:55 +0100
From: Lorenzo Pieralisi <lpieralisi@...nel.org>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, Hanjun Guo <guohanjun@...wei.com>,
	Sudeep Holla <sudeep.holla@....com>, Will Deacon <will@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Marc Zyngier <maz@...nel.org>,
	Zheng Zengkai <zhengzengkai@...wei.com>, stable@...r.kernel.org
Subject: Re: [PATCH] ACPI: GTDT: Relax sanity checking on Platform Timers
 array count

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@...nel.org>
> Cc: Lorenzo Pieralisi <lpieralisi@...nel.org>
> Cc: Zheng Zengkai <zhengzengkai@...wei.com>
> Cc: stable@...r.kernel.org
> Fixes: 263e22d6bd1f ("ACPI: GTDT: Tighten the check for the array of platform timer structures")
> Signed-off-by: Oliver Upton <oliver.upton@...ux.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@...nel.org>

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