Re: [PATCH 26/28] perf timechart: Bounds check cpu_id and fix topology_map allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional ownership claim of backtrace strings in `process_sample_sched_switch` and `process_sample_sched_wakeup` causes memory leaks.
- [High] ABI mismatch in syscall tracepoint handlers causes Control-Flow Integrity (CFI) crashes.
--

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

perf timechart: Bounds check cpu_id and fix topology_map allocation

This commit introduces bounds checking for cpu_id in timechart tracepoint
handlers to prevent out-of-bounds array accesses on corrupted perf.data
files. It also changes the backtrace parameter in handlers to a double
pointer to fix ownership transfer and avoid memory leaks.

> diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
> index 40297f2dcd035..bccc48cfb99a1 100644
> --- a/tools/perf/builtin-timechart.c
> +++ b/tools/perf/builtin-timechart.c

[ ... ]

> @@ -569,7 +570,7 @@ static const char *cat_backtrace(union perf_event *event,
>  typedef int (*tracepoint_handler)(struct timechart *tchart,
>  				  struct evsel *evsel,
>  				  struct perf_sample *sample,
> -				  const char *backtrace);
> +				  const char **backtrace);

This isn't a bug introduced by this patch, but changing this typedef
signature highlights an existing ABI mismatch.

Many syscall handlers registered in the power_tracepoints array (such as
process_enter_read and process_exit_read) are defined to take only 3
arguments.

When process_sample_event() casts these functions to tracepoint_handler
and calls them with 4 arguments, could this trigger Control-Flow Integrity
(CFI) traps on modern builds due to the strict type violation on the
indirect function call?

[ ... ]

> @@ -628,13 +660,20 @@ static int
>  process_sample_sched_wakeup(struct timechart *tchart,
>  			    struct evsel *evsel,
>  			    struct perf_sample *sample,
> -			    const char *backtrace)
> +			    const char **backtrace)
>  {
>  	u8 flags  = evsel__intval(evsel, sample, "common_flags");
>  	int waker = evsel__intval(evsel, sample, "common_pid");
>  	int wakee = evsel__intval(evsel, sample, "pid");
>  
> -	sched_wakeup(tchart, sample->cpu, sample->time, waker, wakee, flags, backtrace);
> +	/* sample->cpu used as index into topology_map[] during SVG generation */
> +	if (sample->cpu >= tchart->numcpus) {
> +		tchart->nr_invalid_cpu++;
> +		return 0;
> +	}
> +
> +	sched_wakeup(tchart, sample->cpu, sample->time, waker, wakee, flags, *backtrace);
> +	*backtrace = NULL;
>  	return 0;
>  }

Does unconditionally clearing the backtrace pointer here cause memory leaks
if the underlying functions do not actually consume the string?

If sched_wakeup() returns early on allocation failure without saving the
string, the backtrace is not stored. Because *backtrace is set to NULL
in this wrapper, the caller's free() becomes a no-op, and the unconsumed
backtrace string would be leaked.

> @@ -642,27 +681,41 @@ static int
>  process_sample_sched_switch(struct timechart *tchart,
>  			    struct evsel *evsel,
>  			    struct perf_sample *sample,
> -			    const char *backtrace)
> +			    const char **backtrace)
>  {
>  	int prev_pid   = evsel__intval(evsel, sample, "prev_pid");
>  	int next_pid   = evsel__intval(evsel, sample, "next_pid");
>  	u64 prev_state = evsel__intval(evsel, sample, "prev_state");
>  
> +	/* sample->cpu used as index into topology_map[] during SVG generation */
> +	if (sample->cpu >= tchart->numcpus) {
> +		tchart->nr_invalid_cpu++;
> +		return 0;
> +	}
> +
>  	sched_switch(tchart, sample->cpu, sample->time, prev_pid, next_pid,
> -		     prev_state, backtrace);
> +		     prev_state, *backtrace);
> +	*backtrace = NULL;
>  	return 0;
>  }

Similar to the issue above, does this cause a leak if sched_switch() ignores
the backtrace?

If sched_switch() ignores the backtrace because
prev_p->current->state == TYPE_NONE, the string isn't consumed but the
pointer is still cleared here.

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