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

Pre-existing issues:
- [Medium] Insufficient bounds check for the CPU metadata parsing loop leading to out-of-bounds memory reads from untrusted data.
- [Medium] Missing bounds validation on `num_cpu` before printing trace metadata causes out-of-bounds memory reads.
- [Medium] Memory leak of `cs_etm_queue` structures and RB trees on initialization error paths.
--

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

perf cs-etm: Validate num_cpu before metadata allocation

This commit bounds num_cpu by checking it against the total available private
data size to prevent an integer overflow and subsequent out-of-bounds writes
during metadata allocation.

> 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;

[Severity: Medium]
This is a pre-existing issue, but does dump_trace in
cs_etm__process_auxtrace_info() extract and use num_cpu for
cs_etm__print_auxtrace_info() before this new validation occurs?

tools/perf/util/cs-etm-base.c:cs_etm__process_auxtrace_info() {
    ...
    if (dump_trace) {
        num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff;
        cs_etm__print_auxtrace_info(ptr, num_cpu);
    ...

If so, an attacker could still trigger out-of-bounds reads by specifying an
excessively large num_cpu when perf report -D is run.

> +
> +	/*
> +	 * 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 the metadata parsing loop later in this
function lack a bounds check for the buffer size?

While num_cpu is now bounded, the loop iterates j < num_cpu and calls
cs_etm__create_meta_blk() which advances the index i:

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

Can a crafted nr_in_params bypass the new bounds check and cause an
out-of-bounds read on the next iteration?

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

[Severity: Medium]
This is a pre-existing issue, but does the err_free_queues path in this
function leak the queue->priv allocations?

If an error occurs later in cs_etm__process_auxtrace_info_full(), it cleans
up via:

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

auxtrace_queues__free() frees the arrays but doesn't free the dynamically
allocated cs_etm_queue structures stored in queue->priv from
cs_etm__setup_queue(). Is an additional cs_etm__free_events() style cleanup
needed to prevent memory leaks?

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