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

Pre-existing issues:
- [High] Integer overflow in auxtrace queue growth logic allows massive out-of-bounds heap corruption.
- [High] Implicit signedness cast of `event->auxtrace.idx` causes a negative index out-of-bounds access.
- [Medium] cs_etm__dump_event() lacks a NULL check for `etmq`, leading to a NULL pointer dereference when processing malformed PERF_RECORD_AUXTRACE events with the dump_trace (-D) option.
- [Medium] The `auxtrace_queues__grow` function leaks the old `queues->queue_array` memory when growing the array.
- [Medium] The error path in `cs_etm__process_auxtrace_info_full` permanently leaks all allocated `etmq` structures.
--

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

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

This commit adds a bounds check for the cpu parameter in cs_etm__get_queue()
to prevent out-of-bounds reads when processing untrusted trace payloads.
It also adds NULL checks for the returned etmq pointer in callers to handle
uninitialized entries correctly.

> 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

[ ... ]

[Severity: High]
This is a pre-existing issue, but does assigning the untrusted u32 index to a
signed int lead to an out-of-bounds access?

In tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_event():
    int idx = event->auxtrace.idx;
    ...
    cs_etm__dump_event(etm->queues.queue_array[idx].priv, buffer);

If event->auxtrace.idx is 0xffffffff, it casts to -1. If dump_trace is
enabled, this uses a negative idx as an array index, reading an out-of-bounds
pointer from the heap before passing it to cs_etm__dump_event().


[Severity: High]
This is a pre-existing issue, but is there an integer overflow in the auxtrace
queue growth logic that could lead to out-of-bounds heap corruption?

In tools/perf/util/auxtrace.c:auxtrace_queues__queue_buffer():
    queue = &queues->queue_array[idx];
    ...
    list_add_tail(&buffer->list, &queue->head);

If idx is read as 0xffffffff from the untrusted perf.data,
auxtrace_queues__grow(queues, idx + 1) is called. The value idx + 1 overflows
to 0. Inside auxtrace_queues__grow():
    if (nr_queues < queues->nr_queues || nr_queues < new_nr_queues)
        return -EINVAL;

This evaluates to false, the array remains at its original size, and
success is returned. Execution continues with
queue = &queues->queue_array[0xffffffff], which writes linked-list pointers
massively out of bounds.


[Severity: Medium]
This is a pre-existing issue, but does cs_etm__dump_event() need a NULL check
for etmq?

In tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_event():
    cs_etm__dump_event(etm->queues.queue_array[idx].priv, buffer);

If idx is larger than the currently allocated queues, auxtrace_queues__grow()
dynamically expands the array, leaving the new .priv pointers
zero-initialized (NULL). Because cs_etm__dump_event() unconditionally
dereferences etmq->decoder, could a malformed PERF_RECORD_AUXTRACE event
with the dump_trace (-D) option cause a segmentation fault?


[Severity: Medium]
This is a pre-existing issue, but does auxtrace_queues__grow() leak the old
queues->queue_array memory?

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

When the array is expanded, a new array is allocated and the old state is
copied, but it doesn't appear that free() is ever called on the old
queues->queue_array pointer.


[Severity: Medium]
This is a pre-existing issue, but does the error path in
cs_etm__process_auxtrace_info_full() permanently leak all allocated etmq
structures?

In tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_info_full():
err_free_queues:
    auxtrace_queues__free(&etm->queues);
    session->auxtrace = NULL;

If a setup step fails, auxtrace_queues__free() is called, which only frees
the queue array and buffers, but not the decoder-specific .priv pointers.
Because session->auxtrace is then set to NULL, the generic session teardown
will also fail to call cs_etm__free_events(), leaking the etmq structures
and their internal red-black trees.

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