Re: [PATCH 13/13] perf cs-etm: Reject CPU IDs that would overflow signed comparison

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer overflow when allocating the `metadata` array using an unvalidated user-controlled `num_cpu`, leading to a severe heap buffer overflow and arbitrary free on 32-bit systems.
--

commit 485dac05154448b657087d80bf132fafa9c32fbd
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf cs-etm: Reject CPU IDs that would overflow signed comparison

This commit validates that the CPU ID from perf.data fits within an int
before performing a signed comparison with max_cpu. This prevents an integer
overflow that could lead to an out-of-bounds access when indexing the
queue_array.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 5e92359f51a7c..0927b0b9c06b1 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> @@ -3468,7 +3469,13 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>  			goto err_free_metadata;
>  		}
>  
> -		if ((int) metadata[j][CS_ETM_CPU] > max_cpu)
> +		/* CPU id comes from perf.data and must fit max_cpu + 1 without overflow */
> +		if (metadata[j][CS_ETM_CPU] >= INT_MAX) {
> +			err = -EINVAL;
> +			goto err_free_metadata;
> +		}
> +

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but since we are looking at error paths and validation in this function, can
the allocation of the metadata array overflow on 32-bit systems?

Earlier in cs_etm__process_auxtrace_info_full(), num_cpu is extracted from
perf.data:

	num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff;
	metadata = zalloc(sizeof(*metadata) * num_cpu);

If a large value like 0x40000001 is provided on a 32-bit architecture, does
sizeof(*metadata) * num_cpu wrap around to a small allocation size?

If metadata is undersized, wouldn't the subsequent parsing loop write out of
bounds:

	for (int j = 0; j < num_cpu; j++) {
		if (ptr[i] == __perf_cs_etmv3_magic) {
			metadata[j] = ...

Additionally, if we take an error path (like the one added in this patch)
and jump to err_free_metadata:

err_free_metadata:
	for (int j = 0; j < num_cpu; j++)
		zfree(&metadata[j]);

does this read out-of-bounds pointers from the undersized metadata array
and pass them to free()?

> +		if ((int)metadata[j][CS_ETM_CPU] > max_cpu)
>  			max_cpu = metadata[j][CS_ETM_CPU];
>  	}
>

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