[PATCHES v2 00/13] perf tools: Fix pre-existing bugs in symbols, dso, bpf, sched, c2c, hwmon, and cs-etm

[PATCHES v2 00/13] perf tools: Fix pre-existing bugs in symbols, dso, bpf, sched, c2c, hwmon, and cs-etm

[Arnaldo Carvalho de Melo]

Hi,

Thirteen more pre-existing bugs found by sashiko-bot during AI-assisted
code review.  All are independent of the perf-data-validation hardening
series -- they are latent bugs in surrounding code exposed during review.

The fixes are grouped by subsystem:

ELF/build-id parsing (patches 1-2):
  symbol-minimal.c carries a copy-paste typo that byte-swaps p_offset
  instead of p_filesz for 32-bit ELF.  The ssize_t p_filesz value is
  used without checking for negative.

ELF note iteration (patch 3):
  sysfs__read_build_id() in the libelf path can loop forever when a
  note section contains zero-filled entries (namesz + descsz == 0).
  Break when no progress can be made.

DSO decompression and open (patches 4-5):
  dso__get_filename() copies a decompressed path with strcpy() into a
  potentially shorter heap buffer.  filename__decompress() fails to set
  the error code on the uncompressed fallback path, leaving callers
  with a stale errno.

Buffer overflow in root_dir path construction (patch 6):
  machine.c and symbol.c use sprintf() to build paths with root_dir,
  which can overflow the fixed-size buffer.  Switch to snprintf().

hwmon fd check (patch 7):
  hwmon_pmu__describe_items() tests fd > 0, rejecting the valid fd 0.

Undefined behavior in perf sched (patch 8):
  map__findnew_thread() uses (void*)1 as a sentinel for colored threads.
  This value gets dereferenced as a struct pointer and passed to free()
  on cleanup.  Replace with a proper allocation and a boolean color flag.

BPF metadata validation (patches 9-11):
  synthesize_bpf_prog_name() trusts func_info_rec_size and sub_id from
  perf.data without validation.  bpf_metadata_alloc() stores the event
  size in a __u16 without overflow checking.  bpil_offs_to_addr()
  converts untrusted offsets to heap pointers without bounds checking.

Memory leak in c2c (patch 12):
  c2c hist entries register format list entries but never unregister
  them on free, leaking the list nodes.

CoreSight ETM CPU ID validation (patch 13):
  cs_etm__process_auxtrace_info_full() compares an unsigned CPU ID
  from perf.data metadata against a signed int without range checking.
  A large unsigned value wraps negative, bypassing the bounds check.

Build-tested with gcc and clang.  Passes perf test on x86_64.

Arnaldo Carvalho de Melo (13):
  perf symbols: Fix bswap copy-paste error for 32-bit ELF p_filesz
  perf symbols: Validate p_filesz before use in filename__read_build_id()
  perf symbols: Break infinite loop on zero-filled notes in sysfs__read_build_id()
  perf dso: Fix heap overflow in dso__get_filename() on decompressed path
  perf dso: Set error code when open() fails on uncompressed fallback path
  perf tools: Use snprintf() for root_dir path construction
  perf hwmon: Fix fd check to accept fd 0 in hwmon_pmu__describe_items()
  perf sched: Replace (void*)1 sentinel with proper runtime allocation
  perf bpf: Validate func_info_rec_size and sub_id in synthesize_bpf_prog_name()
  perf bpf: Reject oversized BPF metadata events that truncate header.size
  perf bpf: Bounds-check array offsets in bpil_offs_to_addr()
  perf c2c: Free format list entries when releasing c2c hist entries
  perf cs-etm: Reject CPU IDs that would overflow signed comparison

 tools/perf/builtin-c2c.c         |  1 +
 tools/perf/builtin-sched.c       | 23 +++++++++++++++++------
 tools/perf/util/bpf-event.c      | 13 ++++++++++++-
 tools/perf/util/bpf-utils.c      | 16 ++++++++++++++++
 tools/perf/util/cs-etm.c         |  9 ++++++++-
 tools/perf/util/dso.c            | 14 ++++++++++++--
 tools/perf/util/hwmon_pmu.c      |  2 +-
 tools/perf/util/machine.c        |  2 +-
 tools/perf/util/symbol-elf.c     |  3 +++
 tools/perf/util/symbol-minimal.c |  5 ++++-
 tools/perf/util/symbol.c         |  2 +-
 11 files changed, 76 insertions(+), 14 deletions(-)

