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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Tue, Jan 28, 2025 at 12:42:24PM -0800, Oliver Upton wrote:
> 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.

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