Re: [PATCH 3/9] perf cs-etm: Validate num_cpu before metadata allocation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Insufficient bounds checking during metadata parsing allows out-of-bounds reads, arbitrary index manipulation, and potential memory disclosure inside the `num_cpu` loop.
--

commit 08062652d1b7507748a2450df846239247c38cd7
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf cs-etm: Validate num_cpu before metadata allocation
    
cs_etm__process_auxtrace_info_full() reads num_cpu from untrusted
perf.data and uses it to allocate the metadata pointer array.
This patch adds bounds checking to prevent an integer overflow 
when allocating the metadata pointer array.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 0927b0b9c06b1..d121c8f22028d 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -3431,6 +3431,18 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>  	/* First the global part */
>  	ptr = (u64 *) auxtrace_info->priv;
>  	num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff;
> +
> +	/*
> +	 * Bound num_cpu by the event size: the global header consumes
> +	 * CS_ETM_HEADER_SIZE bytes, and each CPU needs at least one u64
> +	 * metadata entry after that.
> +	 */
> +	priv_size = total_size - event_header_size - INFO_HEADER_SIZE -
> +		    CS_ETM_HEADER_SIZE;
> +	if (num_cpu <= 0 || priv_size <= 0 ||
> +	    num_cpu > priv_size / (int)sizeof(u64))
> +		return -EINVAL;

[Severity: Medium]
This is a pre-existing issue, but does this bounds check fully protect the
metadata parsing loop that follows?

While this check ensures there is at least one 8-byte element per CPU, inside
the parsing loop, cs_etm__create_meta_blk() immediately accesses elements
beyond this guaranteed size:

nr_in_params = buff_in[i + CS_ETM_NR_TRC_PARAMS]

This could trigger an out-of-bounds read if priv_size is small.

Additionally, because nr_in_params is read without dynamic bounds checking, it
can arbitrarily alter the read index i. On the next iteration, the code blindly
reads ptr[i], and on the error path, it could leak memory:

tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_info_full() {
    ...
    for (int j = 0; j < num_cpu; j++) {
        if (ptr[i] == __perf_cs_etmv3_magic) {
            ...
        } else {
            ui__error("CS ETM Trace: Unrecognised magic number %#"PRIx64". ...\n",
                      ptr[i]);
            err = -EINVAL;
            goto err_free_metadata;
        }
    ...
}

The strict bounds check (if (i * 8 != priv_size)) only occurs after the
loop completes.

> +
>  	metadata = zalloc(sizeof(*metadata) * num_cpu);
>  	if (!metadata)
>  		return -ENOMEM;

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