Re: [PATCH 3/8] perf c2c: Bounds-check CPU and node IDs before bitmap and array access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The bounds check for `cpu` uses signed comparison, allowing negative indices to bypass the check and cause an out-of-bounds read on `c2c.cpu2node`.
- [High] Dangling pointer in `he__get_c2c_hists()` error path leads to Use-After-Free and Double Free.
--

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

perf c2c: Bounds-check CPU and node IDs before bitmap and array access

This commit adds bounds checks for CPU and node IDs before accessing bitmaps
and arrays in builtin-c2c.c. These checks help ensure that a crafted
perf.data file with exceedingly large IDs cannot cause out-of-bounds heap
writes.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
[ ... ]
> @@ -386,7 +394,12 @@ static int process_sample_event(const struct perf_tool *tool __maybe_unused,
>  		 * Doing node stats only for single callchain data.
>  		 */
>  		int cpu = sample->cpu == (unsigned int) -1 ? 0 : sample->cpu;
> -		int node = c2c.cpu2node[cpu];
> +		int node;
> +
> +		/* cpu2node[] has c2c.cpus_cnt entries from env->nr_cpus_avail */
> +		if (cpu >= c2c.cpus_cnt)
> +			cpu = 0;
> +		node = c2c.cpu2node[cpu];

[Severity: High]
This isn't a bug introduced by this patch, but does this bounds check
allow negative indices to bypass it?

Because cpu is declared as a signed int, if sample->cpu is a large value
like 0xFFFFFFFE, it converts to a negative integer (e.g., -2). The bounds
check uses signed comparison, so the negative value bypasses the check,
resulting in an out-of-bounds read at c2c.cpu2node[-2].

>  		c2c_hists = he__get_c2c_hists(he, c2c.cl_sort, 2, machine->env);

[Severity: High]
This is a pre-existing issue, but can he__get_c2c_hists() create a
dangling pointer on its error path?

Inside he__get_c2c_hists():

hists = c2c_he->hists = zalloc(sizeof(*hists));
if (!hists)
        return NULL;

ret = c2c_hists__init(hists, sort, nr_header_lines, env);
if (ret) {
        free(hists);
        return NULL;
}

If c2c_hists__init() fails, it frees hists but leaves the dangling pointer
in c2c_he->hists. Because c2c_he outlives the function execution, later
during shutdown, c2c_he_free() will access the dangling pointer and trigger
a use-after-free and double free.

>  		if (!c2c_hists) {
>  			ret = -ENOMEM;
>  			goto out;
>  		}

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