Re: [PATCH v2 01/17] ACPI: GTDT: Account for GTDTv3 size when walking the platform timer descriptors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Insufficient bounds checking in `platform_timer_valid()` allows reading past the end of the ACPI table.
- [High] Missing base table length validation in `acpi_gtdt_init()` allows an out-of-bounds read of the GTDT table.
--

commit 4d6d9fd7f79fd90d8c3bbeefd718d1658523dfab
Author: Marc Zyngier <maz@kernel.org>

ACPI: GTDT: Account for GTDTv3 size when walking the platform timer descriptors

This commit updates the GTDT table parsing to correctly account for the expanded
size of the base table when dealing with GTDT revision 3. It adds support for
the additional EL2 virtual timer structure when walking the platform timer
descriptors.

> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
> index ffc867bac2d60..b9d9b8edf2df7 100644
> --- a/drivers/acpi/arm64/gtdt.c
> +++ b/drivers/acpi/arm64/gtdt.c
> @@ -39,8 +45,14 @@ static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata;
>  static __init bool platform_timer_valid(void *platform_timer)
>  {
>  	struct acpi_gtdt_header *gh = platform_timer;
> +	void *platform_timer_begin;
> +
> +	if (acpi_gtdt_desc.v3)
> +		platform_timer_begin = container_of(acpi_gtdt_desc.gtdt, struct gtdt_v3, gtdt_v2) + 1;
> +	else
> +		platform_timer_begin = acpi_gtdt_desc.gtdt + 1;
>  
> -	return (platform_timer >= (void *)(acpi_gtdt_desc.gtdt + 1) &&
> +	return (platform_timer >= platform_timer_begin &&
>  		platform_timer < acpi_gtdt_desc.gtdt_end &&
>  		gh->length != 0 &&

Is there an out-of-bounds read regression here where the code allows reading
past the end of the ACPI table?

The condition platform_timer < acpi_gtdt_desc.gtdt_end only ensures that at
least 1 byte remains within the mapped table limits. If a malformed table
positions platform_timer exactly 1 or 2 bytes before gtdt_end, evaluating
gh->length reads a 16-bit integer at offset 1 from platform_timer.

This causes a read of 1 to 2 bytes past gtdt_end. Should this enforce
platform_timer + sizeof(struct acpi_gtdt_header) <= acpi_gtdt_desc.gtdt_end
before dereferencing gh->length?

>  		platform_timer + gh->length <= acpi_gtdt_desc.gtdt_end);
>  }

[ ... ]

> @@ -169,6 +181,7 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
>  	acpi_gtdt_desc.gtdt = gtdt;
>  	acpi_gtdt_desc.gtdt_end = (void *)table + table->length;
>  	acpi_gtdt_desc.platform_timer = NULL;
> +	acpi_gtdt_desc.v3 = gtdt->header.revision >= 3 && gtdt->header.length >= sizeof(struct gtdt_v3);

Could the lack of base table length validation lead to an out-of-bounds read
regression?

While this patch adds a size check for V3 tables, it doesn't appear to verify
that the base V2 table meets the minimum length requirement
(table->length >= sizeof(struct acpi_table_gtdt)).

If firmware provides a truncated GTDT table containing only the standard
36-byte ACPI header, accessing gtdt->platform_timer_count later in this
function reads out-of-bounds memory.

>  	if (platform_timer_count)
>  		*platform_timer_count = 0;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514150945.3917510-1-maz@kernel.org?part=1