Re: [PATCH 3/9] 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Dangling pointer in the `he__get_c2c_hists()` error path leads to a Use-After-Free and double-free during cleanup.
--

commit 4a6b2b9d1bf179db3b813952c508e27db4f604e7
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 in c2c_he__set_cpu(),
c2c_he__set_node(), and process_sample_event(). It aims to prevent
out-of-bounds heap writes and array access when processing perf.data files
with unusually large CPU or node values.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index 3dd45a550fdb7..f060dfbe11c28 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c

[ ... ]

> @@ -391,7 +399,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; large u32 wraps signed negative */
> +		if (cpu < 0 || cpu >= c2c.cpus_cnt)
> +			cpu = 0;
> +		node = c2c.cpu2node[cpu];
>  
>  		c2c_hists = he__get_c2c_hists(he, c2c.cl_sort, 2, machine->env);

[Severity: High]
This is a pre-existing issue, but can the error path in he__get_c2c_hists()
lead to a use-after-free and double-free during cleanup?

If c2c_hists__init() fails, the memory is freed but c2c_he->hists is not
cleared:

tools/perf/builtin-c2c.c: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;
	}
	...
}

Since the hist_entry is already added to the tree earlier in
process_sample_event(), returning -ENOMEM from here triggers teardown via
c2c_he_free(). Does this cause c2c_he_free() to dereference the dangling
c2c_he->hists pointer?

tools/perf/builtin-c2c.c:c2c_he_free() {
	...
	if (c2c_he->hists) {
		hists__delete_entries(&c2c_he->hists->hists);
		zfree(&c2c_he->hists);
	}
	...
}

>  		if (!c2c_hists) {
> @@ -410,7 +423,9 @@ static int process_sample_event(const struct perf_tool *tool __maybe_unused,

[ ... ]

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