Changes since v1:
- Dropped O_NONBLOCK patch per Ian Rogers' review: without
  TEMP_FAILURE_RETRY, O_NONBLOCK causes slow file systems to fail; the
  is_regular_file() checks are the correct mitigation.
- Dropped fixed-buffer rewrite of sysfs__read_build_id() for the
  no-libelf path (type-punning fix); needs more consideration.
- Patch 11 (bpil bounds check): clear the array bit when zeroing invalid
  offsets, so bpil_addr_to_offs() won't leak the heap address into
  output perf.data.
- Patch 13 (cs-etm): change > INT_MAX to >= INT_MAX, preventing
  max_cpu + 1 signed integer overflow in auxtrace_queues__init_nr().

Developed with AI assistance (Claude/sashiko), tagged in commits.

[PATCH 01/13] perf symbols: Fix bswap copy-paste error for 32-bit ELF p_filesz

[Arnaldo Carvalho de Melo]

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

filename__read_build_id() byte-swaps 32-bit ELF program headers on
cross-endian files, but line 178 passes p_offset to bswap_32() instead
of p_filesz:

  hdrs.phdr32[i].p_filesz = bswap_32(hdrs.phdr32[i].p_offset);

This clobbers p_filesz with the already-swapped p_offset value.  The
64-bit path on line 182 is correct and swaps p_filesz from p_filesz.

The consequence is that the PT_NOTE segment read uses the wrong size,
which can cause either a short read (missing the build-id) or an
oversized read (reading past the segment into adjacent data).

Fix by swapping the correct field.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: fef8f648bb47726d ("perf symbol: Fix use-after-free in filename__read_build_id")
Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/symbol-minimal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index 091071d06416e290..f4b0a711a62cf3de 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -175,7 +175,7 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
 			if (elf32) {
 				hdrs.phdr32[i].p_type = bswap_32(hdrs.phdr32[i].p_type);
 				hdrs.phdr32[i].p_offset = bswap_32(hdrs.phdr32[i].p_offset);
-				hdrs.phdr32[i].p_filesz = bswap_32(hdrs.phdr32[i].p_offset);
+				hdrs.phdr32[i].p_filesz = bswap_32(hdrs.phdr32[i].p_filesz);
 			} else {
 				hdrs.phdr64[i].p_type = bswap_32(hdrs.phdr64[i].p_type);
 				hdrs.phdr64[i].p_offset = bswap_64(hdrs.phdr64[i].p_offset);
-- 
2.54.0

[PATCH 02/13] perf symbols: Validate p_filesz before use in filename__read_build_id()

[Arnaldo Carvalho de Melo]

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

filename__read_build_id() stores ELF p_filesz in a ssize_t variable.
A crafted 32-bit ELF with p_filesz = 0xFFFFFFFF produces ssize_t value
-1.  The comparison `p_filesz > buf_size` evaluates false because signed
-1 is less than any non-negative buf_size, so the realloc is skipped and
buf remains NULL.

The subsequent read(fd, NULL, -1) returns -1, which equals p_filesz,
passing the error check.  read_build_id() then dereferences the NULL
buffer.

Add an explicit check for p_filesz <= 0 before using the value,
catching both zero-length and sign-wrapped negative sizes from crafted
ELF files.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: ba0b7081f7a521d7 ("perf symbol-minimal: Fix ehdr reading in filename__read_build_id")
Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/symbol-minimal.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index f4b0a711a62cf3de..0a71d146395271a6 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -186,6 +186,9 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
 			continue;
 
 		p_filesz = elf32 ? hdrs.phdr32[i].p_filesz : hdrs.phdr64[i].p_filesz;
+		/* ssize_t can go negative with crafted ELF p_filesz values */
+		if (p_filesz <= 0)
+			continue;
 		if (p_filesz > buf_size) {
 			void *tmp;
 
-- 
2.54.0

[PATCH 03/13] perf symbols: Break infinite loop on zero-filled notes in sysfs__read_build_id()

[Arnaldo Carvalho de Melo]

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

sysfs__read_build_id() iterates ELF note headers from sysfs files in a
while(1) loop.  If the file contains a zero-filled note header (both
n_namesz and n_descsz are 0), the code computes n = namesz + descsz = 0
and calls read(fd, bf, 0).  read() with count 0 returns 0, which
matches the expected (ssize_t)n value, so the error check passes and
the loop repeats — reading the same zero bytes and spinning forever.

This can happen with corrupted or zero-padded sysfs pseudo-files.

Add a check for n == 0 before the read, since no valid ELF note has
both name and description of zero length.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: f1617b40596cb341 ("perf symbols: Record the build_ids of kernel modules too")
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/symbol-elf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index d84e2e031d430cf5..dc48e2d2763379b9 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -995,6 +995,9 @@ int sysfs__read_build_id(const char *filename, struct build_id *bid)
 			} else {
 				n = namesz + descsz;
 			}
+			/* no valid note has both namesz and descsz zero */
+			if (n == 0)
+				break;
 			if (read(fd, bf, n) != (ssize_t)n)
 				break;
 		}
