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

Pre-existing issues:
- [High] The patch fixes NULL pointer dereferences for dynamically grown queues in ID mapping functions, but misses identical vulnerabilities in `cs_etm__create_decoders()` and `cs_etm__dump_event()` callers.
- [High] The old `queue_array` is unconditionally leaked when dynamically resizing in `auxtrace_queues__grow()`.
--

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

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

This commit adds bounds checking for the CPU value against nr_queues in
cs_etm__get_queue(). It also introduces NULL checks for the queue's private
data across several functions to safely handle untrusted perf.data events.

>     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.  When processing
>     AUX_OUTPUT_HW_ID events, the cpu value comes from untrusted perf.data
>     trace payload and flows through cs_etm__process_trace_id_v0_1() and
>     cs_etm__queue_aux_fragment() without bounds checking, allowing an
>     out-of-bounds read with a crafted file.
>     
>     Add a bounds check in cs_etm__get_queue() and NULL checks in all
>     callers.
>     
>     Also add NULL checks for queue_array[i].priv in the queue iteration
>     loops in cs_etm__map_trace_id_v0() and cs_etm__process_trace_id_v0_1()
>     — after auxtrace_queues__grow() new entries are zero-initialized so
>     .priv can be NULL.  Add a get_cpu_data() NULL check in
>     cs_etm__process_trace_id_v0_1(), matching the existing check in
>     cs_etm__process_trace_id_v0().

[Severity: High]
This isn't a bug introduced by this patch, but while handling the
zero-initialized entries from auxtrace_queues__grow(), does
auxtrace_queues__grow() itself have a memory leak?

When it reallocates the queue array, the old queues->queue_array pointer
seems to be overwritten without being freed:

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

    return 0;
}

Could a crafted perf.data file providing events with successively increasing
indices trigger repeated reallocations and leak the old arrays each time?

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index d121c8f22028d..5d0664ff73b79 100644
> --- 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;
> +
>  		ret = cs_etm__insert_trace_id_node(etmq, trace_chan_id,
>  						   cpu_metadata);

[Severity: High]
This is a pre-existing issue, but since we are adding NULL checks for etmq
here, are similar checks missing in other functions that iterate over the
queues or process them?

In cs_etm__create_decoders():

tools/perf/util/cs-etm.c:cs_etm__create_decoders() {
    ...
    assert(empty || etmq->format != UNSET);
    if (empty)
        continue;

    ret = cs_etm__create_queue_decoders(etmq);
    ...
}

If auxtrace_queues__add_event() adds buffers to a newly grown queue, empty
would be false, but etmq could still be NULL. Could this lead to a NULL
pointer dereference on etmq->format?

Similarly, in dump_queued_data() and cs_etm__process_auxtrace_event(), the
uninitialized priv pointer is passed directly to cs_etm__dump_event() when
iterating over nr_queues:

tools/perf/util/cs-etm.c:dump_queued_data() {
    ...
    for (i = 0; i < etm->queues.nr_queues; ++i)
        list_for_each_entry(buf, &etm->queues.queue_array[i].head, list)
            if (buf->reference == event->reference)
                cs_etm__dump_event(etm->queues.queue_array[i].priv, buf);
}

Inside cs_etm__dump_event(), etmq->decoder is unconditionally dereferenced:

tools/perf/util/cs-etm.c:cs_etm__dump_event() {
    ...
    color_fprintf(stdout, color,
                 ". ... CoreSight %s Trace data: size %#zx bytes\n",
                 cs_etm_decoder__get_name(etmq->decoder), buffer->size);
    ...
}

Could this cause a crash if dump_trace is active?

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