Re: [PATCH 03/13] perf header: Sanity check HEADER_NRCPUS and HEADER_CPU_DOMAIN_INFO

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Medium] Mismatch in domain array traversal bounds causes a memory leak on malformed domain indices.
- [Medium] Unbounded `nr_cpus_avail` allows massive memory allocations and integer cast sign extension.
- [Medium] Integer overflow in the section size validation check bypasses bounds validation.
- [Low] Format specifier mismatch in error messages prints large values as negative numbers.
--

commit 05b748d77f989fb396557569862fea65e5d2e722
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf header: Sanity check HEADER_NRCPUS and HEADER_CPU_DOMAIN_INFO

This commit adds sanity checks to process_nrcpus and process_cpu_domain_info
to validate CPU counts and prevent duplicate domain allocations.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
[ ... ]
> @@ -2722,6 +2724,13 @@ static int process_nrcpus(struct feat_fd *ff, void *data __maybe_unused)
>  	ret = do_read_u32(ff, &nr_cpus_online);
>  	if (ret)
>  		return ret;
> +
> +	if (nr_cpus_online > nr_cpus_avail) {
> +		pr_err("Invalid HEADER_NRCPUS: nr_cpus_online (%u) > nr_cpus_avail (%u)\n",
> +		       nr_cpus_online, nr_cpus_avail);
> +		return -1;
> +	}
> +

Since there is no absolute upper limit checked for nr_cpus_avail here, could
a malformed perf.data file provide an excessively large value?

For example, if nr_cpus_online is 1 and nr_cpus_avail is 0x40000000,
it seems this check would pass.

Later, env->nr_cpus_avail is cast to int, which would result in a negative
value if nr_cpus_avail exceeds INT_MAX. Additionally, in
process_cpu_domain_info, this could lead to a massive memory allocation
when allocating the cd_map array.

[ ... ]
> @@ -3698,6 +3707,17 @@ static int process_cpu_domain_info(struct feat_fd *ff, void *data __maybe_unused
>  	nra = env->nr_cpus_avail;
>  	nr = env->nr_cpus_online;
>  
> +	if (nra == 0 || nr == 0) {
> +		pr_err("Invalid HEADER_CPU_DOMAIN_INFO: missing HEADER_NRCPUS\n");
> +		return -1;
> +	}
> +
> +	if (ff->size < 2 * sizeof(u32) + nr * 2 * sizeof(u32)) {

Can this size validation be bypassed due to an integer overflow?

Because nr is a 32-bit unsigned integer, if a crafted file provides a very
large value like 0x80000000, nr * 2 could overflow to 0 before the promotion
to size_t. This would evaluate the requirement to just 8 bytes, completely
bypassing the bounds check.

> +		pr_err("Invalid HEADER_CPU_DOMAIN_INFO: section too small (%zu) for %u CPUs\n",
> +		       (size_t)ff->size, nr);
> +		return -1;
> +	}
> +
[ ... ]
> @@ -3714,6 +3734,18 @@ static int process_cpu_domain_info(struct feat_fd *ff, void *data __maybe_unused
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Sanity check: real systems have at most ~10 sched domain levels
> +	 * (SMT, CLS, MC, PKG + NUMA hops). Reject obviously bogus values
> +	 * from malformed perf.data files before they cause excessive
> +	 * allocation in the per-CPU loop.
> +	 */
> +	if (max_sched_domains > MAX_SCHED_DOMAINS) {
> +		pr_err("Invalid HEADER_CPU_DOMAIN_INFO: max_sched_domains %u > %u\n",
> +		       max_sched_domains, MAX_SCHED_DOMAINS);
> +		return -1;
> +	}

This isn't a bug, but some of the error messages in this function (such as
the nearby checks for cpu >= nra and domain >= max_sched_domains) use the
%d format specifier for unsigned 32-bit integers. If a malformed file
provides values >= 0x80000000, would they be logged as confusing negative
numbers? Updating them to %u might improve error logging clarity.

[ ... ]
> @@ -3760,7 +3797,13 @@ static int process_cpu_domain_info(struct feat_fd *ff, void *data __maybe_unused
>  			if (!d_info)
>  				return -1;
>  
> -			assert(cd_map[cpu]->domains[domain] == NULL);
> +			if (cd_map[cpu]->domains[domain]) {
> +				pr_err("Invalid HEADER_CPU_DOMAIN_INFO: duplicate domain %u for cpu %u\n",
> +				       domain, cpu);
> +				free(d_info);
> +				return -1;
> +			}
> +
>  			cd_map[cpu]->domains[domain] = d_info;

If a malformed file specifies a domain index such that
nr_domains <= domain < max_sched_domains, does this create a memory leak?

The array is sized by max_sched_domains, so the allocation succeeds and is
stored safely here. However, if free_cpu_domain_info only loops up to
nr_domains - 1 during teardown, any structures placed at an index >=
nr_domains (including their dynamically allocated strings) might never be
accessed and permanently leaked.

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