Re: [PATCH 4/8] perf timechart: Include file offset in CPU bounds check messages

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds read vulnerability in cpu2y() when using --topology, leading to heap information leak or crash.
--

commit df9ed5394176bf63cfdd83249ffcb6787efbba52
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
perf timechart: Include file offset in CPU bounds check messages

This patch adds the perf.data file offset to the out-of-bounds CPU debug
messages in various timechart sample processing functions.

> diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
> index 630756bebe324..071987241a528 100644
> --- a/tools/perf/builtin-timechart.c
> +++ b/tools/perf/builtin-timechart.c
> @@ -605,8 +605,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");
>  
> +	/* perf.data is untrusted input — cpu_id may be corrupted */
>  	if (cpu_id >= MAX_CPUS) {
> -		pr_debug("Out-of-bounds cpu_id %u\n", cpu_id);
> +		pr_debug("at offset %#" PRIx64 ": out-of-bounds cpu_id %u\n",
> +			 sample->file_offset, cpu_id);

[Severity: High]
This is a pre-existing issue, but since the bounds check logic is being
modified here, does checking against MAX_CPUS adequately protect
topology_map when using --topology?

The topology_map is dynamically allocated using the number of CPUs:

tools/perf/util/svghelper.c:svg_build_topology_map() {
    ...
    topology_map = malloc(sizeof(int) * nr_cpus);
    ...
}

However, the untrusted CPU ID in the perf.data sample is only checked against
MAX_CPUS (4096) here. Later, cpu2y() accesses this array using the CPU ID:

tools/perf/util/svghelper.c:cpu2y() {
    ...
    return cpu2slot(topology_map[cpu]) * SLOT_MULT;
}

If nr_cpus is smaller than MAX_CPUS, could a crafted perf.data with a CPU ID
between nr_cpus and 4095 cause an out-of-bounds heap read, potentially leading
to a crash or an information leak into the SVG output?

>  		return -1;
>  	}
>  	if (state == (u32)PWR_EVENT_EXIT)

[ ... ]

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