-- 
2.54.0

[PATCH 04/13] perf dso: Fix heap overflow in dso__get_filename() on decompressed path

[Arnaldo Carvalho de Melo]

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

dso__get_filename() allocates name with malloc(PATH_MAX), but the
dso__filename_with_chroot() path replaces name with an asprintf'd
exact-size string (e.g. 8 bytes for "/a/b.ko").  When the DSO needs
decompression, dso__decompress_kmodule_path() writes the temp path
("/tmp/perf-kmod-XXXXXX", 22 bytes) into newpath, and strcpy(name,
newpath) overflows the smaller allocation.

Replace the strcpy with strdup(newpath) + free(name) so the buffer
is always correctly sized for its content.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: 1d6b3c9ba756a513 ("perf tools: Decompress kernel module when reading DSO data")
Cc: Namhyung Kim <namhyung@kernel.org>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 5d017975873817ec..511921bd901d8145 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -603,8 +603,15 @@ static char *dso__get_filename(struct dso *dso, const char *root_dir,
 
 		/* empty pathname means file wasn't actually compressed */
 		if (newpath[0] != '\0') {
+			char *tmp = strdup(newpath);
+
+			if (!tmp) {
+				unlink(newpath);
+				goto out;
+			}
+			free(name);
+			name = tmp;
 			*decomp = true;
-			strcpy(name, newpath);
 		}
 	}
 	return name;
-- 
2.54.0

[PATCH 05/13] perf dso: Set error code when open() fails on uncompressed fallback path

[Arnaldo Carvalho de Melo]

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

filename__decompress() has an early return for files that are not
actually compressed, where it calls open() directly.  When open()
fails, the function returns -1 but never sets *err.  The caller chain
(decompress_kmodule → dso__decompress_kmodule_path → dso__get_filename)
then reads *dso__load_errno(dso) to set errno, but that field was never
populated, so errno gets a stale or zero value.

With errno=0, __open_dso() computes fd = -errno = 0, which is non-
negative, so callers treat fd 0 (stdin) as a valid DSO file descriptor.

Set *err = errno when open() fails on the uncompressed path, matching
the error handling on the compressed path at line 354.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: 8b42b7e5e8b5692b ("perf tools: Add is_compressed callback to compressions array")
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 511921bd901d8145..1a2fc6d18da74d6c 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -344,9 +344,12 @@ int filename__decompress(const char *name, char *pathname,
 	 * descriptor to the uncompressed file.
 	 */
 	if (!compressions[comp].is_compressed(name)) {
+		fd = open(name, O_RDONLY | O_CLOEXEC);
+		if (fd < 0)
+			*err = errno;
 		if (pathname && len > 0)
 			pathname[0] = '\0';
-		return open(name, O_RDONLY | O_CLOEXEC);
+		return fd;
 	}
 
 	fd = mkostemp(tmpbuf, O_CLOEXEC);
-- 
2.54.0

[PATCH 06/13] perf tools: Use snprintf() for root_dir path construction

[Arnaldo Carvalho de Melo]

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

get_kernel_version() in machine.c and dso__load_guest_kernel_sym() in
symbol.c use sprintf() to construct paths by prepending root_dir to
"/proc/version" and "/proc/kallsyms" respectively.  Both write into
PATH_MAX stack buffers, but root_dir comes from --guestmount or KVM
configuration and is not length-checked.  A root_dir at or near
PATH_MAX causes a stack buffer overflow.

Switch to snprintf() with sizeof(path) to prevent overflow.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: a1645ce12adb6c9c ("perf: 'perf kvm' tool for monitoring guest performance from host")
Cc: Zhang Yanmin <yanmin_zhang@linux.intel.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 2 +-
 tools/perf/util/symbol.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index da1ad58758afd9d9..58fa57e3d1a15f37 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1336,7 +1336,7 @@ static char *get_kernel_version(const char *root_dir)
 	char *name, *tmp;
 	const char *prefix = "Linux version ";
 
-	sprintf(version, "%s/proc/version", root_dir);
+	snprintf(version, sizeof(version), "%s/proc/version", root_dir);
 	file = fopen(version, "r");
 	if (!file)
 		return NULL;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 0c46b24ee0986059..c2328c9cec1565fc 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2275,7 +2275,7 @@ static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map)
 		if (!kallsyms_filename)
 			return -1;
 	} else {
-		sprintf(path, "%s/proc/kallsyms", machine->root_dir);
+		snprintf(path, sizeof(path), "%s/proc/kallsyms", machine->root_dir);
 		kallsyms_filename = path;
 	}
 
