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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

The cpu_idle, cpu_frequency, power_start, and power_frequency
tracepoint handlers extract cpu_id from the event payload via
evsel__intval() and use it directly as an array index into
cpus_cstate_start_times[] and cpus_pstate_start_times[], which
are allocated with MAX_CPUS (4096) entries.

Unlike sample->cpu which is validated in perf_session__deliver_event(),
cpu_id comes from the tracepoint data and is never bounds checked.
A crafted perf.data with a malicious cpu_id in a tracepoint event
causes out-of-bounds array accesses.

Validate cpu_id against tchart->numcpus (nr_cpus_avail from the
file header) and reject the event with an error if it is out of
range, as this indicates a corrupted or crafted file.

The power_end handler uses sample->cpu (not a tracepoint cpu_id
field).  Add a bounds check there too since a crafted file could
omit PERF_SAMPLE_CPU, leaving sample->cpu as the (u32)-1 sentinel
which would cause out-of-bounds access in c_state_end().

Also validate sample->cpu in sched_switch and sched_wakeup
handlers, which store it in cpu_sample structs later used as
array indices into topology_map[] during SVG generation.

Fix svg_build_topology_map() to allocate topology_map using
nr_cpus_avail instead of nr_cpus_online.  When offline CPUs exist,
nr_cpus_online < nr_cpus_avail, and a valid cpu_id that passes
the numcpus check could still exceed the topology_map allocation,
causing a heap out-of-bounds read in cpu2y().  Reject negative CPU
values in str_to_bitmap() to prevent perf_cpu_map__new("") on an
empty topology string from passing -1 to __set_bit(), which would
write at offset ULONG_MAX/BITS_PER_LONG.

Fix the pre-existing backtrace memory leak: change the
tracepoint_handler typedef to pass const char **backtrace
(pointer-to-pointer).  Handlers that consume the string
(sched_switch, sched_wakeup) set *backtrace = NULL to claim
ownership.  The caller always calls free() after the handler
returns — if ownership was taken the pointer is NULL and
free(NULL) is a no-op.  Skip cat_backtrace() entirely when
tchart->with_backtrace is not set.

Cap tchart->numcpus at MAX_CPUS in the HEADER_NRCPUS callback
so the bounds check cannot exceed the array allocation size.

Reported-by: sashiko-bot@kernel.org # Running on a local machine
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-timechart.c | 115 ++++++++++++++++++++++++++++-----
 tools/perf/util/svghelper.c    |   6 +-
 2 files changed, 104 insertions(+), 17 deletions(-)

diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
index 40297f2dcd0353cc..bccc48cfb99a1d57 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -71,6 +71,7 @@ struct timechart {
 	bool			io_only,
 				skip_eagain;
 	u64			io_events;
+	u32			nr_invalid_cpu;
 	u64			min_time,
 				merge_dist;
 };
@@ -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);
 
 static int process_sample_event(const struct perf_tool *tool,
 				union perf_event *event,
@@ -588,22 +589,46 @@ static int process_sample_event(const struct perf_tool *tool,
 
 	if (evsel->handler != NULL) {
 		tracepoint_handler f = evsel->handler;
-		return f(tchart, evsel, sample,
-			 cat_backtrace(event, sample, machine));
+		const char *bt = NULL;
+		int ret;
+
+		if (tchart->with_backtrace)
+			bt = cat_backtrace(event, sample, machine);
+		ret = f(tchart, evsel, sample, &bt);
+		/*
+		 * Handlers that consume backtrace (sched_switch,
+		 * sched_wakeup) store the pointer and set *bt = NULL
+		 * to claim ownership.  For all other handlers bt is
+		 * still ours to free.  free(NULL) is safe.
+		 */
+		free((void *)bt);
+		return ret;
 	}
 
 	return 0;
 }
 
 static int
-process_sample_cpu_idle(struct timechart *tchart __maybe_unused,
+process_sample_cpu_idle(struct timechart *tchart,
 			struct evsel *evsel,
 			struct perf_sample *sample,
-			const char *backtrace __maybe_unused)
+			const char **backtrace __maybe_unused)
 {
 	u32 state  = evsel__intval(evsel, sample, "state");
 	u32 cpu_id = evsel__intval(evsel, sample, "cpu_id");
 
+	/*
+	 * cpu_id from tracepoint data indexes cpus_cstate_start_times[]
+	 * and cpus_pstate_start_times[], both allocated as MAX_CPUS
+	 * entries.  Reject out-of-range values to prevent OOB writes;
+	 * numcpus (from nr_cpus_avail) is the tighter, valid bound.
+	 */
+	if (cpu_id >= tchart->numcpus) {
+		pr_err("cpu_idle event cpu_id %u >= nr_cpus_avail %u\n",
+		       cpu_id, tchart->numcpus);
+		return -EINVAL;
+	}
+
 	if (state == (u32)PWR_EVENT_EXIT)
 		c_state_end(tchart, cpu_id, sample->time);
 	else
@@ -615,11 +640,18 @@ static int
 process_sample_cpu_frequency(struct timechart *tchart,
 			     struct evsel *evsel,
 			     struct perf_sample *sample,
-			     const char *backtrace __maybe_unused)
+			     const char **backtrace __maybe_unused)
 {
 	u32 state  = evsel__intval(evsel, sample, "state");
 	u32 cpu_id = evsel__intval(evsel, sample, "cpu_id");
 
+	/* Same bounds check as process_sample_cpu_idle — see comment there */
+	if (cpu_id >= tchart->numcpus) {
+		pr_err("cpu_frequency event cpu_id %u >= nr_cpus_avail %u\n",
+		       cpu_id, tchart->numcpus);
+		return -EINVAL;
+	}
+
 	p_state_change(tchart, cpu_id, sample->time, state);
 	return 0;
 }
@@ -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;
 }
 
