Re: [PATCH 4/9] perf cs-etm: Require full global header in auxtrace_info size check

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Insufficient bounds checking for per-CPU metadata allows out-of-bounds reads when parsing and printing crafted auxtrace_info events.
--

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

perf cs-etm: Require full global header in auxtrace_info size check

cs_etm__process_auxtrace_info() checks that header.size covers
event_header_size + INFO_HEADER_SIZE (16 bytes total), but then
accesses ptr[CS_PMU_TYPE_CPUS] at offset 24 from the start of the
event.  A crafted 16-byte auxtrace_info event passes the size check
but reads out-of-bounds. Include CS_ETM_HEADER_SIZE in the minimum
size check so that the global header entries are guaranteed to fit.

> diff --git a/tools/perf/util/cs-etm-base.c b/tools/perf/util/cs-etm-base.c
> index 4abe416e3febd..aebef71d3a0a1 100644
> --- a/tools/perf/util/cs-etm-base.c
> +++ b/tools/perf/util/cs-etm-base.c
> @@ -170,7 +170,9 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
>  	u64 *ptr = NULL;
>  	u64 hdr_version;
>  
> -	if (auxtrace_info->header.size < (event_header_size + INFO_HEADER_SIZE))
> +	/* Ensure priv[] is large enough for the global header entries */
> +	if (auxtrace_info->header.size < (event_header_size + INFO_HEADER_SIZE +
> +					  CS_ETM_HEADER_SIZE))
>  		return -EINVAL;
>  
>  	/* First the global part */

[Severity: High]
This is a pre-existing issue, but I have a question about the subsequent
parsing of the event payload in cs_etm__process_auxtrace_info().

When dump_trace is enabled, it extracts num_cpu from the payload and passes
it to cs_etm__print_auxtrace_info():

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);
	}
    ...
}

Does cs_etm__print_auxtrace_info() or its children validate that the event
is actually large enough to contain this many CPUs?

It looks like cs_etm__print_auxtrace_info() iterates num_cpu times, calling
cs_etm__print_cpu_metadata_v1(), which accesses array elements directly:

tools/perf/util/cs-etm-base.c:cs_etm__print_cpu_metadata_v1() {
    ...
	magic = val[i + CS_ETM_MAGIC];
	/* total params to print is NR_PARAMS + common block size for v1 */
	total_params = val[i + CS_ETM_NR_TRC_PARAMS] + CS_ETM_COMMON_BLK_MAX_V1;
    ...
}

Since i is advanced based on the payload-provided parameter count, could
these operations read out-of-bounds if a crafted auxtrace_info event has a
large num_cpu but a short payload?

Additionally, a similar bounds issue seems to exist in the non-dump_trace path
in cs_etm__process_auxtrace_info_full(). While it verifies there is at least
one u64 per CPU, the loop processing each CPU reads further into the buffer:

tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_info_full() {
    ...
	for (int j = 0; j < num_cpu; j++) {
		if (ptr[i] == __perf_cs_etmv3_magic) {
    ...
			err = cs_etm__create_meta_blk(ptr, &i,
    ...
}

Where cs_etm__create_meta_blk() accesses multiple elements without checking
against the total event size:

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

The strict event size check (i * 8 != priv_size) in
cs_etm__process_auxtrace_info_full() only occurs after the loop finishes.

Would it be safer to check the current offset i and the required size against
auxtrace_info->header.size before reading elements from ptr[] or buff_in[]?

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