Re: [PATCH v3 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:

Pre-existing issues:
- [High] Missing minimum length validation for platform timer subtables can lead to out-of-bounds memory accesses when parsing malformed ACPI tables.
- [High] The patch adds size validation for GTDT revisions 2 and 3, but misses length validation for GTDT revision 1 tables.
--

commit 6795242683ef78c0e32a77285ea86c559ab6c178
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 support the expanded size of
GTDT revision 3, which includes an EL2-private virtual timer. It adjusts
the platform timer start offset based on the revision and adds a length
validation check for the base table.

> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
> --- a/drivers/acpi/arm64/gtdt.c
> +++ b/drivers/acpi/arm64/gtdt.c
> @@ -34,14 +34,25 @@ struct acpi_gtdt_descriptor {

[ ... ]

>  static __init bool platform_timer_valid(void *platform_timer)
>  {
>  	struct acpi_gtdt_header *gh = platform_timer;
> +	void *platform_timer_begin;
>  
> -	return (platform_timer >= (void *)(acpi_gtdt_desc.gtdt + 1) &&
> -		platform_timer < acpi_gtdt_desc.gtdt_end &&
> +	if (acpi_gtdt_desc.gtdt->header.revision >= 3)
> +		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 >= platform_timer_begin &&
> +		platform_timer + sizeof(*gh) <= acpi_gtdt_desc.gtdt_end &&
>  		gh->length != 0 &&
>  		platform_timer + gh->length <= acpi_gtdt_desc.gtdt_end);
>  }

[Severity: High]
This is a pre-existing issue, but does this validation fully protect against
malformed ACPI tables?

While it checks that the subtable's declared length fits within the main
GTDT table, it does not verify that the length is large enough to contain
the specific subtable structure being parsed.

If a malformed GTDT table provides a subtable with a truncated length,
parser functions like is_non_secure_watchdog() might read fields past the
end of the provided table data. Could this lead to out-of-bounds memory
accesses when reading fields like wd->timer_flags?

[ ... ]

> @@ -166,6 +177,13 @@ int __init acpi_gtdt_init(struct acpi_table_header *table,
>  	u32 cnt = 0;
>  
>  	gtdt = container_of(table, struct acpi_table_gtdt, header);
> +
> +	if ((gtdt->header.revision >= 3 && gtdt->header.length < sizeof(struct gtdt_v3)) ||
> +	    (gtdt->header.revision == 2 && gtdt->header.length < sizeof(*gtdt))) {
> +		pr_err(FW_BUG "GTDT with invalid size %d\n", gtdt->header.length);
> +		return -EINVAL;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we are adding length
validation for revisions 2 and 3, should we also validate the length for
revision 1 tables?

If a system provides a malformed revision 1 GTDT table that is smaller than
its baseline required size, the initialization could succeed. Later accesses
to fields in acpi_gtdt_map_ppi() might read out-of-bounds memory.

Would it be safer to ensure all supported revisions meet their minimum
required sizes?

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