Re: [PATCH 5/9] perf cs-etm: Bounds-check CPU in cs_etm__get_queue()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing NULL pointer checks for dynamically expanded queue `.priv` fields allows malicious perf.data to crash the perf tool.
- [High] Integer overflow in `idx` validation bypasses queue array growth, leading to a massive out-of-bounds heap write.
- [Medium] Error path in `cs_etm__process_auxtrace_info_full()` leaks dynamically allocated `cs_etm_queue` contexts.
- [Medium] `auxtrace_queues__grow()` permanently leaks the old `queue_array` memory when expanding the queue.
--

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

perf cs-etm: Bounds-check CPU in cs_etm__get_queue()

cs_etm__get_queue() indexes etm->queues.queue_array[cpu] without
validating that cpu is within nr_queues. This patch adds bounds checking
and NULL checks to prevent out-of-bounds reads when processing untrusted
perf.data files.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
[ ... ]
> @@ -318,6 +324,9 @@ static int cs_etm__map_trace_id_v0(struct cs_etm_auxtrace *etm, u8 trace_chan_id
>  		int ret;
>  
>  		etmq = etm->queues.queue_array[i].priv;
> +		if (!etmq)
> +			continue;
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we are adding NULL 
checks for dynamically expanded queue .priv fields here, do we also need 
them in other functions?

auxtrace_queues__add_event() dynamically expands the queue_array if an 
untrusted perf.data file provides a large idx.

The newly allocated queues are zero-initialized, leaving their .priv pointers 
NULL.

A subsequent iteration in cs_etm__create_decoders() checks if the new 
queue is empty. Since a buffer was appended, it is not empty, and the code 
dereferences etmq->format unconditionally.

Additionally, if dump_trace is active, cs_etm__process_auxtrace_event() 
and cs_etm__dump_event() unconditionally dereference the NULL priv pointer.

Does this allow an untrusted file to cause memory corruption or a crash?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the integer overflow in idx 
validation bypass queue array growth, leading to an out-of-bounds heap write?

If untrusted perf.data input provides idx = 0xFFFFFFFF, the condition 
idx >= queues->nr_queues is met, but auxtrace_queues__grow(queues, idx + 1)
wraps idx + 1 to 0.

tools/perf/util/auxtrace.c:auxtrace_queues__grow() {
    ...
    if (nr_queues < queues->nr_queues || nr_queues < new_nr_queues)
        return -EINVAL;
    ...
}

Because new_nr_queues is 0, the validation passes and the function falsely
indicates success.

Then auxtrace_queues__queue_buffer() accesses:

    queue = &queues->queue_array[0xFFFFFFFF]

and modifies memory via queue->set = true and 
list_add_tail(&buffer->list, &queue->head).

Could this cause a massive out-of-bounds heap write?

[Severity: Medium]
This is a pre-existing issue, but does the error path in 
cs_etm__process_auxtrace_info_full() leak dynamically allocated 
cs_etm_queue contexts?

If a subsequent operation fails, the code jumps to the err_free_queues label:

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

auxtrace_queues__free() clears the array without releasing the nested priv 
pointers:

tools/perf/util/auxtrace.c:auxtrace_queues__free() {
    ...
    zfree(&queues->queue_array);
    queues->nr_queues = 0;
    ...
}

Does this leak num_cpu * sizeof(struct cs_etm_queue) bytes on the error path?

[Severity: Medium]
This isn't a bug introduced by this patch, but does auxtrace_queues__grow() 
permanently leak the old queue_array memory when expanding the queue?

tools/perf/util/auxtrace.c:auxtrace_queues__grow() {
    ...
    queues->queue_array = queue_array;
    return 0;
    ...
}

The old queues->queue_array pointer is overwritten with the new one without 
ever being freed via free() or zfree().

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