Re: [RFC 06/12] ACPI: TLBI: Parse domains from table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] acpi: tlbi: missing bounds check and integer overflow in structure getters
- [Medium] acpi: tlbi: memory leak of domains array on initialization error paths
- [Low] acpi: tlbi: dangling global pointer after ACPI table is unmapped
--

commit cbbbb2dad21c37d8cb180622bf2879e0d09c3f0d
Author: Kristina Martšenko <kristina.martsenko@arm.com>

ACPI: TLBI: Parse domains from table

This commit parses the Arm TLBI ACPI table to extract a hierarchy of TLBI
domains, identifying member CPUs and subdomains. It builds and validates
cpumasks for each domain so they can be used for domain management in
arm64 code.

> diff --git a/drivers/acpi/arm64/tlbi.c b/drivers/acpi/arm64/tlbi.c
> new file mode 100644
> index 0000000000000..b705a5b198426
> --- /dev/null
> +++ b/drivers/acpi/arm64/tlbi.c
> @@ -0,0 +1,188 @@

[ ... ]

> +struct acpi_table_tlbi *tlbi;
> +static struct tlbi_domain *domains;
> +static int domain_count;

[ ... ]

> +static struct acpi_tlbi_descendant *__init tlbi_get_descendant(u32 offset)
> +{
> +	struct acpi_tlbi_descendant *descendant;
> +
> +	descendant = ACPI_ADD_PTR(struct acpi_tlbi_descendant, tlbi, offset);
> +	if (descendant->length < sizeof(*descendant) ||
> +	    offset + descendant->length > tlbi->header.length)

[Severity: High]
Does this code safely access the descendant structure? If the offset
parameter provided by the ACPI table is larger than the table size,
the computed descendant pointer will point out of bounds. Reading
descendant->length before validating the offset will cause an
out-of-bounds memory read.

Additionally, since offset and descendant->length are 32-bit integers,
could the addition offset + descendant->length overflow? If a large
length is provided, the overflow could bypass this bounds check and
allow traversal into out-of-bounds memory.

> +		return NULL;
> +
> +	return descendant;
> +}
> +
> +static struct acpi_tlbi_domain *__init tlbi_get_domain(u32 offset)
> +{
> +	struct acpi_tlbi_domain *domain;
> +
> +	domain = ACPI_ADD_PTR(struct acpi_tlbi_domain, tlbi, offset);
> +	if (domain->length < sizeof(*domain) ||
> +	    offset + domain->length > tlbi->header.length)

[Severity: High]
Similarly to tlbi_get_descendant(), does this safely read domain->length?
Reading from the computed pointer before verifying the offset could cause
an out-of-bounds read, and the 32-bit addition here is also vulnerable to
integer overflow.

> +		return NULL;
> +
> +	return domain;
> +}

[ ... ]

> +static int __init parse_tlbi_table(void)
> +{
> +	struct acpi_table_header *table __free(acpi_put_table) =
> +		acpi_get_table_pointer(ACPI_SIG_TLBI, 0);
> +	u32 offset;
> +	int ret;
> +
> +	if (IS_ERR(table))
> +		return PTR_ERR(table);
> +
> +	tlbi = (struct acpi_table_tlbi *)table;

[Severity: Low]
Does this leave the global tlbi pointer dangling after the function
returns? The table variable uses the __free(acpi_put_table) cleanup
helper, which will unmap the ACPI table memory at the end of the scope.
However, the global tlbi pointer is assigned to this memory and never
cleared.

> +	if (tlbi->header.length < sizeof(*tlbi) ||
> +	    tlbi->domain_count > MAX_TLBI_DOMAINS)
> +		return -EINVAL;
> +	if (!tlbi->domain_count)
> +		return -ENOENT;
> +
> +	domain_count = tlbi->domain_count;
> +	domains = kcalloc(domain_count, sizeof(*domains), GFP_KERNEL);
> +	if (!domains)
> +		return -ENOMEM;
> +
> +	offset = tlbi->domains_offset;
> +
> +	for (int i = 0; i < domain_count; i++) {
> +		struct acpi_tlbi_domain *domain = tlbi_get_domain(offset);
> +
> +		if (!domain || domain->id > U16_MAX)
> +			return -EINVAL;

[Severity: Medium]
Do these early error paths leak the domains array and the already
allocated cpumasks? Once domains is allocated via kcalloc(), returning
directly without a cleanup path leaves the memory permanently leaked.

> +
> +		domains[i].id = domain->id;
> +		if (!zalloc_cpumask_var(&domains[i].cpus, GFP_KERNEL))
> +			return -ENOMEM;
> +
> +		ret = parse_tlbi_domain(domain, domains[i].cpus, 0);
> +		if (ret)
> +			return ret;
> +
> +		offset += domain->length;
> +	}
> +
> +	ret = validate_tlbi_domains();
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001110655.461473-1-kristina.martsenko@arm.com?part=6