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] Insufficient bounds checking during per-CPU metadata parsing allows out-of-bounds reads and potential info leaks.
--

commit 473102d55bf7bd2e3011265b42ca3a3d07213392
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. On 32-bit,
this can overflow the multiplication and cause a zero-sized allocation.
This patch computes priv_size early and uses it to bound num_cpu to prevent
out-of-bounds writes.

> 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;
> +
>  	metadata = zalloc(sizeof(*metadata) * num_cpu);
>  	if (!metadata)
>  		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but does the per-CPU metadata
parsing loop still allow out-of-bounds reads when processing crafted
perf.data files?

While this patch bounds num_cpu, the subsequent parsing loop repeatedly
calls cs_etm__create_meta_blk(). This function advances the parse index
based on an untrusted length read directly from the payload:

tools/perf/util/cs-etm.c:cs_etm__create_meta_blk() {
    ...
    } else {
        /* read version 1 info block - input and output nr_params may differ */
        /* version 1 has 3 common params */
        nr_cmn_params = 3;
        nr_in_params = buff_in[i + CS_ETM_NR_TRC_PARAMS];
    ...
    /* adjust in offset by number of in params used */
    i += nr_in_params + nr_cmn_params;
    *buff_in_offset = i;
    return metadata;
}

The updated index is then used in the next loop iteration without being
checked against the bounded priv_size:

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, ...);
        } else if (ptr[i] == __perf_cs_etmv4_magic) {
            ...
        } else {
            ui__error("CS ETM Trace: Unrecognised magic number %#"PRIx64". ...\n",
                      ptr[i]);
            err = -EINVAL;
            goto err_free_metadata;
        }
    ...
}

Could an excessively large or negative value for nr_in_params cause arbitrary
forward or backward out-of-bounds reads when evaluating ptr[i]?

If the out-of-bounds memory does not match a recognized magic number,
ui__error() prints the memory directly to stderr, which could potentially
leak heap memory or trigger a segmentation fault.

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