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
