[PATCHES v2 00/11] perf tools: Assorted fixes

[PATCHES v2 00/11] perf tools: Assorted fixes

[Arnaldo Carvalho de Melo]

Hi,

Sixth batch of pre-existing bug fixes found by sashiko-bot AI review
during the perf-data-validation hardening series.  All bugs are latent
in existing code — none were introduced by the hardening patches.

Three broad categories:

1. snprintf() accumulation overflows (patches 2, 9, 10, 11):

   Several functions accumulate formatted output via ret += snprintf().
   snprintf() returns the would-have-been-written count, so on truncation
   ret overshoots the buffer size and the next 'size - ret' underflows
   to a huge unsigned value, disabling bounds checking.  Switched to
   scnprintf() which returns actual bytes written.

   Affected: cpu_map__snprint(), snprintf_hex(),
   synthesize_bpf_prog_name(), hists__scnprintf_title(),
   build_id__snprintf(), hwmon_pmu__describe_items().

2. Missing safety checks on untrusted data (patches 1, 3, 6, 7):

   - get_max_num(): size_t underflow on empty sysfs file causes heap
     over-read.
   - machine__resolve(): unguarded env->cpu[] access with untrusted
     CPU index — switched to perf_env__get_cpu_topology() accessor.
   - timehist: test_bit(prio, ...) without bounds check on untrusted
     tracepoint priority.
   - idle-hist: rb_first_cached() on a tree populated with plain
     rb_insert_color() — rb_leftmost never set, callchains silently
     dropped.

3. Resource hygiene (patches 4, 5, 8):

   - mbind() bitmap allocation one bit short of what kernel reads.
   - bitmap_free() without NULLing the pointer (3 call sites).
   - O_CLOEXEC missing from open() calls in DSO and ELF code
     (12 call sites across 2 files).

Also expanded the libperf ABI TODO (tools/lib/perf/TODO) to emphasize
the code simplification argument for widening struct perf_cpu.cpu from
int16_t to int — the narrow type forces defensive truncation checks at
every boundary where wider CPU indices are narrowed.

 tools/lib/perf/TODO          |  7 +++++++
 tools/perf/builtin-record.c  |  1 +
 tools/perf/builtin-sched.c   |  7 +++++--
 tools/perf/util/bpf-event.c  | 11 ++++++-----
 tools/perf/util/build-id.c   |  2 +-
 tools/perf/util/cpumap.c     | 24 +++++++++++++++---------
 tools/perf/util/dso.c        |  4 ++--
 tools/perf/util/event.c      |  9 +++++++--
 tools/perf/util/header.c     |  4 +++-
 tools/perf/util/hist.c       |  7 ++++---
 tools/perf/util/hwmon_pmu.c  | 12 ++++++------
 tools/perf/util/mmap.c       |  4 +++-
 tools/perf/util/symbol-elf.c | 20 ++++++++++----------
 13 files changed, 70 insertions(+), 42 deletions(-)

Changes since v1:
  - Patch 6: fix prio bounds-check logic — the v1 condition
    (prio < 0 || prio >= MAX_PRIO || !test_bit(...)) incorrectly
    skipped events with unknown priority (prio == -1).  Changed to
    (prio >= 0 && (prio >= MAX_PRIO || !test_bit(...))) to preserve
    the original pass-through for events without priority info.
    (Found by sashiko-bot lore review)


Thanks,

- Arnaldo

[PATCH 01/11] perf tools: Fix get_max_num() size_t underflow on empty sysfs file

[Arnaldo Carvalho de Melo]

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

get_max_num() reads a sysfs file (cpu/possible, cpu/present, or
node/possible) and scans backward from the end to find the last
number.  If the file is empty, filename__read_str() returns num == 0.
The loop `while (--num)` decrements the size_t from 0 to SIZE_MAX,
reading backward across the heap until a comma or hyphen is found
or unmapped memory is hit.

Add an early return for empty files before the backward scan.

Fixes: 7780c25bae59fd04 ("perf tools: Allow ability to map cpus to nodes easily")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Don Zickus <dzickus@redhat.com>
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/cpumap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 21fa781b03cc7409..1fab00ec4a59a0c7 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -448,6 +448,12 @@ static int get_max_num(char *path, int *max)
 
 	buf[num] = '\0';
 