-- 
2.54.0

[PATCH 07/13] perf hwmon: Fix fd check to accept fd 0 in hwmon_pmu__describe_items()

[Arnaldo Carvalho de Melo]

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

hwmon_pmu__describe_items() checks 'if (fd > 0)' after openat(), which
incorrectly rejects fd 0.  While fd 0 is normally stdin, if stdin has
been closed (common in daemon/service contexts), the kernel reuses fd 0
for the next open.  With fd > 0, the sysfs file is not read and the fd
is leaked.

Change to 'if (fd >= 0)' to match the standard openat() error check.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: 53cc0b351ec99278 ("perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs")
Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hwmon_pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/hwmon_pmu.c b/tools/perf/util/hwmon_pmu.c
index efd3067a2e591050..ed544dca70c380b2 100644
--- a/tools/perf/util/hwmon_pmu.c
+++ b/tools/perf/util/hwmon_pmu.c
@@ -435,7 +435,7 @@ static size_t hwmon_pmu__describe_items(struct hwmon_pmu *hwm, char *out_buf, si
 			hwmon_item_strs[bit],
 			is_alarm ? "_alarm" : "");
 		fd = openat(dir, buf, O_RDONLY);
-		if (fd > 0) {
+		if (fd >= 0) {
 			ssize_t read_len = read(fd, buf, sizeof(buf) - 1);
 
 			while (read_len > 0 && buf[read_len - 1] == '\n')
-- 
2.54.0

[PATCH 08/13] perf sched: Replace (void*)1 sentinel with proper runtime allocation

[Arnaldo Carvalho de Melo]

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

map__findnew_thread() marks color-pid threads by storing (void*)1 as
the thread private data via thread__set_priv().  This sentinel value
causes two problems:

1. thread__get_runtime() returns (void*)1 as a struct thread_runtime
   pointer.  Any field access (e.g. tr->shortname) dereferences address
   1, which is an unmapped page — immediate segfault.

2. cmd_sched() registers free() as the thread priv destructor, so thread
   cleanup calls free((void*)1) — undefined behavior that corrupts the
   heap on many allocators.

Fix by adding a 'color' flag to struct thread_runtime and allocating a
real runtime struct for color-pid threads.  thread__has_color() now
checks the flag instead of relying on priv being non-NULL.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: 58a606149c60d5da ("perf sched: Avoid union type punning undefined behavior")
Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-sched.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 5f16caebd9645da0..7fd63a9db4574fbf 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -274,6 +274,7 @@ struct thread_runtime {
 	u64 migrations;
 
 	int prio;
+	bool color;
 };
 
 /* per event run time data */
@@ -1589,22 +1590,32 @@ static int process_sched_wakeup_ignore(const struct perf_tool *tool __maybe_unus
 
 static bool thread__has_color(struct thread *thread)
 {
-	return thread__priv(thread) != NULL;
+	struct thread_runtime *tr = thread__priv(thread);
+
+	return tr != NULL && tr->color;
 }
 
 static struct thread*
 map__findnew_thread(struct perf_sched *sched, struct machine *machine, pid_t pid, pid_t tid)
 {
 	struct thread *thread = machine__findnew_thread(machine, pid, tid);
-	bool color = false;
 
-	if (!sched->map.color_pids || !thread || thread__priv(thread))
+	if (!sched->map.color_pids || !thread)
 		return thread;
 
-	if (thread_map__has(sched->map.color_pids, tid))
-		color = true;
+	/*
+	 * Always check the color-pids map, even if thread__priv() is
+	 * already set.  COMM events processed before the first sched_switch
+	 * allocate a thread_runtime via thread__get_runtime(), so priv is
+	 * non-NULL before we ever get here.  Skipping the check on non-NULL
+	 * priv would prevent those threads from being colored.
+	 */
+	if (thread_map__has(sched->map.color_pids, tid)) {
+		struct thread_runtime *tr = thread__get_runtime(thread);
 
-	thread__set_priv(thread, color ? ((void*)1) : NULL);
+		if (tr)
+			tr->color = true;
+	}
 	return thread;
 }
 
-- 
2.54.0

[PATCH 09/13] perf bpf: Validate func_info_rec_size and sub_id in synthesize_bpf_prog_name()

[Arnaldo Carvalho de Melo]

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

synthesize_bpf_prog_name() computes a pointer into the func_info array
using sub_id * info->func_info_rec_size without validating either value.
Both come from perf.data and are untrusted:

- A func_info_rec_size smaller than sizeof(struct bpf_func_info) means
  the finfo pointer would reference a truncated entry, reading past it
  into adjacent data.

- A sub_id >= nr_func_info computes an offset past the func_info buffer,
  causing an out-of-bounds read.

Add bounds checks for both values before computing the pointer offset.
When validation fails, fall through to the non-BTF name path instead
of reading garbage.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: 7b612e291a5affb1 ("perf tools: Synthesize PERF_RECORD_* for loaded BPF programs")
Cc: Song Liu <songliubraving@fb.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/bpf-event.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
index c4594969d7677238..fe6fbca508c5135c 100644
--- a/tools/perf/util/bpf-event.c
+++ b/tools/perf/util/bpf-event.c
@@ -143,7 +143,9 @@ static int synthesize_bpf_prog_name(char *buf, int size,
 	name_len = scnprintf(buf, size, "bpf_prog_");
 	name_len += snprintf_hex(buf + name_len, size - name_len,
 				 prog_tags[sub_id], BPF_TAG_SIZE);
-	if (btf) {
+	if (btf &&
+	    info->func_info_rec_size >= sizeof(*finfo) &&
+	    sub_id < info->nr_func_info) {
 		finfo = func_infos + sub_id * info->func_info_rec_size;
 		t = btf__type_by_id(btf, finfo->type_id);
 		if (t)
-- 
2.54.0

[PATCH 10/13] perf bpf: Reject oversized BPF metadata events that truncate header.size

[Arnaldo Carvalho de Melo]

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

bpf_metadata_alloc() computes event_size from the number of BPF metadata
variables and stores it in header.size, which is __u16.  With 204 or
more .rodata variables prefixed "bpf_metadata_", event_size exceeds
65535 and silently truncates.

The truncated header.size causes synthesize_perf_record_bpf_metadata()
to allocate a buffer sized by the truncated value, then memcpy the full
event data into it — a heap buffer overflow.

Add a check that event_size fits in __u16 before proceeding.  BPF
programs with that many metadata variables are exotic enough that
silently dropping the metadata is acceptable.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: ab38e84ba9a80581 ("perf record: collect BPF metadata from existing BPF programs")
Cc: Blake Jones <blakejones@google.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/bpf-event.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
index fe6fbca508c5135c..57d53ba848359e12 100644
--- a/tools/perf/util/bpf-event.c
+++ b/tools/perf/util/bpf-event.c
@@ -369,6 +369,15 @@ static struct bpf_metadata *bpf_metadata_alloc(__u32 nr_prog_tags,
 
 	event_size = sizeof(metadata->event->bpf_metadata) +
 	    nr_variables * sizeof(metadata->event->bpf_metadata.entries[0]);
+	/*
+	 * header.size is __u16.  synthesize_perf_record_bpf_metadata()
+	 * adds machine->id_hdr_size (up to ~64 bytes) after this, so
+	 * leave headroom to prevent the final size from wrapping.
+	 */
+	if (event_size > UINT16_MAX - 256) {
+		bpf_metadata_free(metadata);
+		return NULL;
+	}
 	metadata->event = zalloc(event_size);
 	if (!metadata->event) {
 		bpf_metadata_free(metadata);
-- 
2.54.0

[PATCH 11/13] perf bpf: Bounds-check array offsets in bpil_offs_to_addr()

[Arnaldo Carvalho de Melo]

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

bpil_offs_to_addr() converts offsets stored in perf.data's
bpf_prog_info_linear structure into heap pointers by adding the offset
to the data allocation base.  The offsets come from untrusted file input
and are not validated against data_len.

If an offset exceeds data_len, the computed address points outside the
allocated data buffer.  Callers like synthesize_bpf_prog_name() then
dereference prog_tags[sub_id] or func_info pointers, reading arbitrary
heap memory.

Add a bounds check: when an offset exceeds data_len, zero the field
and skip the conversion.  This prevents out-of-bounds pointer
construction from crafted perf.data files.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Fixes: 6ac22d036f86c4e2 ("perf bpf: Pull in bpf_program__get_prog_info_linear()")
Cc: Dave Marchevsky <davemarchevsky@fb.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/bpf-utils.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/perf/util/bpf-utils.c b/tools/perf/util/bpf-utils.c
index d6d2c9c190f7afbf..69148197b1ef9e8b 100644
--- a/tools/perf/util/bpf-utils.c
+++ b/tools/perf/util/bpf-utils.c
@@ -264,12 +264,28 @@ void bpil_offs_to_addr(struct perf_bpil *info_linear)
 	for (i = PERF_BPIL_FIRST_ARRAY; i < PERF_BPIL_LAST_ARRAY; ++i) {
 		const struct bpil_array_desc *desc = &bpil_array_desc[i];
 		__u64 addr, offs;
+		__u32 count, size;
 
 		if ((info_linear->arrays & (1UL << i)) == 0)
 			continue;
 
 		offs = bpf_prog_info_read_offset_u64(&info_linear->info,
 						     desc->array_offset);
+		count = bpf_prog_info_read_offset_u32(&info_linear->info,
+						      desc->count_offset);
+		size = bpf_prog_info_read_offset_u32(&info_linear->info,
+						     desc->size_offset);
+		/* offset and extent from perf.data are untrusted — keep within data[] */
+		if (offs >= info_linear->data_len ||
+		    (u64)count * size > info_linear->data_len - offs) {
+			bpf_prog_info_set_offset_u64(&info_linear->info,
+						     desc->array_offset, 0);
+			bpf_prog_info_set_offset_u32(&info_linear->info,
+						     desc->count_offset, 0);
+			/* clear the bit so bpil_addr_to_offs() won't reverse a zeroed address */
+			info_linear->arrays &= ~(1UL << i);
+			continue;
+		}
 		addr = offs + ptr_to_u64(info_linear->data);
 		bpf_prog_info_set_offset_u64(&info_linear->info,
 					     desc->array_offset, addr);
-- 
2.54.0

[PATCH 12/13] perf c2c: Free format list entries when releasing c2c hist entries

[Arnaldo Carvalho de Melo]

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

c2c_hists__init() calls hpp_list__parse() which allocates and registers
format entries on hists->list.  When c2c_he_free() destroys a c2c hist
entry, it deletes the histogram entries and frees the hists container but
never unregisters the format list entries, leaking them.

Call perf_hpp__reset_output_field() before freeing the hists to properly
unregister and free all format entries.

Fixes: f485e33c4543ac31 ("perf c2c report: Add cacheline hists processing")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/finding/41
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-c2c.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index e205f58b2f3d3786..07c7e8fb315e6cf3 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -185,6 +185,7 @@ static void c2c_he_free(void *he)
 	c2c_he = container_of(he, struct c2c_hist_entry, he);
 	if (c2c_he->hists) {
 		hists__delete_entries(&c2c_he->hists->hists);
+		perf_hpp__reset_output_field(&c2c_he->hists->list);
 		zfree(&c2c_he->hists);
 	}
 
-- 
2.54.0

[PATCH 13/13] perf cs-etm: Reject CPU IDs that would overflow signed comparison

[Arnaldo Carvalho de Melo]

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

metadata[j][CS_ETM_CPU] is a u64 from perf.data, but the comparison
with max_cpu casts it to (int).  A crafted value like 0xFFFFFFFF becomes
-1 after the cast, which compares less than max_cpu (0), so the queue
array is never sized to accommodate it.  When the value is later passed
to cs_etm__get_queue(), it indexes queue_array with the original large
value, causing an out-of-bounds access.

Validate that CS_ETM_CPU fits in an int before using it in the signed
comparison.

Fixes: 57880a7966be510c ("perf: cs-etm: Allocate queues for all CPUs")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/finding/2
Cc: James Clark <james.clark@arm.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/cs-etm.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 5e92359f51a7cb87..0927b0b9c06b1504 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -6,6 +6,7 @@
  * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
  */
 
+#include <limits.h>
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/coresight-pmu.h>
@@ -3468,7 +3469,13 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
 			goto err_free_metadata;
 		}
 
-		if ((int) metadata[j][CS_ETM_CPU] > max_cpu)
+		/* CPU id comes from perf.data and must fit max_cpu + 1 without overflow */
+		if (metadata[j][CS_ETM_CPU] >= INT_MAX) {
+			err = -EINVAL;
+			goto err_free_metadata;
+		}
+
+		if ((int)metadata[j][CS_ETM_CPU] > max_cpu)
 			max_cpu = metadata[j][CS_ETM_CPU];
 	}
 
-- 
2.54.0

Re: [PATCH 05/13] perf dso: Set error code when open() fails on uncompressed fallback path

[sashiko-bot]

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

Pre-existing issues:
- [High] Negative custom error codes leak into standard errno and cause __open_dso() to compute a positive fake file descriptor.
--

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

perf dso: Set error code when open() fails on uncompressed fallback path

This commit sets the error code when open() fails on the uncompressed
fallback path in filename__decompress(). It ensures the caller chain
correctly reads the populated errno instead of a stale or zero value.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 511921bd901d8..1a2fc6d18da74 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -344,9 +344,12 @@ int filename__decompress(const char *name, char *pathname,
>  	 * descriptor to the uncompressed file.
>  	 */
>  	if (!compressions[comp].is_compressed(name)) {
> +		fd = open(name, O_RDONLY | O_CLOEXEC);
> +		if (fd < 0)
> +			*err = errno;
>  		if (pathname && len > 0)
>  			pathname[0] = '\0';
> -		return open(name, O_RDONLY | O_CLOEXEC);
> +		return fd;
>  	}
>  
>  	fd = mkostemp(tmpbuf, O_CLOEXEC);

[Severity: High]
This isn't a bug introduced by this patch, but the commit message mentions
matching the error handling on the compressed path. Does that compressed
path have an issue where negative custom error codes leak into standard
errno?

If filename__decompress() sets *err to the custom enum value
DSO_LOAD_ERRNO__DECOMPRESSION_FAILURE (which is negative),
dso__get_filename() assigns this directly into standard errno:

tools/perf/util/dso.c:dso__get_filename() {
    ...
    errno = *dso__load_errno(dso);
    ...
}

Then __open_dso() handles the failure by calculating fd = -errno:

tools/perf/util/dso.c:__open_dso() {
    ...
    fd = -errno;
    ...
}

Could this double-negate the custom error code into a positive file
descriptor value (like 9996)?

If open_dso() then incorrectly believes it successfully opened the DSO
because fd >= 0, could it cache this as the active file descriptor?
This might cause close_data_fd() to later close an arbitrary file
descriptor used by another subsystem when perf cleans up.

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

Re: [PATCH 09/13] perf bpf: Validate func_info_rec_size and sub_id in synthesize_bpf_prog_name()

[sashiko-bot]

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

Pre-existing issues:
- [Medium] `prog_tags[sub_id]` is unconditionally accessed, allowing NULL or arbitrary pointer dereferences due to `perf.data` bitmask manipulation.
--

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

perf bpf: Validate func_info_rec_size and sub_id in synthesize_bpf_prog_name()

This commit adds bounds checking for func_info_rec_size and sub_id in
synthesize_bpf_prog_name() to prevent out-of-bounds reads when parsing
untrusted data from perf.data files.

> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
> index c4594969d7677..fe6fbca508c51 100644
> --- a/tools/perf/util/bpf-event.c
> +++ b/tools/perf/util/bpf-event.c
> @@ -143,7 +143,9 @@ static int synthesize_bpf_prog_name(char *buf, int size,
>  	name_len = scnprintf(buf, size, "bpf_prog_");
>  	name_len += snprintf_hex(buf + name_len, size - name_len,
>  				 prog_tags[sub_id], BPF_TAG_SIZE);

[Severity: Medium]
This is a pre-existing issue, but since we are actively hardening this
function against untrusted perf.data input, should we also validate prog_tags
before dereferencing it here?

If an attacker crafts a perf.data file that clears the PERF_BPIL_PROG_TAGS
bit in info_linear->arrays but leaves info->prog_tags set to an arbitrary
64-bit value, the offset translation and bounds checking in
bpil_offs_to_addr() are skipped.

Because the translation is skipped, synthesize_bpf_prog_name() retains the
untrusted value, blindly casts info->prog_tags to a pointer, and dereferences
it unconditionally via prog_tags[sub_id].

Could this lead to an arbitrary memory read or a NULL pointer dereference
crash during parsing?

> -	if (btf) {
> +	if (btf &&
> +	    info->func_info_rec_size >= sizeof(*finfo) &&
> +	    sub_id < info->nr_func_info) {
>  		finfo = func_infos + sub_id * info->func_info_rec_size;
>  		t = btf__type_by_id(btf, finfo->type_id);
>  		if (t)

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

Re: [PATCH 13/13] perf cs-etm: Reject CPU IDs that would overflow signed comparison

[sashiko-bot]

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

Pre-existing issues:
- [High] Integer overflow when allocating the `metadata` array using an unvalidated user-controlled `num_cpu`, leading to a severe heap buffer overflow and arbitrary free on 32-bit systems.
--

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

perf cs-etm: Reject CPU IDs that would overflow signed comparison

This commit validates that the CPU ID from perf.data fits within an int
before performing a signed comparison with max_cpu. This prevents an integer
overflow that could lead to an out-of-bounds access when indexing the
queue_array.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 5e92359f51a7c..0927b0b9c06b1 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> @@ -3468,7 +3469,13 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>  			goto err_free_metadata;
>  		}
>  
> -		if ((int) metadata[j][CS_ETM_CPU] > max_cpu)
> +		/* CPU id comes from perf.data and must fit max_cpu + 1 without overflow */
> +		if (metadata[j][CS_ETM_CPU] >= INT_MAX) {
> +			err = -EINVAL;
> +			goto err_free_metadata;
> +		}
> +

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but since we are looking at error paths and validation in this function, can
the allocation of the metadata array overflow on 32-bit systems?

Earlier in cs_etm__process_auxtrace_info_full(), num_cpu is extracted from
perf.data:

	num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff;
	metadata = zalloc(sizeof(*metadata) * num_cpu);

If a large value like 0x40000001 is provided on a 32-bit architecture, does
sizeof(*metadata) * num_cpu wrap around to a small allocation size?

If metadata is undersized, wouldn't the subsequent parsing loop write out of
bounds:

	for (int j = 0; j < num_cpu; j++) {
		if (ptr[i] == __perf_cs_etmv3_magic) {
			metadata[j] = ...

Additionally, if we take an error path (like the one added in this patch)
and jump to err_free_metadata:

err_free_metadata:
	for (int j = 0; j < num_cpu; j++)
		zfree(&metadata[j]);

does this read out-of-bounds pointers from the undersized metadata array
and pass them to free()?

> +		if ((int)metadata[j][CS_ETM_CPU] > max_cpu)
>  			max_cpu = metadata[j][CS_ETM_CPU];
>  	}
>

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

Re: [PATCH 06/13] perf tools: Use snprintf() for root_dir path construction

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete Fix / Stack Buffer Overflow in `machines__findnew` and `machines__create_guest_kernel_maps`
- [High] Logic Error: Masked initialization failure in `machine__init` leads to crashes.
--

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

    perf tools: Use snprintf() for root_dir path construction
    
    get_kernel_version() in machine.c and dso__load_guest_kernel_sym() in
    symbol.c use sprintf() to construct paths by prepending root_dir to
    "/proc/version" and "/proc/kallsyms" respectively. Both write into
    PATH_MAX stack buffers, but root_dir comes from --guestmount or KVM
    configuration and is not length-checked.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index da1ad58758afd..58fa57e3d1a15 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1336,7 +1336,7 @@ static char *get_kernel_version(const char *root_dir)
>  	char *name, *tmp;
>  	const char *prefix = "Linux version ";
>  
> -	sprintf(version, "%s/proc/version", root_dir);
> +	snprintf(version, sizeof(version), "%s/proc/version", root_dir);

[Severity: High]
This isn't a bug introduced by this patch, but the fix appears to be
incomplete. While this replaces sprintf() with snprintf() for root_dir,
there are two other unpatched sprintf() calls in this file that write
the unchecked symbol_conf.guestmount into a char path[PATH_MAX] buffer:

In machines__findnew():
    sprintf(path, "%s/%d", symbol_conf.guestmount, pid);

In machines__create_guest_kernel_maps():
    sprintf(path, "%s/%s/proc/kallsyms",
            symbol_conf.guestmount,
            namelist[i]->d_name);

Could these also overflow the PATH_MAX stack buffer if a very long string
is provided to the --guestmount option?

[Severity: High]
This is a pre-existing issue, but I noticed a potential logic error in
machine__init() in this file regarding the error path. If an allocation
fails (e.g., strdup(root_dir)), the code jumps to the 'out' label:

out:
	if (err) {
		zfree(&machine->kmaps);
		zfree(&machine->root_dir);
		zfree(&machine->mmap_name);
	}
	return 0;

Does this intentionally return 0 instead of err? It appears this masks
the initialization failure from callers like machines__add(), leading
to crashes later when they use the partially uninitialized machine struct.

>  	file = fopen(version, "r");
>  	if (!file)
>  		return NULL;

[ ... ]

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