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: <Z5lBMBY7XoFJmpGM@linux.dev>
Date: Tue, 28 Jan 2025 12:42:24 -0800
From: Oliver Upton <oliver.upton@...ux.dev>
To: Lorenzo Pieralisi <lpieralisi@...nel.org>
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

Hi Lorenzo,

On Tue, Jan 28, 2025 at 11:50:55AM +0100, Lorenzo Pieralisi wrote:
> > @@ -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'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.

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

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.

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

-- 
Thanks,
Oliver