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

Pre-existing issues:
- [High] Integer overflow in auxtrace_queues__queue_buffer() leads to out-of-bounds memory access.
- [Medium] Null pointer dereference in cs_etm__dump_event() when processing a crafted perf.data file.
- [Medium] Out-of-bounds read in cs_etm__create_meta_blk() when parsing CPU metadata blocks.
--

commit 6ff616c3e2c2577ec55eda8099842f63dcc43ed7
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.  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.

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

[Severity: Medium]
This is a pre-existing issue, but since we are adding NULL checks for
zero-initialized queues here, could this also cause a null pointer
dereference in cs_etm__dump_event()?

If dump_trace is enabled, cs_etm__process_auxtrace_event() and
dump_queued_data() can pass the new queue's priv pointer directly:

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

And cs_etm__dump_event() unconditionally dereferences it:

tools/perf/util/cs-etm.c:cs_etm__dump_event() {
    ...
    cs_etm_decoder__get_name(etmq->decoder)
    ...
}

Should cs_etm__dump_event() also have a NULL check for etmq?

[ ... ]
> @@ -3144,6 +3162,9 @@ static int cs_etm__queue_aux_fragment(struct perf_session *session, off_t file_o
>  	    aux_offset + aux_size <= auxtrace_event->offset + auxtrace_event->size) {
>  		struct cs_etm_queue *etmq = cs_etm__get_queue(etm, auxtrace_event->cpu);
>  
> +		if (!etmq)
> +			return -EINVAL;
> +

[Severity: High]
This is a pre-existing issue, but I noticed a potential integer overflow in
auxtrace_queues__queue_buffer() that could lead to out-of-bounds memory access.

When processing an untrusted perf.data file, if the idx field of a
PERF_RECORD_AUXTRACE event is UINT_MAX, does the idx + 1 calculation
overflow to 0?

tools/perf/util/auxtrace.c:auxtrace_queues__queue_buffer() {
    ...
	if (idx >= queues->nr_queues) {
		err = auxtrace_queues__grow(queues, idx + 1);
		if (err)
			return err;
	}

	queue = &queues->queue_array[idx];
    ...
}

Since auxtrace_queues__grow() evaluates nr_queues < new_nr_queues as false
when new_nr_queues is 0, it returns without growing the array. Does this
cause the subsequent array access at queues->queue_array[UINT_MAX] to
write out of bounds?

[Severity: Medium]
This is a pre-existing issue, but there appears to be a potential out-of-bounds
read in cs_etm__create_meta_blk() when parsing CPU metadata blocks.

When parsing a PERF_RECORD_AUXTRACE_INFO event, does
cs_etm__process_auxtrace_info_full() validate that the payload contains
enough bytes before calling cs_etm__create_meta_blk()?

tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_info_full() {
    ...
		metadata[j] =
			cs_etm__create_meta_blk(ptr, &i,
						CS_ETMV4_PRIV_MAX,
						CS_ETMV4_NR_TRC_PARAMS_V0);
    ...
}

A crafted file with a large num_cpu and small auxtrace_info.header.size
might bypass the num_cpu > priv_size / 8 check. cs_etm__create_meta_blk()
then unconditionally reads up to CS_ETMV4_NR_TRC_PARAMS_V0 elements:

tools/perf/util/cs-etm.c:cs_etm__create_meta_blk() {
    ...
    metadata[k + 1] = buff_in[i + k];
    ...
}

Since the length validation (i * 8 != priv_size) only happens after the
loop finishes, can this cause a heap buffer over-read of buff_in?

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