@@ -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;
 }
 
 #ifdef SUPPORT_OLD_POWER_EVENTS
 static int
-process_sample_power_start(struct timechart *tchart __maybe_unused,
+process_sample_power_start(struct timechart *tchart,
 			   struct evsel *evsel,
 			   struct perf_sample *sample,
-			   const char *backtrace __maybe_unused)
+			   const char **backtrace __maybe_unused)
 {
 	u64 cpu_id = evsel__intval(evsel, sample, "cpu_id");
 	u64 value  = evsel__intval(evsel, sample, "value");
 
+	/* Same bounds check as process_sample_cpu_idle — see comment there */
+	if (cpu_id >= tchart->numcpus) {
+		pr_err("power_start event cpu_id %" PRIu64 " >= nr_cpus_avail %u\n",
+		       cpu_id, tchart->numcpus);
+		return -EINVAL;
+	}
+
 	c_state_start(cpu_id, sample->time, value);
 	return 0;
 }
@@ -671,8 +724,16 @@ static int
 process_sample_power_end(struct timechart *tchart,
 			 struct evsel *evsel __maybe_unused,
 			 struct perf_sample *sample,
-			 const char *backtrace __maybe_unused)
+			 const char **backtrace __maybe_unused)
 {
+	/*
+	 * sample->cpu is validated centrally when PERF_SAMPLE_CPU is
+	 * set, but a crafted file could omit it from sample_type.
+	 */
+	if (sample->cpu >= tchart->numcpus) {
+		tchart->nr_invalid_cpu++;
+		return 0;
+	}
 	c_state_end(tchart, sample->cpu, sample->time);
 	return 0;
 }
@@ -681,11 +742,18 @@ static int
 process_sample_power_frequency(struct timechart *tchart,
 			       struct evsel *evsel,
 			       struct perf_sample *sample,
-			       const char *backtrace __maybe_unused)
+			       const char **backtrace __maybe_unused)
 {
 	u64 cpu_id = evsel__intval(evsel, sample, "cpu_id");
 	u64 value  = evsel__intval(evsel, sample, "value");
 
+	/* Same bounds check as process_sample_cpu_idle — see comment there */
+	if (cpu_id >= tchart->numcpus) {
+		pr_err("power_frequency event cpu_id %" PRIu64 " >= nr_cpus_avail %u\n",
+		       cpu_id, tchart->numcpus);
+		return -EINVAL;
+	}
+
 	p_state_change(tchart, cpu_id, sample->time, value);
 	return 0;
 }
@@ -1519,7 +1587,8 @@ static int process_header(struct perf_file_section *section __maybe_unused,
 
 	switch (feat) {
 	case HEADER_NRCPUS:
-		tchart->numcpus = ph->env.nr_cpus_avail;
+		/* Cap at MAX_CPUS — the allocation size of cpus_cstate/pstate arrays */
+		tchart->numcpus = min((int)ph->env.nr_cpus_avail, MAX_CPUS);
 		break;
 
 	case HEADER_CPU_TOPOLOGY:
@@ -1625,6 +1694,16 @@ static int __cmd_timechart(struct timechart *tchart, const char *output_name)
 					    tchart,
 					    process_header);
 
+	/*
+	 * Truncated files (interrupted recording) lose all feature
+	 * sections so the HEADER_NRCPUS callback never fires, and
+	 * pipe mode doesn't use perf_header__process_sections at all.
+	 * Fall back to MAX_CPUS — the actual allocation size of the
+	 * cpus_cstate/pstate arrays.
+	 */
+	if (!tchart->numcpus)
+		tchart->numcpus = MAX_CPUS;
+
 	if (!perf_session__has_traces(session, "timechart record"))
 		goto out_delete;
 
@@ -1646,6 +1725,12 @@ static int __cmd_timechart(struct timechart *tchart, const char *output_name)
 
 	pr_info("Written %2.1f seconds of trace to %s.\n",
 		(tchart->last_time - tchart->first_time) / (double)NSEC_PER_SEC, output_name);
+
+	if (tchart->nr_invalid_cpu) {
+		pr_warning("WARNING: %u events had invalid CPU values and were skipped.\n"
+			   "  Scheduling and power state data may be incomplete.\n",
+			   tchart->nr_invalid_cpu);
+	}
 out_delete:
 	perf_session__delete(session);
 	return ret;
diff --git a/tools/perf/util/svghelper.c b/tools/perf/util/svghelper.c
index e360e7736c7ba65b..a3c7cfecc072f3e3 100644
--- a/tools/perf/util/svghelper.c
+++ b/tools/perf/util/svghelper.c
@@ -736,7 +736,8 @@ static int str_to_bitmap(char *s, cpumask_t *b, int nr_cpus)
 		return -1;
 
 	perf_cpu_map__for_each_cpu(cpu, idx, map) {
-		if (cpu.cpu >= nr_cpus) {
+		/* perf_cpu_map__new("") yields cpu=-1; reject to prevent __set_bit OOB */
+		if (cpu.cpu < 0 || cpu.cpu >= nr_cpus) {
 			ret = -1;
 			break;
 		}
@@ -756,7 +757,8 @@ int svg_build_topology_map(struct perf_env *env)
 	char *sib_core, *sib_thr;
 	int ret = -1;
 
-	nr_cpus = min(env->nr_cpus_online, MAX_NR_CPUS);
+	/* Use nr_cpus_avail: offline CPUs still need slots in the topology map */
+	nr_cpus = min(env->nr_cpus_avail, MAX_NR_CPUS);
 
 	t.sib_core_nr = env->nr_sibling_cores;
 	t.sib_thr_nr = env->nr_sibling_threads;
-- 
2.54.0