Re: [PATCH 15/15] perf cs-etm: Reject CPU IDs that would overflow signed comparison

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The new bounds check allows `INT_MAX`, resulting in undefined behavior via signed integer overflow when calculating `max_cpu + 1`.

Pre-existing issues:
- [Critical] The calculation `sizeof(*metadata) * num_cpu` wraps around on 32-bit architectures, leading to a zero-sized allocation and massive out-of-bounds heap write.
- [High] Global metadata attributes are accessed from the event payload before verifying that the event buffer is large enough.
- [High] Unvalidated `sample.cpu` index leads to an out-of-bounds array access in `cs_etm__get_queue()` when triggered by `PERF_RECORD_AUX_OUTPUT_HW_ID` events.
--

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

perf cs-etm: Reject CPU IDs that would overflow signed comparison

This commit fixes an out-of-bounds access in
cs_etm__process_auxtrace_info_full() caused by a cast from u64 to int. It
adds a validation check to ensure the CPU ID fits within an int before
performing a signed comparison against max_cpu.

Cc: James Clark <james.clark@arm.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/finding/2

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 5e92359f51a7c..d8876b8362056 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -6,6 +6,7 @@
>   * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
>   */
>  
> +#include <limits.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/coresight-pmu.h>

[ ... ]

> @@ -3468,7 +3469,13 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>  			goto err_free_metadata;
>  		}
>  
> -		if ((int) metadata[j][CS_ETM_CPU] > max_cpu)
> +		/* CPU id comes from perf.data and must be a valid index */
> +		if (metadata[j][CS_ETM_CPU] > INT_MAX) {
> +			err = -EINVAL;
> +			goto err_free_metadata;
> +		}

[Severity: Medium]
Does this upper bound check still allow max_cpu to become INT_MAX?

If the trace provides exactly INT_MAX, max_cpu is set to INT_MAX. Later in
cs_etm__process_auxtrace_info_full(), it calls:

    err = auxtrace_queues__init_nr(&etm->queues, max_cpu + 1);

Will computing INT_MAX + 1 cause a signed integer overflow?

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential calculation overflow
earlier in cs_etm__process_auxtrace_info_full():

    num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff;
    metadata = zalloc(sizeof(*metadata) * num_cpu);

On 32-bit platforms, could a crafted num_cpu (e.g., 1073741825) cause the
multiplication to wrap around to 4? If zalloc() succeeds with a tiny buffer,
wouldn't the subsequent loop:

    for (int j = 0; j < num_cpu; j++) {
        ...
        metadata[j] = cs_etm__create_meta_blk(...);

continuously write out-of-bounds into the undersized allocation?

[Severity: High]
This is a pre-existing issue, but is it possible to read past the end of the
event payload before its size is fully validated?

In cs_etm__process_auxtrace_info_full():

    ptr = (u64 *) auxtrace_info->priv;
    num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff;

Since the initial perf_record_auxtrace_info size check only guarantees
16 bytes, and priv[] starts at offset 16, accessing ptr[CS_PMU_TYPE_CPUS]
reads from offset 24.

Could this access unmapped memory if the event is truncated?

[Severity: High]
This is a pre-existing issue, but is there an unvalidated index used for
queue array lookups?

When processing PERF_RECORD_AUX_OUTPUT_HW_ID events, sample.cpu is parsed
directly from the payload. It gets passed down the call chain:

cs_etm__process_aux_output_hw_id()
  cs_etm__process_trace_id_v0_1()
    cs_etm__get_queue()

Inside cs_etm__get_queue():

    return etm->queues.queue_array[cpu].priv;

Since cpu comes directly from the trace data, could an arbitrarily large or
negative value cause an out-of-bounds read if it exceeds nr_queues?

> +
> +		if ((int)metadata[j][CS_ETM_CPU] > max_cpu)
>  			max_cpu = metadata[j][CS_ETM_CPU];
>  	}

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