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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Insufficient bounds checking in the per-CPU metadata parsing loop allows arbitrary out-of-bounds reads based on untrusted event data.
- [Medium] Memory allocated for `etm->queues` array elements (`etmq`) is leaked when initialization fails on the error path.
--

commit 3997f8cbed57aaf438aeb6594523932b36c0d9d5
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf cs-etm: Validate num_cpu before metadata allocation

This commit computes priv_size early to bound num_cpu against the private
data area size. This prevents a 32-bit multiplication overflow and
subsequent out-of-bounds writes when allocating the metadata 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: High]
This is a pre-existing issue, but does the new validation fully prevent
out-of-bounds reads during metadata parsing?

While the check ensures a minimum of one 8-byte entry per CPU, it doesn't
protect the inner parsing loop. When cs_etm__create_meta_blk() is called,
it reads nr_in_params directly from the untrusted perf.data file payload:

tools/perf/util/cs-etm.c:cs_etm__create_meta_blk()
    ...
    nr_in_params = buff_in[i + CS_ETM_NR_TRC_PARAMS];
    ...
    for (k = 0; k < nr_in_params; k++)
        metadata[k] = buff_in[i + k];
    ...

Could a maliciously crafted perf.data file provide an arbitrarily large
nr_in_params, causing an out-of-bounds read before the index i is finally
validated against priv_size at the end of the function?

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

[Severity: Medium]
This is a pre-existing issue, but does the error handling in this function
leak memory?

During initialization, cs_etm__setup_queue() allocates a struct
cs_etm_queue and assigns it to the .priv pointer. If initialization fails
later, the code branches here:

tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_info_full()
    ...
err_free_queues:
    auxtrace_queues__free(&etm->queues);

Since auxtrace_queues__free() is a generic helper that doesn't clean up
subsystem-specific .priv contexts, will these dynamically allocated
queue structures be leaked when the error path is taken?

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