Re: [PATCH v8 26/29] perf timechart: Bounds check CPU

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] Incomplete bounds checking for CPU indexes (commit message mismatch)
--

commit 25dc3fef25602cbfcf60331e4d743c6fc8cc7612
Author: Ian Rogers <irogers@google.com>

perf timechart: Bounds check CPU

This commit adds bounds checking for cpu_id in several perf timechart
event handlers to prevent out-of-bounds memory accesses in CPU state
arrays.

> Ensure the CPU is within expected bounds to avoid out of bound memory
> accesses in arrays tracking CPU state.

Does this update catch all of the out-of-bounds accesses in this file?

Looking at the other handlers in builtin-timechart.c, it appears that
process_sample_power_end() was skipped. It passes the unvalidated
sample->cpu directly to c_state_end(), which uses it to index the
fixed-size cpus_cstate_state and cpus_cstate_start_times arrays:

static int
process_sample_power_end(struct timechart *tchart,
                         struct perf_sample *sample,
                         const char *backtrace __maybe_unused)
{
        c_state_end(tchart, sample->cpu, sample->time);
        return 0;
}

Should process_sample_power_end() also bounds-check sample->cpu?

Similarly, process_sample_sched_switch() and process_sample_sched_wakeup()
pass unvalidated sample->cpu values. Could these be used later in cpu2y()
via svg_process() to index the heap-allocated topology_map, allowing
out-of-bounds reads?

Also, does end_sample_processing() need a check against MAX_CPUS?

It loops up to tchart->numcpus (which is populated from the trace file
header) and accesses cpus_pstate_state. If the header specifies more
than 4096 CPUs, could this loop cause a buffer overflow during
finalization?

static void end_sample_processing(struct timechart *tchart)
{
        u64 cpu;
        struct power_event *pwr;

        for (cpu = 0; cpu <= tchart->numcpus; cpu++) {
                ...
                pwr = p_state_end(tchart, cpu, tchart->last_time);
                ...
        }
}

> diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
> index 64f69bd67c6b7..0db2a9bd5b911 100644
> --- a/tools/perf/builtin-timechart.c
> +++ b/tools/perf/builtin-timechart.c
> @@ -607,6 +607,10 @@ process_sample_cpu_idle(struct timechart *tchart __maybe_unused,
>  	u32 state  = perf_sample__intval(sample, "state");
>  	u32 cpu_id = perf_sample__intval(sample, "cpu_id");
>  
> +	if (cpu_id >= MAX_CPUS) {
> +		pr_debug("Out-of-bounds cpu_id %u\n", cpu_id);
> +		return -1;
> +	}
>  	if (state == (u32)PWR_EVENT_EXIT)
>  		c_state_end(tchart, cpu_id, sample->time);
>  	else

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260411065718.372240-1-irogers@google.com?part=26