+	/* empty file — nothing to parse */
+	if (num == 0) {
+		err = -1;
+		goto out;
+	}
+
 	/* start on the right, to find highest node num */
 	while (--num) {
 		if ((buf[num] == ',') || (buf[num] == '-')) {
-- 
2.54.0

Re: [PATCH 01/11] perf tools: Fix get_max_num() size_t underflow on empty sysfs file

[sashiko-bot]

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

Pre-existing issues:
- [High] Heap out-of-bounds write (poison null byte) due to size_t underflow when reading empty files
--

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

perf tools: Fix get_max_num() size_t underflow on empty sysfs file

This commit fixes a size_t underflow in get_max_num() by adding an early
return when a sysfs file is empty. Previously, reading an empty file would
result in num == 0, causing a backward scan to underflow and read
out-of-bounds memory.

> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -448,6 +448,12 @@ static int get_max_num(char *path, int *max)
>  
>  	buf[num] = '\0';
>  
> +	/* empty file — nothing to parse */
> +	if (num == 0) {
> +		err = -1;
> +		goto out;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but while you are fixing
empty file reads here, does this same size_t underflow pattern occur in
other places that read strings?

For instance, in tools/perf/util/pmu.c:pmu_id():

	if (filename__read_str(path, &str, &len) < 0)
		return NULL;

	str[len - 1] = 0; /* remove line feed */

If len is 0 because the identifier file is completely empty, len - 1
underflows to SIZE_MAX. Since the string is heap-allocated and returned
with a length of 0, writing to index SIZE_MAX effectively writes one byte
before the string, which corrupts the malloc chunk size metadata.

A similar pattern seems to exist in
tools/perf/util/thread.c:thread__set_comm_from_proc():

	if (procfs__read_str(path, &comm, &sz) == 0) {
		comm[sz - 1] = '\0';
		err = thread__set_comm(thread, comm, 0);
	}

Could these empty file underflows trigger similar heap corruption if the
identifier or comm files happen to be empty?

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

[PATCH 02/11] perf tools: Use scnprintf() in cpu_map__snprint() to prevent overflow

[Arnaldo Carvalho de Melo]

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

cpu_map__snprint() accumulates snprintf() return values in ret.
snprintf() returns the number of characters that *would have been
written* on truncation, not the actual count.  When a fragmented CPU
list exceeds the buffer, ret grows past size, causing `size - ret` to
underflow (both are size_t), and subsequent snprintf() calls write
past the end of the caller's stack buffer.

Switch to scnprintf() which returns the actual number of characters
written, making ret accumulation safe by construction.

Fixes: a24020e6b7cf6eb8 ("perf tools: Change cpu_map__fprintf output")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
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/cpumap.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 1fab00ec4a59a0c7..23ebe9b97f8e58af 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -692,21 +692,21 @@ size_t cpu_map__snprint(struct perf_cpu_map *map, char *buf, size_t size)
 		if (start == -1) {
 			start = i;
 			if (last) {
-				ret += snprintf(buf + ret, size - ret,
-						"%s%d", COMMA,
-						perf_cpu_map__cpu(map, i).cpu);
+				ret += scnprintf(buf + ret, size - ret,
+						 "%s%d", COMMA,
+						 perf_cpu_map__cpu(map, i).cpu);
 			}
 		} else if (((i - start) != (cpu.cpu - perf_cpu_map__cpu(map, start).cpu)) || last) {
 			int end = i - 1;
 
 			if (start == end) {
-				ret += snprintf(buf + ret, size - ret,
-						"%s%d", COMMA,
-						perf_cpu_map__cpu(map, start).cpu);
+				ret += scnprintf(buf + ret, size - ret,
+						 "%s%d", COMMA,
+						 perf_cpu_map__cpu(map, start).cpu);
 			} else {
-				ret += snprintf(buf + ret, size - ret,
-						"%s%d-%d", COMMA,
-						perf_cpu_map__cpu(map, start).cpu, perf_cpu_map__cpu(map, end).cpu);
+				ret += scnprintf(buf + ret, size - ret,
+						 "%s%d-%d", COMMA,
+						 perf_cpu_map__cpu(map, start).cpu, perf_cpu_map__cpu(map, end).cpu);
 			}
 			first = false;
 			start = i;
-- 
2.54.0

[PATCH 03/11] perf tools: Use perf_env__get_cpu_topology() in machine__resolve()

[Arnaldo Carvalho de Melo]

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

machine__resolve() accesses env->cpu[al->cpu].socket_id after checking
al->cpu >= 0 and env->cpu != NULL, but without validating al->cpu
against env->nr_cpus_avail.  Since al->cpu comes from the untrusted
perf.data sample, a crafted file with a large CPU index causes an
out-of-bounds heap read.

Use perf_env__get_cpu_topology() which validates both NULL and bounds.

Fixes: 0c4c4debb0adda4c ("perf tools: Add processor socket info to hist_entry and addr_location")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Kan Liang <kan.liang@intel.com>
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/event.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 66f4843bb235df53..66293fea64fde9fd 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -14,6 +14,7 @@
 #include <linux/perf_event.h>
 #include "cpumap.h"
 #include "dso.h"
+#include "env.h"
 #include "event.h"
 #include "debug.h"
 #include "hist.h"
@@ -835,9 +836,13 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
 
 	if (al->cpu >= 0) {
 		struct perf_env *env = machine->env;
+		struct cpu_topology_map *topo;
 
-		if (env && env->cpu)
-			al->socket = env->cpu[al->cpu].socket_id;
+		if (env) {
+			topo = perf_env__get_cpu_topology(env, (struct perf_cpu){ al->cpu });
+			if (topo)
+				al->socket = topo->socket_id;
+		}
 	}
 
 	/* Account for possible out-of-order switch events. */
-- 
2.54.0

Re: [PATCH 03/11] perf tools: Use perf_env__get_cpu_topology() in machine__resolve()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Silent truncation of a 32-bit CPU index (`al->cpu`) down to a 16-bit `struct perf_cpu` bypasses intended bounds validation for malformed inputs.
--

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

perf tools: Use perf_env__get_cpu_topology() in machine__resolve()

This commit updates machine__resolve() to use perf_env__get_cpu_topology(),
replacing direct array access to prevent out-of-bounds heap reads from
untrusted perf.data samples.

> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c

[ ... ]

> @@ -835,9 +836,13 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
>  
>  	if (al->cpu >= 0) {
>  		struct perf_env *env = machine->env;
> +		struct cpu_topology_map *topo;
>  
> -		if (env && env->cpu)
> -			al->socket = env->cpu[al->cpu].socket_id;
> +		if (env) {
> +			topo = perf_env__get_cpu_topology(env, (struct perf_cpu){ al->cpu });

[Severity: Medium]
Does this cast silently truncate the 32-bit al->cpu?

Since al->cpu is an s32 from the untrusted sample, and struct perf_cpu uses a
16-bit integer for the cpu index, an attacker providing an index like 65536
would truncate it to 0. Wouldn't this bypass the downstream bounds validation
and falsely associate the sample with CPU 0's topology?

Also, for systems with more than 32,767 CPUs, wouldn't this wrap to a
negative number and incorrectly fail the bounds check?

> +			if (topo)
> +				al->socket = topo->socket_id;
> +		}
>  	}
>  
>  	/* Account for possible out-of-order switch events. */

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

[PATCH 04/11] perf mmap: Fix mbind() maxnode vs bitmap allocation mismatch in aio_bind

[Arnaldo Carvalho de Melo]

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

perf_mmap__aio_bind() allocates a node mask bitmap with
bitmap_zalloc(node_index + 1) bits, but passes node_index + 2 as the
maxnode argument to mbind().  The mbind syscall interprets maxnode as
the number of bits to read from the mask.

When node_index + 2 crosses a BITS_PER_LONG boundary (e.g.
node_index = 63 on 64-bit), the bitmap occupies 8 bytes but mbind
reads 16 — an out-of-bounds read of user heap memory into kernel
space.

Allocate node_index + 2 bits to match what mbind will actually read.

Fixes: 44d462acc0bf3eab ("perf record: Fix binding of AIO user space buffers to nodes")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Alexey Budankov <alexey.budankov@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/mmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
index d64aec6c7c843e81..8012301d3cf2ac9a 100644
--- a/tools/perf/util/mmap.c
+++ b/tools/perf/util/mmap.c
@@ -113,7 +113,8 @@ static int perf_mmap__aio_bind(struct mmap *map, int idx, struct perf_cpu cpu, i
 		if (node < 0)
 			return 0;
 		node_index = node;
-		node_mask = bitmap_zalloc(node_index + 1);
+		/* mbind's maxnode is node_index + 2 — allocate to match */
+		node_mask = bitmap_zalloc(node_index + 2);
 		if (!node_mask) {
 			pr_err("Failed to allocate node mask for mbind: error %m\n");
 			return -1;
-- 
2.54.0

[PATCH 05/11] perf tools: NULL bitmap pointers after bitmap_free()

[Arnaldo Carvalho de Melo]

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

Three call sites free bitmaps without NULLing the pointer, risking
double-free if the structure is reused or cleanup is called twice:

 - mmap__munmap():                 map->affinity_mask.bits
 - record__mmap_cpu_mask_free():   mask->bits
 - memory_node__delete_nodes():    nodesp[i].set

Set each pointer to NULL after bitmap_free().

Fixes: 8384a2600c7ddfc8 ("perf record: Adapt affinity to machines with #CPUs > 1K")
Fixes: f466e5ed6c356d1d ("perf record: Extend --threads command line option")
Fixes: 36d8658618c2505f ("perf header: Validate bitmap size before allocating in do_read_bitmap()")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Alexey Budankov <alexey.budankov@linux.intel.com>
Cc: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 1 +
 tools/perf/util/header.c    | 4 +++-
 tools/perf/util/mmap.c      | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index a33c78f030d91012..e915390556752b9e 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -3084,6 +3084,7 @@ static int record__mmap_cpu_mask_alloc(struct mmap_cpu_mask *mask, int nr_bits)
 static void record__mmap_cpu_mask_free(struct mmap_cpu_mask *mask)
 {
 	bitmap_free(mask->bits);
+	mask->bits = NULL;
 	mask->nbits = 0;
 }
 
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index d7f41db7322cbcb4..8d2ab440a1c8ee4a 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1481,8 +1481,10 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
 
 static void memory_node__delete_nodes(struct memory_node *nodesp, u64 cnt)
 {
-	for (u64 i = 0; i < cnt; i++)
+	for (u64 i = 0; i < cnt; i++) {
 		bitmap_free(nodesp[i].set);
+		nodesp[i].set = NULL;
+	}
 
 	free(nodesp);
 }
diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
index 8012301d3cf2ac9a..ee3ebdf53e15291e 100644
--- a/tools/perf/util/mmap.c
+++ b/tools/perf/util/mmap.c
@@ -239,6 +239,7 @@ static void perf_mmap__aio_munmap(struct mmap *map __maybe_unused)
 void mmap__munmap(struct mmap *map)
 {
 	bitmap_free(map->affinity_mask.bits);
+	map->affinity_mask.bits = NULL;
 
 	zstd_fini(&map->zstd_data);
 
-- 
2.54.0

Re: [PATCH 05/11] perf tools: NULL bitmap pointers after bitmap_free()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Setting `nodesp[i].set = NULL` in `memory_node__delete_nodes` is a dead store and fails to prevent double-frees because the parent `nodesp` array is immediately freed.
--

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

perf tools: NULL bitmap pointers after bitmap_free()

This commit modifies three call sites to set bitmap pointers to NULL after
they are freed using bitmap_free(). The purpose is to avoid double-free
scenarios if the structure is later reused or cleaned up multiple times.

[ ... ]

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index d7f41db7322cb..8d2ab440a1c8e 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1481,8 +1481,10 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
>  
>  static void memory_node__delete_nodes(struct memory_node *nodesp, u64 cnt)
>  {
> -	for (u64 i = 0; i < cnt; i++)
> +	for (u64 i = 0; i < cnt; i++) {
>  		bitmap_free(nodesp[i].set);
> +		nodesp[i].set = NULL;

[Severity: Low]
Since the entire nodesp array is freed immediately after this loop, does
nullifying this pointer provide any protection against double-frees?

If a cleanup function were called twice on this structure, wouldn't it result
in a use-after-free on the nodesp array itself before the set pointer could
even be evaluated?

> +	}
>  
>  	free(nodesp);
>  }

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

[PATCH 06/11] perf sched: Bounds-check prio before test_bit() in timehist

[Arnaldo Carvalho de Melo]

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

timehist_skip_sample() reads prio from untrusted tracepoint data via
perf_sample__intval(sample, "prev_prio") without bounds validation.
A crafted perf.data with prev_prio >= MAX_PRIO (140) causes test_bit()
to read past the end of the prio_bitmap, which is only MAX_PRIO bits.

Add a prio >= 0 && prio < MAX_PRIO check before the test_bit() call.
This also makes the != -1 sentinel check explicit as >= 0.

Fixes: 9b3a48bbe20d9692 ("perf sched timehist: Add --prio option")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Yang Jihong <yangjihong@bytedance.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/perf/TODO        | 7 +++++++
 tools/perf/builtin-sched.c | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/lib/perf/TODO b/tools/lib/perf/TODO
index 486dd95dc57208a8..1a3644aa1f38dde4 100644
--- a/tools/lib/perf/TODO
+++ b/tools/lib/perf/TODO
@@ -11,6 +11,13 @@ together.
      (x86_64 max is 8192, arm64 is 4096), but NR_CPUS limits keep
      growing.  perf clamps to INT16_MAX in set_max_cpu_num() as a
      safety net.
+   - Code simplification: the int16_t forces defensive truncation
+     checks at every boundary where a wider CPU index (int from
+     sample->cpu, al->cpu, etc.) is narrowed into struct perf_cpu.
+     Without these checks, values > 32767 silently wrap to small
+     positive numbers, bypassing bounds validation.  Widening to int
+     eliminates this entire class of silent truncation bugs and
+     removes the need for the INT16_MAX clamp in set_max_cpu_num().
    - Scope: struct perf_cpu is embedded everywhere — perf_cpu_map__cpu(),
      perf_cpu_map__min(), perf_cpu_map__max(), perf_cpu_map__has(), the
      for_each_cpu macros, and all internal callers.  The perf_cpu_map
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 1ff01f03d2ad1ad3..ded511d8518803a0 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -2645,7 +2645,9 @@ static bool timehist_skip_sample(struct perf_sched *sched,
 		else if (evsel__name_is(sample->evsel, "sched:sched_switch"))
 			prio = perf_sample__intval(sample, "prev_prio");
 
-		if (prio != -1 && !test_bit(prio, sched->prio_bitmap)) {
+		/* prio comes from untrusted tracepoint data — bounds-check before test_bit */
+		if (prio >= 0 &&
+		    (prio >= MAX_PRIO || !test_bit(prio, sched->prio_bitmap))) {
 			rc = true;
 			sched->skipped_samples++;
 		}
-- 
2.54.0

Re: [PATCH 06/11] perf sched: Bounds-check prio before test_bit() in timehist

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Integer truncation of `sample->cpu` into `int16_t` bypasses bounds checks and silently misattributes data
- [Medium] perf sched subcommands fail to register required API callbacks (`.attr` and `.feature`) for pipe mode
--

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

perf sched: Bounds-check prio before test_bit() in timehist
    
timehist_skip_sample() reads prio from untrusted tracepoint data via
perf_sample__intval(sample, "prev_prio") without bounds validation.
A crafted perf.data with prev_prio >= MAX_PRIO (140) causes test_bit()
to read past the end of the prio_bitmap, which is only MAX_PRIO bits.
Add a prio >= 0 && prio < MAX_PRIO check before the test_bit() call.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 1ff01f03d2ad1..ded511d851880 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2645,7 +2645,9 @@ static bool timehist_skip_sample(struct perf_sched *sched,
>  		else if (evsel__name_is(sample->evsel, "sched:sched_switch"))
>  			prio = perf_sample__intval(sample, "prev_prio");
>  
> -		if (prio != -1 && !test_bit(prio, sched->prio_bitmap)) {
> +		/* prio comes from untrusted tracepoint data — bounds-check before test_bit */
> +		if (prio >= 0 &&
> +		    (prio >= MAX_PRIO || !test_bit(prio, sched->prio_bitmap))) {
>  			rc = true;
>  			sched->skipped_samples++;
>  		}

[Severity: Medium]
This is a pre-existing issue, but I noticed that the sample->cpu value is
narrowed to a 16-bit integer during struct initialization in map_switch_event()
and perf_timehist__process_sample() in tools/perf/builtin-sched.c.

Does this narrowing regression allow wrapped out-of-bounds cpu values to bypass
the subsequent validation check?

For example in map_switch_event():

	struct perf_cpu this_cpu = {
		.cpu = sample->cpu,
	};

	/* perf.data is untrusted input — CPU may be absent or corrupted */
	if (this_cpu.cpu >= MAX_CPUS || this_cpu.cpu < 0) {

If sample->cpu is larger than 32767, it will silently wrap, passing the bounds
check but potentially misattributing data. A similar pattern exists in
perf_timehist__process_sample().

[Severity: Medium]
This isn't a bug introduced by this patch, but looking at cmd_sched() and
perf_sched__timehist() in tools/perf/builtin-sched.c, it appears the perf sched
subcommands fail to register required API callbacks like .attr and .feature
for pipe mode.

In cmd_sched():

	perf_tool__init(&sched.tool, /*ordered_events=*/true);
	sched.tool.sample	 = perf_sched__process_tracepoint_sample;
	sched.tool.comm		 = perf_sched__process_comm;
	sched.tool.namespaces	 = perf_event__process_namespaces;
	sched.tool.lost		 = perf_event__process_lost;
	sched.tool.fork		 = perf_sched__process_fork_event;

And in perf_sched__timehist():

	sched->tool.lost	 = process_lost;
	sched->tool.attr	 = perf_event__process_attr;
	sched->tool.tracing_data = perf_event__process_tracing_data;
	sched->tool.build_id	 = perf_event__process_build_id;

Without the .attr callback, no evsel structures are created, and without the
.feature callback, perf_env remains unpopulated in pipe mode.

Does this omission lead to a regression where dropped events or uninitialized
system metadata occur when processing pipelined inputs?

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

[PATCH 07/11] perf sched: Fix idle-hist callchain display using wrong rb_first variant

[Arnaldo Carvalho de Melo]

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

timehist_print_idlehist_callchain() calls rb_first_cached() on
sorted_root, but the sort function (callchain_param.sort) populates it
via rb_insert_color() on the plain rb_root member — not the cached
variant.  This means rb_leftmost is never set, so rb_first_cached()
always returns NULL and the entire callchain summary is silently
dropped from --idle-hist output.

Use rb_first(&root->rb_root) to match how the tree was populated.

Fixes: ba957ebb54893aca ("perf sched timehist: Show callchains for idle stat")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
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/builtin-sched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index ded511d8518803a0..85f11d388392d316 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3130,7 +3130,8 @@ static size_t timehist_print_idlehist_callchain(struct rb_root_cached *root)
 	size_t ret = 0;
 	FILE *fp = stdout;
 	struct callchain_node *chain;
-	struct rb_node *rb_node = rb_first_cached(root);
+	/* sort() uses rb_insert_color() on rb_root, not rb_root_cached */
+	struct rb_node *rb_node = rb_first(&root->rb_root);
 
 	printf("  %16s  %8s  %s\n", "Idle time (msec)", "Count", "Callchains");
 	printf("  %.16s  %.8s  %.50s\n", graph_dotted_line, graph_dotted_line,
-- 
2.54.0

[PATCH 08/11] perf tools: Add O_CLOEXEC to open() calls in DSO and ELF code

[Arnaldo Carvalho de Melo]

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

open() calls in dso.c and symbol-elf.c omit O_CLOEXEC, which leaks
file descriptors to child processes spawned during symbol resolution
(e.g., addr2line, objdump).  This can exhaust the fd limit during
long profiling sessions or when processing many DSOs.

Add O_CLOEXEC to all open() calls in both files (12 call sites).

Fixes: cdd059d731eeb466 ("perf tools: Move dso_* related functions into dso object")
Fixes: e5a1845fc0aeca85 ("perf symbols: Split out util/symbol-elf.c")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
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        |  4 ++--
 tools/perf/util/symbol-elf.c | 20 ++++++++++----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 7dced896c64eafd7..fb2e78fe2aa8eb94 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -344,7 +344,7 @@ int filename__decompress(const char *name, char *pathname,
 	 * descriptor to the uncompressed file.
 	 */
 	if (!compressions[comp].is_compressed(name))
-		return open(name, O_RDONLY);
+		return open(name, O_RDONLY | O_CLOEXEC);
 
 	fd = mkstemp(tmpbuf);
 	if (fd < 0) {
@@ -1911,7 +1911,7 @@ static const u8 *__dso__read_symbol(struct dso *dso, const char *symfs_filename,
 	int saved_errno;
 
 	nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
-	fd = open(symfs_filename, O_RDONLY);
+	fd = open(symfs_filename, O_RDONLY | O_CLOEXEC);
 	saved_errno = errno;
 	nsinfo__mountns_exit(&nsc);
 	if (fd < 0) {
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 186e6d92ac3d7742..c2bdfd0003df2abe 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -217,7 +217,7 @@ bool filename__has_section(const char *filename, const char *sec)
 	GElf_Shdr shdr;
 	bool found = false;
 
-	fd = open(filename, O_RDONLY);
+	fd = open(filename, O_RDONLY | O_CLOEXEC);
 	if (fd < 0)
 		return false;
 
@@ -872,7 +872,7 @@ static int read_build_id(const char *filename, struct build_id *bid)
 	if (size < BUILD_ID_SIZE)
 		goto out;
 
-	fd = open(filename, O_RDONLY);
+	fd = open(filename, O_RDONLY | O_CLOEXEC);
 	if (fd < 0)
 		goto out;
 
@@ -935,7 +935,7 @@ int sysfs__read_build_id(const char *filename, struct build_id *bid)
 	size_t size = sizeof(bid->data);
 	int fd, err = -1;
 
-	fd = open(filename, O_RDONLY);
+	fd = open(filename, O_RDONLY | O_CLOEXEC);
 	if (fd < 0)
 		goto out;
 
@@ -995,7 +995,7 @@ int filename__read_debuglink(const char *filename, char *debuglink,
 	if (err >= 0)
 		goto out;
 
-	fd = open(filename, O_RDONLY);
+	fd = open(filename, O_RDONLY | O_CLOEXEC);
 	if (fd < 0)
 		goto out;
 
@@ -1153,7 +1153,7 @@ int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name,
 
 		type = dso__symtab_type(dso);
 	} else {
-		fd = open(name, O_RDONLY);
+		fd = open(name, O_RDONLY | O_CLOEXEC);
 		if (fd < 0) {
 			*dso__load_errno(dso) = errno;
 			return -1;
@@ -1952,7 +1952,7 @@ static int kcore__open(struct kcore *kcore, const char *filename)
 {
 	GElf_Ehdr *ehdr;
 
-	kcore->fd = open(filename, O_RDONLY);
+	kcore->fd = open(filename, O_RDONLY | O_CLOEXEC);
 	if (kcore->fd == -1)
 		return -1;
 
@@ -1985,7 +1985,7 @@ static int kcore__init(struct kcore *kcore, char *filename, int elfclass,
 	if (temp)
 		kcore->fd = mkstemp(filename);
 	else
-		kcore->fd = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0400);
+		kcore->fd = open(filename, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0400);
 	if (kcore->fd == -1)
 		return -1;
 
@@ -2461,11 +2461,11 @@ static int kcore_copy__compare_files(const char *from_filename,
 {
 	int from, to, err = -1;
 
-	from = open(from_filename, O_RDONLY);
+	from = open(from_filename, O_RDONLY | O_CLOEXEC);
 	if (from < 0)
 		return -1;
 
-	to = open(to_filename, O_RDONLY);
+	to = open(to_filename, O_RDONLY | O_CLOEXEC);
 	if (to < 0)
 		goto out_close_from;
 
@@ -2883,7 +2883,7 @@ int get_sdt_note_list(struct list_head *head, const char *target)
 	Elf *elf;
 	int fd, ret;
 
-	fd = open(target, O_RDONLY);
+	fd = open(target, O_RDONLY | O_CLOEXEC);
 	if (fd < 0)
 		return -EBADF;
 
-- 
2.54.0

Re: [PATCH 08/11] perf tools: Add O_CLOEXEC to open() calls in DSO and ELF code

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch incompletely fixes the file descriptor leak because it misses multiple mkstemp() calls that do not set O_CLOEXEC.
--

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

perf tools: Add O_CLOEXEC to open() calls in DSO and ELF code

This patch adds O_CLOEXEC to open() calls in dso.c and symbol-elf.c to
prevent file descriptors from leaking to child processes spawned during
symbol resolution. This aims to prevent fd exhaustion during long
profiling sessions or when processing many DSOs.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 7dced896c64ea..fb2e78fe2aa8e 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -344,7 +344,7 @@ int filename__decompress(const char *name, char *pathname,
>  	 * descriptor to the uncompressed file.
>  	 */
>  	if (!compressions[comp].is_compressed(name))
> -		return open(name, O_RDONLY);
> +		return open(name, O_RDONLY | O_CLOEXEC);
>  
>  	fd = mkstemp(tmpbuf);

[Severity: High]
This is a pre-existing issue, but does mkstemp() also leak file descriptors
to child processes?

Since mkstemp() does not automatically set the O_CLOEXEC flag, the file
descriptor created here for the decompressed kernel module might still remain
open and leak to child processes (like addr2line) spawned during symbol
resolution.

Should a subsequent fcntl(fd, F_SETFD, FD_CLOEXEC) be added to fully resolve
the file descriptor exhaustion issue this patch targets?

>  	if (fd < 0) {

[ ... ]

> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 186e6d92ac3d7..c2bdfd0003df2 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c

[ ... ]

> @@ -1985,7 +1985,7 @@ static int kcore__init(struct kcore *kcore, char *filename, int elfclass,
>  	if (temp)
>  		kcore->fd = mkstemp(filename);

[Severity: High]
This is a pre-existing issue, but this mkstemp() call also creates a
temporary file for kcore without setting O_CLOEXEC.

Additionally, looking at read_gnu_debugdata() in the same file, there is
another call to mkstemp() used for extracted .gnu_debugdata:

tools/perf/util/symbol-elf.c:read_gnu_debugdata() {
    ...
    temp_fd = mkstemp(temp_filename);
    ...
}

Do these mkstemp() descriptors also need to have the FD_CLOEXEC flag set to
prevent them from leaking to child processes?

>  	else
> -		kcore->fd = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0400);
> +		kcore->fd = open(filename, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0400);
>  	if (kcore->fd == -1)
>  		return -1;

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

[PATCH 09/11] perf bpf: Use scnprintf() in snprintf_hex() and synthesize_bpf_prog_name()

[Arnaldo Carvalho de Melo]

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

Both functions accumulate formatted output via ret += snprintf(buf + ret,
size - ret, ...).  If the buffer is too small and snprintf() returns more
than the remaining space, ret exceeds size and the next 'size - ret'
underflows, causing snprintf() to write past the buffer end.

Switch to scnprintf() which returns the actual number of bytes written,
making the accumulation safe.

Fixes: 7b612e291a5affb1 ("perf tools: Synthesize PERF_RECORD_* for loaded BPF programs")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Song Liu <song@kernel.org>
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 | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
index a27945c279efb779..2c09842469f1f28c 100644
--- a/tools/perf/util/bpf-event.c
+++ b/tools/perf/util/bpf-event.c
@@ -36,7 +36,7 @@ static int snprintf_hex(char *buf, size_t size, unsigned char *data, size_t len)
 	size_t i;
 
 	for (i = 0; i < len; i++)
-		ret += snprintf(buf + ret, size - ret, "%02x", data[i]);
+		ret += scnprintf(buf + ret, size - ret, "%02x", data[i]);
 	return ret;
 }
 
@@ -140,7 +140,7 @@ static int synthesize_bpf_prog_name(char *buf, int size,
 	const struct btf_type *t;
 	int name_len;
 
-	name_len = snprintf(buf, size, "bpf_prog_");
+	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) {
@@ -153,9 +153,10 @@ static int synthesize_bpf_prog_name(char *buf, int size,
 			short_name = info->name;
 	} else
 		short_name = "F";
-	if (short_name)
-		name_len += snprintf(buf + name_len, size - name_len,
-				     "_%s", short_name);
+	if (short_name) {
+		name_len += scnprintf(buf + name_len, size - name_len,
+				      "_%s", short_name);
+	}
 	return name_len;
 }
 
-- 
2.54.0

[PATCH 10/11] perf hists: Fix snprintf() in hists__scnprintf_title() UID filter path

[Arnaldo Carvalho de Melo]

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

hists__scnprintf_title() accumulates formatted output into a buffer
using scnprintf() for all filter clauses except the UID filter, which
uses snprintf().  If the buffer fills up and snprintf() returns more
than the remaining space, printed exceeds size and the next 'size -
printed' underflows, causing later scnprintf() calls to write past
the buffer.

Switch the UID filter clause to scnprintf() to match the rest of the
function.

Fixes: 25c312dbf88ca402 ("perf hists: Move hists__scnprintf_title() away from the TUI code")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 811d68fa6770c5b7..df978c996b6c2262 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2963,9 +2963,10 @@ int __hists__scnprintf_title(struct hists *hists, char *bf, size_t size, bool sh
 			   ev_name, sample_freq_str, enable_ref ? ref : " ", nr_events);
 
 
-	if (hists->uid_filter_str)
-		printed += snprintf(bf + printed, size - printed,
-				    ", UID: %s", hists->uid_filter_str);
+	if (hists->uid_filter_str) {
+		printed += scnprintf(bf + printed, size - printed,
+				     ", UID: %s", hists->uid_filter_str);
+	}
 	if (thread) {
 		if (hists__has(hists, thread)) {
 			printed += scnprintf(bf + printed, size - printed,
-- 
2.54.0

Re: [PATCH 10/11] perf hists: Fix snprintf() in hists__scnprintf_title() UID filter path

[sashiko-bot]

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

Pre-existing issues:
- [High] Stack buffer overflow in perf_pmu__for_each_event due to unchecked snprintf accumulation.
--

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

perf hists: Fix snprintf() in hists__scnprintf_title() UID filter path

This patch fixes a potential buffer overflow by switching an unchecked
snprintf call to scnprintf. The original code accumulated the return value
of snprintf, which could underflow the remaining size on subsequent calls.

> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 811d68fa6770c..df978c996b6c2 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -2963,9 +2963,10 @@ int __hists__scnprintf_title(struct hists *hists, char *bf, size_t size, bool sh
>  			   ev_name, sample_freq_str, enable_ref ? ref : " ", nr_events);
>  
>  
> -	if (hists->uid_filter_str)
> -		printed += snprintf(bf + printed, size - printed,
> -				    ", UID: %s", hists->uid_filter_str);
> +	if (hists->uid_filter_str) {
> +		printed += scnprintf(bf + printed, size - printed,
> +				     ", UID: %s", hists->uid_filter_str);
> +	}

[Severity: High]
This is a pre-existing issue, but while you are fixing this unchecked
snprintf accumulation here, there appears to be another instance of the
exact same pattern in tools/perf/util/pmu.c:perf_pmu__for_each_event().

When accumulating buf_used across consecutive snprintf calls, if the first
formatted string is large enough:

tools/perf/util/pmu.c:perf_pmu__for_each_event() {
	...
	char buf[1024];
	...
	buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
			     "%G%s", event->scale, event->unit) + 1;
	...
	buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
			     "%.*s/%s/", (int)pmu_name_len, info.pmu_name,
			     event->terms) + 1;
	...
}

If the first snprintf returns a value causing buf_used to exceed
sizeof(buf), the second snprintf will execute with an underflowed limit
and an out-of-bounds destination pointer.

Could this write past the stack buffer if a crafted PMU sysfs entry is
encountered?

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

[PATCH 11/11] perf tools: Use scnprintf() in build_id__snprintf() and hwmon read_events()

[Arnaldo Carvalho de Melo]

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

build_id__snprintf() and hwmon_pmu__read_events() accumulate formatted
output via snprintf(), which returns the would-have-been-written count
on truncation.  In build_id__snprintf(), this inflates the return
value beyond the buffer size.  In hwmon_pmu__read_events(), len
overshoots out_buf_len and the next 'out_buf_len - len' underflows.

Switch both to scnprintf() which returns actual bytes written.

Fixes: fccaaf6fbbc59910 ("perf build-id: Change sprintf functions to snprintf")
Fixes: 53cc0b351ec99278 ("perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
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/build-id.c  |  2 +-
 tools/perf/util/hwmon_pmu.c | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 8c0a9ae932aa5798..a3b92108f96263c6 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -94,7 +94,7 @@ int build_id__snprintf(const struct build_id *build_id, char *bf, size_t bf_size
 	}
 
 	for (size_t i = 0; i < build_id->size && offs < bf_size; ++i)
-		offs += snprintf(bf + offs, bf_size - offs, "%02x", build_id->data[i]);
+		offs += scnprintf(bf + offs, bf_size - offs, "%02x", build_id->data[i]);
 
 	return offs;
 }
diff --git a/tools/perf/util/hwmon_pmu.c b/tools/perf/util/hwmon_pmu.c
index fb3ffa8d32ad2a93..dbf6a71af47f9a42 100644
--- a/tools/perf/util/hwmon_pmu.c
+++ b/tools/perf/util/hwmon_pmu.c
@@ -442,12 +442,12 @@ static size_t hwmon_pmu__describe_items(struct hwmon_pmu *hwm, char *out_buf, si
 
 				buf[read_len] = '\0';
 				val = strtoll(buf, /*endptr=*/NULL, 10);
-				len += snprintf(out_buf + len, out_buf_len - len, "%s%s%s=%g%s",
-						len == 0 ? " " : ", ",
-						hwmon_item_strs[bit],
-						is_alarm ? "_alarm" : "",
-						(double)val / 1000.0,
-						hwmon_units[key.type]);
+				len += scnprintf(out_buf + len, out_buf_len - len, "%s%s%s=%g%s",
+						 len == 0 ? " " : ", ",
+						 hwmon_item_strs[bit],
+						 is_alarm ? "_alarm" : "",
+						 (double)val / 1000.0,
+						 hwmon_units[key.type]);
 			}
 			close(fd);
 		}
-- 
2.54.0

Re: [PATCH 11/11] perf tools: Use scnprintf() in build_id__snprintf() and hwmon read_events()

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing size adjustment in read() and pread() size arguments leads to out-of-bounds null termination on stack buffers.
- [High] If the `read()` system call returns `<= 0`, uninitialized stack memory is evaluated and potentially duplicated, leading to an information leak or crash.
- [Medium] A flawed check `if (fd > 0)` fails to process or close file descriptor 0, causing a resource leak if openat returns 0.
--

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

perf tools: Use scnprintf() in build_id__snprintf() and hwmon read_events()

build_id__snprintf() and hwmon_pmu__read_events() accumulate formatted
output via snprintf(), which returns the would-have-been-written count
on truncation. In hwmon_pmu__read_events(), len overshoots out_buf_len
and the next 'out_buf_len - len' underflows. Switch both to scnprintf()
which returns actual bytes written.

[ ... ]

> diff --git a/tools/perf/util/hwmon_pmu.c b/tools/perf/util/hwmon_pmu.c
> index fb3ffa8d32ad2..dbf6a71af47f9 100644
> --- a/tools/perf/util/hwmon_pmu.c
> +++ b/tools/perf/util/hwmon_pmu.c
> @@ -442,12 +442,12 @@ static size_t hwmon_pmu__describe_items(struct hwmon_pmu *hwm, char *out_buf, si
>  
>  				buf[read_len] = '\0';
                                ^^^^^^^^^^^^^^^^^^^^
[Severity: High]
This isn't a bug introduced by this patch, but doesn't this result in an
out-of-bounds write?

Since read() can return up to sizeof(buf), if the sysfs file contents
completely fill the buffer (i.e. read_len == sizeof(buf)), writing the null
terminator here will overwrite one byte past the end of the buf array on the
stack.

Could we pass sizeof(buf) - 1 to read() to avoid this?

This same pre-existing issue seems to be present in hwmon_pmu__read_events()
and evsel__hwmon_pmu_read() as well, where read() and pread() are called with
the full buffer size and similarly null-terminated.


[Severity: Medium]
Also a pre-existing issue in hwmon_pmu__describe_items() just above this hunk,
can this leak a file descriptor?

hwmon_pmu__describe_items() opens a sysfs file using openat(), and then checks:

    fd = openat(dir, buf, O_RDONLY);
    if (fd > 0) {

If the tool runs in an environment where standard input has been closed,
openat() can successfully return 0. The current logic treats 0 as an error,
skips file processing, and fails to call close(fd), causing a resource leak.

Should this check be if (fd >= 0) instead?


[Severity: High]
Another pre-existing issue in hwmon_pmu__read_events(), can this leak
uninitialized stack memory?

If read() returns 0 or less (for example, on an error or empty file), read_len
will be 0 or less. The code skips null-terminating the buffer and proceeds
to check:

    if (buf[0] == '\0') {
        ...
    value->label = strdup(buf);

Since char buf[128] is uninitialized, wouldn't evaluating buf[0] and passing
buf to strdup() result in reading and copying garbage stack memory until it
hits a random null byte?

>  				val = strtoll(buf, /*endptr=*/NULL, 10);
> -				len += snprintf(out_buf + len, out_buf_len - len, "%s%s%s=%g%s",
> -						len == 0 ? " " : ", ",
> -						hwmon_item_strs[bit],
> -						is_alarm ? "_alarm" : "",
> -						(double)val / 1000.0,
> -						hwmon_units[key.type]);
> +				len += scnprintf(out_buf + len, out_buf_len - len, "%s%s%s=%g%s",
> +						 len == 0 ? " " : ", ",
> +						 hwmon_item_strs[bit],
> +						 is_alarm ? "_alarm" : "",
> +						 (double)val / 1000.0,
> +						 hwmon_units[key.type]);
>  			}
>  			close(fd);
>  		}

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

[PATCHES v4 00/11] perf tools: Assorted fixes

[Arnaldo Carvalho de Melo]

Hi,

Sixth batch of pre-existing bug fixes found by sashiko-bot AI review
during the perf-data-validation hardening series.  All bugs are latent
in existing code — none were introduced by the hardening patches.

Three broad categories:

1. snprintf() accumulation overflows (patches 2, 8, 9, 10):

   Several functions accumulate formatted output via ret += snprintf().
   snprintf() returns the would-have-been-written count, so on truncation
   ret overshoots the buffer size and the next 'size - ret' underflows
   to a huge unsigned value, disabling bounds checking.  Switched to
   scnprintf() which returns actual bytes written.

   Affected: cpu_map__snprint(), snprintf_hex(),
   synthesize_bpf_prog_name(), hists__scnprintf_title(),
   build_id__snprintf(), hwmon_pmu__describe_items().

2. Missing safety checks on untrusted data (patches 1, 3, 5, 6):

   - get_max_num(): size_t underflow on empty sysfs file causes heap
     over-read.
   - machine__resolve(): unguarded env->cpu[] access with untrusted
     CPU index — switched to perf_env__get_cpu_topology() accessor,
     added bounds check before int16_t truncation.
   - timehist: test_bit(prio, ...) without bounds check on untrusted
     tracepoint priority.
   - idle-hist: rb_first_cached() on a tree populated with plain
     rb_insert_color() — rb_leftmost never set, callchains silently
     dropped.

3. Resource hygiene (patches 4, 7):

   - bitmap_free() without NULLing the pointer (2 call sites).
   - O_CLOEXEC missing from open() calls in DSO and ELF code
     (12 call sites across 2 files).

Patch 11 expands the libperf ABI TODO with the code simplification
argument for widening struct perf_cpu.cpu — the int16_t forces
truncation checks at every boundary where wider CPU indices are
narrowed.

Arnaldo Carvalho de Melo (11):
  perf tools: Fix get_max_num() size_t underflow on empty sysfs file
  perf tools: Use scnprintf() in cpu_map__snprint() to prevent overflow
  perf tools: Use perf_env__get_cpu_topology() in machine__resolve()
  perf tools: NULL bitmap pointers after bitmap_free()
  perf sched: Bounds-check prio before test_bit() in timehist
  perf sched: Fix idle-hist callchain display using wrong rb_first variant
  perf tools: Add O_CLOEXEC to open() calls in DSO and ELF code
  perf bpf: Use scnprintf() in snprintf_hex() and synthesize_bpf_prog_name()
  perf hists: Fix snprintf() in hists__scnprintf_title() UID filter path
  perf tools: Use scnprintf() in build_id__snprintf() and hwmon read_events()
  libperf: Document code simplification case for widening struct perf_cpu

 tools/lib/perf/TODO          |  8 ++++++++
 tools/perf/builtin-record.c  |  1 +
 tools/perf/builtin-sched.c   |  7 +++++--
 tools/perf/util/bpf-event.c  | 11 ++++++-----
 tools/perf/util/build-id.c   |  4 ++--
 tools/perf/util/cpumap.c     | 24 +++++++++++++++---------
 tools/perf/util/dso.c        |  4 ++--
 tools/perf/util/event.c      | 15 +++++++++++++--
 tools/perf/util/hist.c       |  7 ++++---
 tools/perf/util/hwmon_pmu.c  | 12 ++++++------
 tools/perf/util/mmap.c       |  1 +
 tools/perf/util/symbol-elf.c | 20 ++++++++++----------
 12 files changed, 73 insertions(+), 41 deletions(-)

Changes since v3:
  - Patch 3 (machine__resolve): expanded comment explaining why the
    outer al->cpu < nr_cpus_avail check is needed — the int16_t cast
    to struct perf_cpu silently truncates e.g. 65536 to 0, bypassing
    the accessor's internal bounds check.
    (Ian Rogers review)
  - Patch 10 (build_id__snprintf): fixed loop termination — after
    switching to scnprintf(), offs never reaches bf_size, so the
    loop spun doing zero-byte writes.  Changed condition to
    offs + 1 < bf_size.
    (Found by sashiko-bot, confirmed by Ian Rogers)
  - Patch 11 (TODO wording): fixed "wrap to small positive numbers"
    to "wrap to negative numbers (two's complement)".
    (Found by sashiko-bot, confirmed by Ian Rogers)

Changes since v2:
  - Dropped mbind patch (was v2 patch 4): the original code was
    correct — get_nodes() does --maxnode before computing
    BITS_TO_LONGS, so bitmap_zalloc(node_index + 1) with
    maxnode = node_index + 2 already match.  The commit message
    misstated the kernel-side semantics.
  - Split libperf ABI TODO hunk out of prio patch into standalone
    patch 11.
  - Patch 3 (machine__resolve): bounds-check al->cpu against
    env->nr_cpus_avail before truncating to int16_t struct perf_cpu.
    (Found by sashiko-bot lore review)
  - Patch 4 (was v2 patch 5, bitmap_free): reworded from "Three
    call sites" to "Two call sites" — removed dead store from
    memory_node__delete_nodes() where NULLing a pointer right
    before freeing the containing struct was useless.
    (Found by sashiko-bot lore review)

Changes since v1:
  - Patch 5 (was v1 patch 6): fix prio bounds-check logic — the
    v1 condition (prio < 0 || prio >= MAX_PRIO || !test_bit(...))
    incorrectly skipped events with unknown priority (prio == -1).
    Changed to (prio >= 0 && (prio >= MAX_PRIO || !test_bit(...)))
    to preserve the original pass-through for events without
    priority info.
    (Found by sashiko-bot lore review)

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

Thanks,

- Arnaldo

[PATCH 03/11] perf tools: Use perf_env__get_cpu_topology() in machine__resolve()

[Arnaldo Carvalho de Melo]

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

machine__resolve() accesses env->cpu[al->cpu].socket_id after checking
al->cpu >= 0 and env->cpu != NULL, but without validating al->cpu
against env->nr_cpus_avail.  Since al->cpu comes from the untrusted
perf.data sample, a crafted file with a large CPU index causes an
out-of-bounds heap read.

Use perf_env__get_cpu_topology() which validates both NULL and bounds.
Also bounds-check al->cpu before the cast to struct perf_cpu (int16_t):
without this, values like 65536 silently truncate to 0, bypassing the
accessor's internal check and returning CPU 0's topology.

Fixes: 0c4c4debb0adda4c ("perf tools: Add processor socket info to hist_entry and addr_location")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Kan Liang <kan.liang@intel.com>
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/event.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 66f4843bb235df53..ea75816d126a14be 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -14,6 +14,7 @@
 #include <linux/perf_event.h>
 #include "cpumap.h"
 #include "dso.h"
+#include "env.h"
 #include "event.h"
 #include "debug.h"
 #include "hist.h"
@@ -836,8 +837,18 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
 	if (al->cpu >= 0) {
 		struct perf_env *env = machine->env;
 
-		if (env && env->cpu)
-			al->socket = env->cpu[al->cpu].socket_id;
+		/*
+		 * Bounds-check al->cpu (s32) before casting to struct perf_cpu
+		 * (int16_t): without this, e.g. 65536 truncates to 0 and silently
+		 * returns CPU 0's topology.  Can go once perf_cpu.cpu is widened.
+		 */
+		if (env && al->cpu < env->nr_cpus_avail) {
+			struct cpu_topology_map *topo;
+
+			topo = perf_env__get_cpu_topology(env, (struct perf_cpu){ al->cpu });
+			if (topo)
+				al->socket = topo->socket_id;
+		}
 	}
 
 	/* Account for possible out-of-order switch events. */
-- 
2.54.0

Re: [PATCH 03/11] perf tools: Use perf_env__get_cpu_topology() in machine__resolve()

[sashiko-bot]

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

Pre-existing issues:
- [High] Out-of-bounds read on the global `symbol_conf.parallelism_filter` bitmap due to unclamped `al->parallelism`.
--

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

perf tools: Use perf_env__get_cpu_topology() in machine__resolve()

This commit fixes an out-of-bounds heap read in machine__resolve() by properly
validating al->cpu against env->nr_cpus_avail before accessing the CPU
topology. It also adds a bounds check before casting to struct perf_cpu to
prevent silent truncation on large CPU values.

> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c

[ ... ]

> @@ -836,8 +837,18 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
>  	if (al->cpu >= 0) {
>  		struct perf_env *env = machine->env;
>  
> -		if (env && env->cpu)
> -			al->socket = env->cpu[al->cpu].socket_id;
> +		/*
> +		 * Bounds-check al->cpu (s32) before casting to struct perf_cpu
> +		 * (int16_t): without this, e.g. 65536 truncates to 0 and silently
> +		 * returns CPU 0's topology.  Can go once perf_cpu.cpu is widened.
> +		 */
> +		if (env && al->cpu < env->nr_cpus_avail) {
> +			struct cpu_topology_map *topo;
> +
> +			topo = perf_env__get_cpu_topology(env, (struct perf_cpu){ al->cpu });
> +			if (topo)
> +				al->socket = topo->socket_id;
> +		}
>  	}
>  
>  	/* Account for possible out-of-order switch events. */
>  	al->parallelism = max(1, min(machine->parallelism, machine__nr_cpus_avail(machine)));
>  	if (test_bit(al->parallelism, symbol_conf.parallelism_filter))

[Severity: High]
This is a pre-existing issue, but does this test_bit() allow an out-of-bounds
read on the global symbol_conf.parallelism_filter bitmap?

Since al->parallelism is computed using machine__nr_cpus_avail(machine),
which depends on the HEADER_NRCPUS value in the perf.data file, a crafted
file could specify an extremely large number of CPUs.

If numerous PERF_RECORD_SWITCH events artificially inflate
machine->parallelism, the unclamped al->parallelism value could far exceed
the bounds of symbol_conf.parallelism_filter, which is statically sized to
MAX_NR_CPUS + 1 bits (typically 4097 bits).

Could reading so far beyond the mapped boundaries of the global symbol_conf
structure cause a segmentation fault when parsing an untrusted file?

>  		al->filtered |= (1 << HIST_FILTER__PARALLELISM);

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

[PATCHES v3 00/11] perf tools: Assorted fixes

[Arnaldo Carvalho de Melo]

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

Hi,

Sixth batch of pre-existing bug fixes found by sashiko-bot AI review
during the perf-data-validation hardening series.  All bugs are latent
in existing code — none were introduced by the hardening patches.

Three broad categories:

1. snprintf() accumulation overflows (patches 2, 8, 9, 10):

   Several functions accumulate formatted output via ret += snprintf().
   snprintf() returns the would-have-been-written count, so on truncation
   ret overshoots the buffer size and the next 'size - ret' underflows
   to a huge unsigned value, disabling bounds checking.  Switched to
   scnprintf() which returns actual bytes written.

   Affected: cpu_map__snprint(), snprintf_hex(),
   synthesize_bpf_prog_name(), hists__scnprintf_title(),
   build_id__snprintf(), hwmon_pmu__describe_items().

2. Missing safety checks on untrusted data (patches 1, 3, 5, 6):

   - get_max_num(): size_t underflow on empty sysfs file causes heap
     over-read.
   - machine__resolve(): unguarded env->cpu[] access with untrusted
     CPU index — switched to perf_env__get_cpu_topology() accessor,
     added bounds check before int16_t truncation.
   - timehist: test_bit(prio, ...) without bounds check on untrusted
     tracepoint priority.
   - idle-hist: rb_first_cached() on a tree populated with plain
     rb_insert_color() — rb_leftmost never set, callchains silently
     dropped.

3. Resource hygiene (patches 4, 7):

   - bitmap_free() without NULLing the pointer (2 call sites).
   - O_CLOEXEC missing from open() calls in DSO and ELF code
     (12 call sites across 2 files).

Also expanded the libperf ABI TODO (tools/lib/perf/TODO) to emphasize
the code simplification argument for widening struct perf_cpu.cpu from
int16_t to int — the narrow type forces defensive truncation checks at
every boundary where wider CPU indices are narrowed.

Arnaldo Carvalho de Melo (10):
  perf tools: Fix get_max_num() size_t underflow on empty sysfs file
  perf tools: Use scnprintf() in cpu_map__snprint() to prevent overflow
  perf tools: Use perf_env__get_cpu_topology() in machine__resolve()
  perf tools: NULL bitmap pointers after bitmap_free()
  perf sched: Bounds-check prio before test_bit() in timehist
  perf sched: Fix idle-hist callchain display using wrong rb_first variant
  perf tools: Add O_CLOEXEC to open() calls in DSO and ELF code
  perf bpf: Use scnprintf() in snprintf_hex() and synthesize_bpf_prog_name()
  perf hists: Fix snprintf() in hists__scnprintf_title() UID filter path
  perf tools: Use scnprintf() in build_id__snprintf() and hwmon read_events()

 tools/lib/perf/TODO          |  7 +++++++
 tools/perf/builtin-record.c  |  1 +
 tools/perf/builtin-sched.c   |  7 +++++--
 tools/perf/util/bpf-event.c  | 11 ++++++-----
 tools/perf/util/build-id.c   |  2 +-
 tools/perf/util/cpumap.c     | 24 +++++++++++++++---------
 tools/perf/util/dso.c        |  4 ++--
 tools/perf/util/event.c      | 11 +++++++++--
 tools/perf/util/hist.c       |  7 ++++---
 tools/perf/util/hwmon_pmu.c  | 12 ++++++------
 tools/perf/util/mmap.c       |  1 +
 tools/perf/util/symbol-elf.c | 20 ++++++++++----------
 12 files changed, 67 insertions(+), 40 deletions(-)

Changes since v2:
  - Dropped mbind patch (was v2 patch 4): the original code was
    correct — get_nodes() does --maxnode before computing
    BITS_TO_LONGS, so bitmap_zalloc(node_index + 1) with
    maxnode = node_index + 2 already match.  The commit message
    misstated the kernel-side semantics.
  - Patch 3 (machine__resolve): bounds-check al->cpu against
    env->nr_cpus_avail before truncating to int16_t struct perf_cpu.
    (Found by sashiko-bot lore review)
  - Patch 4 (was v2 patch 5, bitmap_free): reworded from "Three
    call sites" to "Two call sites" — removed dead store from
    memory_node__delete_nodes() where NULLing a pointer right
    before freeing the containing struct was useless.
    (Found by sashiko-bot lore review)

Changes since v1:
  - Patch 5 (was v1 patch 6): fix prio bounds-check logic — the
    v1 condition (prio < 0 || prio >= MAX_PRIO || !test_bit(...))
    incorrectly skipped events with unknown priority (prio == -1).
    Changed to (prio >= 0 && (prio >= MAX_PRIO || !test_bit(...)))
    to preserve the original pass-through for events without
    priority info.
    (Found by sashiko-bot lore review)

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

Thanks,

- Arnaldo

[PATCH 03/11] perf tools: Use perf_env__get_cpu_topology() in machine__resolve()

[Arnaldo Carvalho de Melo]

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

machine__resolve() accesses env->cpu[al->cpu].socket_id after checking
al->cpu >= 0 and env->cpu != NULL, but without validating al->cpu
against env->nr_cpus_avail.  Since al->cpu comes from the untrusted
perf.data sample, a crafted file with a large CPU index causes an
out-of-bounds heap read.

Use perf_env__get_cpu_topology() which validates both NULL and bounds.

Fixes: 0c4c4debb0adda4c ("perf tools: Add processor socket info to hist_entry and addr_location")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Kan Liang <kan.liang@intel.com>
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/event.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 66f4843bb235df53..001db00be1073ad4 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -14,6 +14,7 @@
 #include <linux/perf_event.h>
 #include "cpumap.h"
 #include "dso.h"
+#include "env.h"
 #include "event.h"
 #include "debug.h"
 #include "hist.h"
@@ -836,8 +837,14 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
 	if (al->cpu >= 0) {
 		struct perf_env *env = machine->env;
 
-		if (env && env->cpu)
-			al->socket = env->cpu[al->cpu].socket_id;
+		/* bounds-check before truncating to struct perf_cpu (int16_t) */
+		if (env && al->cpu < env->nr_cpus_avail) {
+			struct cpu_topology_map *topo;
+
+			topo = perf_env__get_cpu_topology(env, (struct perf_cpu){ al->cpu });
+			if (topo)
+				al->socket = topo->socket_id;
+		}
 	}
 
 	/* Account for possible out-of-order switch events. */
-- 
2.54.0

Re: [PATCH 03/11] perf tools: Use perf_env__get_cpu_topology() in machine__resolve()

[Ian Rogers]

On Mon, Jun 8, 2026 at 1:18 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> machine__resolve() accesses env->cpu[al->cpu].socket_id after checking
> al->cpu >= 0 and env->cpu != NULL, but without validating al->cpu
> against env->nr_cpus_avail.  Since al->cpu comes from the untrusted
> perf.data sample, a crafted file with a large CPU index causes an
> out-of-bounds heap read.
>
> Use perf_env__get_cpu_topology() which validates both NULL and bounds.
>
> Fixes: 0c4c4debb0adda4c ("perf tools: Add processor socket info to hist_entry and addr_location")
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Cc: Kan Liang <kan.liang@intel.com>
> 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/event.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 66f4843bb235df53..001db00be1073ad4 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -14,6 +14,7 @@
>  #include <linux/perf_event.h>
>  #include "cpumap.h"
>  #include "dso.h"
> +#include "env.h"
>  #include "event.h"
>  #include "debug.h"
>  #include "hist.h"
> @@ -836,8 +837,14 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
>         if (al->cpu >= 0) {
>                 struct perf_env *env = machine->env;
>
> -               if (env && env->cpu)
> -                       al->socket = env->cpu[al->cpu].socket_id;
> +               /* bounds-check before truncating to struct perf_cpu (int16_t) */
> +               if (env && al->cpu < env->nr_cpus_avail) {

Isn't this already covered by the test in perf_env__get_cpu_topology?

https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/env.h?h=perf-tools-next#n199
```
if (env->cpu && cpu.cpu >= 0 && cpu.cpu < env->nr_cpus_avail)
  return &env->cpu[cpu.cpu];
return NULL;
```

Thanks,
Ian




> +                       struct cpu_topology_map *topo;
> +
> +                       topo = perf_env__get_cpu_topology(env, (struct perf_cpu){ al->cpu });
> +                       if (topo)
> +                               al->socket = topo->socket_id;
> +               }
>         }
>
>         /* Account for possible out-of-order switch events. */
> --
> 2.54.0
>

[PATCHES v1 00/11] perf tools: Assorted fixes

[Arnaldo Carvalho de Melo]

Hi,

Sixth batch of pre-existing bug fixes found by sashiko-bot AI review
during the perf-data-validation hardening series.  All bugs are latent
in existing code — none were introduced by the hardening patches.

Three broad categories:

1. snprintf() accumulation overflows (patches 2, 9, 10, 11):

   Several functions accumulate formatted output via ret += snprintf().
   snprintf() returns the would-have-been-written count, so on truncation
   ret overshoots the buffer size and the next 'size - ret' underflows
   to a huge unsigned value, disabling bounds checking.  Switched to
   scnprintf() which returns actual bytes written.

   Affected: cpu_map__snprint(), snprintf_hex(),
   synthesize_bpf_prog_name(), hists__scnprintf_title(),
   build_id__snprintf(), hwmon_pmu__describe_items().

2. Missing safety checks on untrusted data (patches 1, 3, 6, 7):

   - get_max_num(): size_t underflow on empty sysfs file causes heap
     over-read.
   - machine__resolve(): unguarded env->cpu[] access with untrusted
     CPU index — switched to perf_env__get_cpu_topology() accessor.
   - timehist: test_bit(prio, ...) without bounds check on untrusted
     tracepoint priority.
   - idle-hist: rb_first_cached() on a tree populated with plain
     rb_insert_color() — rb_leftmost never set, callchains silently
     dropped.

3. Resource hygiene (patches 4, 5, 8):

   - mbind() bitmap allocation one bit short of what kernel reads.
   - bitmap_free() without NULLing the pointer (3 call sites).
   - O_CLOEXEC missing from open() calls in DSO and ELF code
     (12 call sites across 2 files).

Also expanded the libperf ABI TODO (tools/lib/perf/TODO) to emphasize
the code simplification argument for widening struct perf_cpu.cpu from
int16_t to int — the narrow type forces defensive truncation checks at
every boundary where wider CPU indices are narrowed.

 tools/lib/perf/TODO          |  7 +++++++
 tools/perf/builtin-record.c  |  1 +
 tools/perf/builtin-sched.c   |  7 +++++--
 tools/perf/util/bpf-event.c  | 11 ++++++-----
 tools/perf/util/build-id.c   |  2 +-
 tools/perf/util/cpumap.c     | 24 +++++++++++++++---------
 tools/perf/util/dso.c        |  4 ++--
 tools/perf/util/event.c      |  9 +++++++--
 tools/perf/util/header.c     |  4 +++-
 tools/perf/util/hist.c       |  7 ++++---
 tools/perf/util/hwmon_pmu.c  | 12 ++++++------
 tools/perf/util/mmap.c       |  4 +++-
 tools/perf/util/symbol-elf.c | 20 ++++++++++----------
 13 files changed, 70 insertions(+), 42 deletions(-)

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Thanks,

- Arnaldo

[PATCH 03/11] perf tools: Use perf_env__get_cpu_topology() in machine__resolve()

[Arnaldo Carvalho de Melo]

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

machine__resolve() accesses env->cpu[al->cpu].socket_id after checking
al->cpu >= 0 and env->cpu != NULL, but without validating al->cpu
against env->nr_cpus_avail.  Since al->cpu comes from the untrusted
perf.data sample, a crafted file with a large CPU index causes an
out-of-bounds heap read.

Use perf_env__get_cpu_topology() which validates both NULL and bounds.

Fixes: 0c4c4debb0adda4c ("perf tools: Add processor socket info to hist_entry and addr_location")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Kan Liang <kan.liang@intel.com>
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/event.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 66f4843bb235df53..66293fea64fde9fd 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -14,6 +14,7 @@
 #include <linux/perf_event.h>
 #include "cpumap.h"
 #include "dso.h"
+#include "env.h"
 #include "event.h"
 #include "debug.h"
 #include "hist.h"
@@ -835,9 +836,13 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
 
 	if (al->cpu >= 0) {
 		struct perf_env *env = machine->env;
+		struct cpu_topology_map *topo;
 
-		if (env && env->cpu)
-			al->socket = env->cpu[al->cpu].socket_id;
+		if (env) {
+			topo = perf_env__get_cpu_topology(env, (struct perf_cpu){ al->cpu });
+			if (topo)
+				al->socket = topo->socket_id;
+		}
 	}
 
 	/* Account for possible out-of-order switch events. */
-- 
2.54.0