Re: [PATCH] ACPI: PPTT: Fix processor subtable walk

[Yicong Yang <yangyicong@huawei.com>]

On 2025/5/8 10:30, Jeremy Linton wrote:
> The original PPTT code had a bug where the processor subtable length
> was not correctly validated when encountering a truncated
> acpi_pptt_processor node.
> 
> Commit 7ab4f0e37a0f4 ("ACPI PPTT: Fix coding mistakes in a couple of
> sizeof() calls") attempted to fix this by validating the size is as
> large as the acpi_pptt_processor node structure. This introduced a
> regression where the last processor node in the PPTT table is ignored
> if it doesn't contain any private resources. That results errors like:
> 
>   ACPI PPTT: PPTT table found, but unable to locate core XX (XX)
>   ACPI: SPE must be homogeneous
> 
> Furthermore, it fail in a common case where the node length isn't
> equal to the acpi_pptt_processor structure size, leaving the original
> bug in a modified form.
> 
> Correct the regression by adjusting the loop termination conditions as
> suggested by the bug reporters. An additional check performed after
> the subtable node type is detected, validates the acpi_pptt_processor
> node is fully contained in the PPTT table. Repeating the check in
> acpi_pptt_leaf_node() is largely redundant as the node is already
> known to be fully contained in the table.
> 
> The case where a final truncated node's parent property is accepted,
> but the node itself is rejected should not be considered a bug.
> 
> Fixes: 7ab4f0e37a0f4 ("ACPI PPTT: Fix coding mistakes in a couple of sizeof() calls")
> Reported-by: Maximilian Heyne <mheyne@amazon.de>
> Closes: https://lore.kernel.org/linux-acpi/20250506-draco-taped-15f475cd@mheyne-amazon/
> Reported-by: Yicong Yang <yangyicong@hisilicon.com>

Thanks for the fix. The last CPU in the PPTT can be parsed on my board with this.

Tested-by: Yicong Yang <yangyicong@hisilicon.com>

> Closes: https://lore.kernel.org/linux-acpi/20250507035124.28071-1-yangyicong@huawei.com/
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Cc: Jean-Marc Eurin <jmeurin@google.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/acpi/pptt.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index f73ce6e13065..54676e3d82dd 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -231,16 +231,18 @@ static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
>  			     sizeof(struct acpi_table_pptt));
>  	proc_sz = sizeof(struct acpi_pptt_processor);
>  
> -	while ((unsigned long)entry + proc_sz < table_end) {
> +	/* ignore subtable types that are smaller than a processor node */
> +	while ((unsigned long)entry + proc_sz <= table_end) {
>  		cpu_node = (struct acpi_pptt_processor *)entry;
> +
>  		if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
>  		    cpu_node->parent == node_entry)
>  			return 0;
>  		if (entry->length == 0)
>  			return 0;
> +
>  		entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
>  				     entry->length);
> -
>  	}
>  	return 1;
>  }
> @@ -273,15 +275,18 @@ static struct acpi_pptt_processor *acpi_find_processor_node(struct acpi_table_he
>  	proc_sz = sizeof(struct acpi_pptt_processor);
>  
>  	/* find the processor structure associated with this cpuid */
> -	while ((unsigned long)entry + proc_sz < table_end) {
> +	while ((unsigned long)entry + proc_sz <= table_end) {
>  		cpu_node = (struct acpi_pptt_processor *)entry;
>  
>  		if (entry->length == 0) {
>  			pr_warn("Invalid zero length subtable\n");
>  			break;
>  		}
> +		/* entry->length may not equal proc_sz, revalidate the processor structure length */
>  		if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
>  		    acpi_cpu_id == cpu_node->acpi_processor_id &&
> +		    (unsigned long)entry + entry->length <= table_end &&
> +		    entry->length == proc_sz + cpu_node->number_of_priv_resources * sizeof(u32) &&
>  		     acpi_pptt_leaf_node(table_hdr, cpu_node)) {
>  			return (struct acpi_pptt_processor *)entry;
>  		}
>