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:
- [High] The newly added bounds check only guarantees one 64-bit word per CPU, but the subsequent metadata extraction loop consumes variable-length blocks without enforcing bounds, leading to arbitrary out-of-bounds reads.
--

commit 1a99258ab7b1aa2bd3fbb2be661ac8c232d38a5d
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf cs-etm: Validate num_cpu before metadata allocation

This commit addresses a potential integer overflow and out-of-bounds write in
cs_etm__process_auxtrace_info_full(). It fixes the issue by computing priv_size
early and bounding num_cpu so that it cannot exceed the total number of u64
entries in the event's private data area.

> 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 newly added bounds check guarantee
that subsequent accesses in the parsing loop remain within bounds?

The added check only guarantees one 64-bit word per CPU, but each CPU's
metadata block can have a variable length. During the parsing loop:

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) {
            metadata[j] = cs_etm__create_meta_blk(ptr, &i, ...);
    ...
}

The cs_etm__create_meta_blk() function consumes a variable number of elements
without validating bounds against priv_size:

tools/perf/util/cs-etm.c:cs_etm__create_meta_blk() {
    ...
    nr_in_params = buff_in[i + CS_ETM_NR_TRC_PARAMS];
    ...
    *info_read = i + nr_cmn_params + nr_in_params;
    ...
}

Could this allow a crafted perf.data file to set a large nr_in_params,
causing the shared index i to be incremented past the buffer bounds? On the
next iteration of the loop in cs_etm__process_auxtrace_info_full(), ptr[i]
would then be read out-of-bounds.

The final size validation (i * 8 != priv_size) occurs after the parsing loop
completes, which appears too late to prevent these out-of-bounds reads.

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

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