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

[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 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 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Heap buffer underflow due to unsafe length manipulation after `filename__read_str` on empty PMU identifier file.
- [High] Heap buffer underflow due to unsafe length manipulation after `procfs__read_str` on empty procfs file.
- [High] Reading an empty PMU scale file leads to a stack buffer underflow.
- [High] Reading an empty PMU unit file leads to an out-of-bounds array access inside the `perf_pmu_alias` struct.
--

commit 3cacfb3c562f81ba051910331484d58d28297996
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 heap buffer underflow in get_max_num() by adding an
early return when an empty sysfs file is read. This prevents the size_t
variable from underflowing during a backward scan of the buffer.

> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 21fa781b03cc7..1fab00ec4a59a 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;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a similar pattern in
tools/perf/util/pmu.c:pmu_id(). Since filename__read_str() returns success (0)
and sets len to 0 when reading an empty file, does the following code
underflow the len variable?

tools/perf/util/pmu.c:pmu_id() {
...
	if (filename__read_str(path, &str, &len) < 0)
		return NULL;

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

[Severity: High]
This is a pre-existing issue, but another instance seems to exist in
tools/perf/util/thread.c:thread__set_comm_from_proc(). If the procfs file is
empty (e.g., due to a race), procfs__read_str() returns 0 and sets sz to 0.
Could this lead to a heap buffer underflow when setting the NUL byte?

tools/perf/util/thread.c:thread__set_comm_from_proc() {
...
	if (!(snprintf(path, sizeof(path), "%d/task/%d/comm",
		       thread__pid(thread), thread__tid(thread)) >= (int)sizeof(path)) &&
	    procfs__read_str(path, &comm, &sz) == 0) {
		comm[sz - 1] = '\0';
...
}

[Severity: High]
This isn't a bug introduced by this patch, but reading an empty PMU scale
file might cause a stack buffer underflow in
tools/perf/util/pmu.c:perf_pmu__parse_scale(). 

If the file is empty, read() returns 0, setting sret to 0. The error check
passes, and we evaluate scale[-1]. Could this read uninitialized stack
memory and potentially write a NUL byte out-of-bounds?

tools/perf/util/pmu.c:perf_pmu__parse_scale() {
...
	sret = read(fd, scale, sizeof(scale)-1);
	if (sret < 0)
		goto error;

	if (scale[sret - 1] == '\n')
		scale[sret - 1] = '\0';
...
}

[Severity: High]
This is a pre-existing issue, but there appears to be a similar out-of-bounds
access in tools/perf/util/pmu.c:perf_pmu__parse_unit(). 

If read() returns 0 for an empty file, sret is set to 0. The code then
evaluates alias->unit[-1]. Since unit is part of the heap-allocated struct
perf_pmu_alias, could this overwrite the adjacent pmu_name pointer field if
the preceding byte happens to be a newline?

tools/perf/util/pmu.c:perf_pmu__parse_unit() {
...
	sret = read(fd, alias->unit, UNIT_MAX_LEN);
	if (sret < 0)
		goto error;

	close(fd);

	if (alias->unit[sret - 1] == '\n')
		alias->unit[sret - 1] = '\0';
...
}

>  	/* start on the right, to find highest node num */
>  	while (--num) {
>  		if ((buf[num] == ',') || (buf[num] == '-')) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607232925.1935819-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

[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

[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..5f3510f9ca249132 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 — skip invalid values */
+		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 1 potential issue(s) to consider:
- [Medium] Logic inversion in priority bounds check breaks --prio filtering for initial events.
--

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

perf sched: Bounds-check prio before test_bit() in timehist

This commit adds bounds checking for the priority variable in
timehist_skip_sample to prevent out-of-bounds memory accesses in
test_bit. It attempts to skip samples if the priority falls outside the
valid range.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 1ff01f03d2ad1..5f3510f9ca249 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 — skip invalid values */
> +		if (prio < 0 || prio >= MAX_PRIO ||
> +		    !test_bit(prio, sched->prio_bitmap)) {

[Severity: Medium]
Does this logic inversion unintentionally skip valid initial events that do
not have their priority recorded yet?

In timehist_skip_sample(), prio is initialized to -1 to represent an
unknown priority:

    int prio = -1;

With the previous logic, an unknown priority of -1 caused the condition to
evaluate to false, allowing the event to pass the filter:

    if (prio != -1 && !test_bit(prio, sched->prio_bitmap)) {
        rc = true;
        sched->skipped_samples++;
    }

With the new logic, the condition prio < 0 evaluates to true when prio is -1,
causing the event to be unexpectedly skipped.

>  			rc = true;
>  			sched->skipped_samples++;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607232925.1935819-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 5f3510f9ca249132..30de749e066335a5 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 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Stack buffer overflow in `sysfs__read_build_id` due to integer overflow.
- [High] Incomplete fix for file descriptor leaks; `mkstemp()` calls still lack `O_CLOEXEC`.
- [High] Out-of-bounds memory read in `filename__read_debuglink` when parsing malformed ELF files.
--

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

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

This commit adds the O_CLOEXEC flag to open() calls in dso.c and
symbol-elf.c. The goal is to prevent file descriptors from leaking to
child processes spawned during symbol resolution.

> 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 isn't a bug introduced by this patch, but since the goal is to prevent
file descriptor leaks to child processes, should we also update the
mkstemp() calls?

Here in filename__decompress(), mkstemp() creates a temporary file
descriptor without the close-on-exec flag. This FD will be inherited by
spawned child processes. Could this be changed to use
mkostemp(tmpbuf, O_CLOEXEC)?

[ ... ]
> 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
[ ... ]
> @@ -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);

[Severity: Critical]
This is a pre-existing issue, but there appears to be an integer overflow
risk further down in sysfs__read_build_id() that could lead to a stack
buffer overflow.

The function reads nhdr.n_namesz and nhdr.n_descsz directly from the file.
Since these are 32-bit values, they can be arbitrarily large. The
calculation:

    int n = namesz + descsz;

can overflow the 32-bit signed integer n, resulting in a negative value.
The size check:

    if (n > (int)sizeof(bf)) {
        n = sizeof(bf);
        ...
    }

bypasses truncation because a negative number is smaller than sizeof(bf).
When read(fd, bf, n) is called, the negative n is interpreted as a massive
size_t count, potentially reading up to 2GB of data into the 8KB stack
buffer bf[BUFSIZ].

Could this be exploited if perf processes data from an untrusted root
directory?

>  	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);

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of an
out-of-bounds read further down in filename__read_debuglink() when parsing
malformed ELF files?

Later in this function, the code blindly copies the section data:

    /* the start of this section is a zero-terminated string */
    strncpy(debuglink, data->d_buf, size);

If a corrupted ELF file provides a small .gnu_debuglink section without
a null terminator, strncpy() lacks bounds validation against the actual
section size (data->d_size) and could read past the end of the mapped
section data.

If this crosses into unmapped memory, could it cause a segmentation fault
and crash perf?

>  	if (fd < 0)
>  		goto out;
>  
[ ... ]
> @@ -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);

[Severity: High]
This is also a pre-existing issue, but the if (temp) branch uses mkstemp()
which lacks O_CLOEXEC. This creates an inconsistent state compared to the
else branch that correctly adds O_CLOEXEC.

Does this mkstemp() need to be replaced with mkostemp(..., O_CLOEXEC)
to fully prevent file descriptor exhaustion? (There is also another
mkstemp() call in read_gnu_debugdata() that exhibits the same behavior).

>  	if (kcore->fd == -1)
>  		return -1;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607232925.1935819-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

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Buffer overflow in `hwmon_pmu__for_each_event` due to unsafe `snprintf` accumulation.
- [Critical] Buffer overflow in `perf_pmu__for_each_event` due to unsafe `snprintf` accumulation.
- [Medium] Missing validation for `btf__type_by_id()` return value in `synthesize_bpf_prog_name` leads to a NULL pointer dereference.
- [Medium] Missing bounds checking when accessing BPF function info and program tags allows out-of-bounds memory reads.
- [Medium] Leaked `map.btf` and `map.rodata` in `bpf_metadata_create()` on error path.
- [Medium] Leaked `info_node->metadata` when `perf_env__insert_bpf_prog_info` fails in `perf_env__add_bpf_info()`.
--

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

perf bpf: Use scnprintf() in snprintf_hex() and synthesize_bpf_prog_name()

This commit fixes buffer overflows by switching from snprintf() to scnprintf()
when accumulating formatted strings. This prevents writing past the end of
the buffer if truncation previously occurred.

> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
> index a27945c279efb..2c09842469f1f 100644
> --- a/tools/perf/util/bpf-event.c
> +++ b/tools/perf/util/bpf-event.c
[ ... ]
> @@ -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) {

While reviewing this function, I noticed a couple of pre-existing issues
just below this diff hunk:

tools/perf/util/bpf-event.c:synthesize_bpf_prog_name() {
        if (btf) {
                finfo = func_infos + sub_id * info->func_info_rec_size;
                t = btf__type_by_id(btf, finfo->type_id);
                short_name = btf__name_by_offset(btf, t->name_off);
...

[Severity: Medium]
This isn't a bug introduced by this patch, but does calculating the finfo
pointer using the unvalidated info->func_info_rec_size from the untrusted
perf.data file allow out-of-bounds memory reads?

[Severity: Medium]
This is a pre-existing issue, but btf__type_by_id() can return NULL if
finfo->type_id is invalid or out of bounds. Will accessing t->name_off
immediately after cause a NULL pointer dereference here?


Additionally, I noticed a few other pre-existing issues in the perf tools
codebase during this review:

tools/perf/util/bpf-event.c:bpf_metadata_create() {
...
        metadata = bpf_metadata_alloc(info->nr_prog_tags, map.num_vars);
        if (!metadata)
                continue;
...

[Severity: Medium]
This isn't a bug introduced by this patch, but does this error path
permanently leak the memory allocated for map.btf and map.rodata by
skipping the bpf_metadata_free_map_data() cleanup?


tools/perf/util/bpf-event.c:perf_env__add_bpf_info() {
...
        if (!perf_env__insert_bpf_prog_info(env, info_node)) {
                pr_debug("%s: duplicate add bpf info request for id %u\n",
                         __func__, btf_id);
                free(info_linear);
                free(info_node);
                goto out;
        }
...

[Severity: Medium]
This is a pre-existing issue, but does freeing info_node here without freeing
info_node->metadata leak the dynamically allocated metadata?


tools/perf/util/hwmon_pmu.c:hwmon_pmu__for_each_event() {
...
        len += hwmon_pmu__describe_items(hwm, desc_buf + len, sizeof(desc_buf) - len,
                                         key, value->items, /*is_alarm=*/false);
...

[Severity: Critical]
This isn't a bug introduced by this patch, but it is a similar issue to the
ones being fixed.

Does accumulating the length from snprintf() without checking for truncation
cause sizeof(desc_buf) - len to wrap to a huge value, leading to a stack
buffer overflow in subsequent calls? 

Note that this issue was not fixed by the later patches in the series.


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

[Severity: Critical]
This is a pre-existing issue, but does accumulating the return value of
snprintf() into buf_used risk truncation wrapping? 

If buf_used exceeds sizeof(buf), sizeof(buf) - buf_used underflows, leading
to a stack buffer overflow on the next write. 

Note that this issue was also not fixed by the later patches in the series.

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

[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 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Buffer overflow in config_show() via test_driver/test_fs sysfs attributes due to snprintf underflow.
- [High] Buffer overflow in pm80xx_get_non_fatal_dump() when reading sysfs attributes due to unbounded snprintf usage.
- [Medium] Stack buffer overflow in hwmon_pmu__for_each_event() due to snprintf underflow.
- [Medium] Stack buffer overflow in perf_pmu__for_each_event() when parsing PMU aliases due to snprintf accumulation.
--

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

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

This commit replaces snprintf() with scnprintf() in the UID filter path
of hists__scnprintf_title() to prevent buffer size underflow. It ensures
consistent buffer length calculation, matching the rest of the function.

> 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);
> +	}
>  	if (thread) {
>  		if (hists__has(hists, thread)) {
>  			printed += scnprintf(bf + printed, size - printed,

While reviewing this fix, I noticed a few similar pre-existing issues in
other areas of the codebase where snprintf() return values might cause buffer
overflows.

lib/test_kmod.c:config_show() {
    ...
	if (config->test_driver)
		len += snprintf(buf+len, PAGE_SIZE - len,
				"driver:\t%s\n",
				config->test_driver);
    ...
}

[Severity: High]
This is a pre-existing issue, but could this calculation underflow
PAGE_SIZE - len?

Since test_driver can be up to PAGE_SIZE bytes long, the accumulated length
can exceed PAGE_SIZE. If snprintf() returns the would-have-written length,
does the subtraction result in a large positive value that allows subsequent
snprintf() calls to write out of bounds of the sysfs buffer?


drivers/scsi/pm8001/pm80xx_hwi.c:pm80xx_get_non_fatal_dump() {
    ...
	for (index = 0; index < output_length/4; index++)
		buf_copy += snprintf(buf_copy, PAGE_SIZE,
				"%08x ", *(temp+index));
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but might this loop write beyond
the bounds of the sysfs buffer?

The pointer buf_copy advances with each write, but the size parameter passed
to snprintf() is constantly PAGE_SIZE instead of the remaining buffer space.
If the hardware provides a large output_length, would this continuously write
up to PAGE_SIZE bytes and cause a heap buffer overflow?


tools/perf/util/hwmon_pmu.c:hwmon_pmu__for_each_event() {
    ...
		char desc_buf[256];
    ...
		len = snprintf(desc_buf, sizeof(desc_buf), "%s in unit %s named %s.",
			hwmon_desc[key.type],
			pmu->name + 6,
			value->label ?: info.name);

		len += hwmon_pmu__describe_items(hwm, desc_buf + len, sizeof(desc_buf) - len,
						key, value->items, /*is_alarm=*/false);
    ...
}

[Severity: Medium]
This is a pre-existing issue, but is there a risk of a stack buffer overflow
here?

Since value->label can be up to 255 bytes, snprintf() might return a length
greater than sizeof(desc_buf). Would passing sizeof(desc_buf) - len to
hwmon_pmu__describe_items() then underflow to a massive size_t value,
allowing scnprintf() to write out-of-bounds on the stack?


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;
		}
		info.desc = event->desc;
		info.long_desc = event->long_desc;
		info.encoding_desc = buf + buf_used;
		buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
				"%.*s/%s/", (int)pmu_name_len, info.pmu_name, event->terms) + 1;
    ...
}

[Severity: Medium]
This isn't a bug introduced by this patch, but does accumulating the snprintf()
return values into buf_used risk underflowing sizeof(buf) - buf_used?

If an earlier snprintf() truncates, it returns the number of bytes it would
have written. Could this cause buf_used to exceed sizeof(buf), making the
size limit on the next snprintf() underflow and write beyond the end of the
stack buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607232925.1935819-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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Reading sysfs file contents into fixed-size stack buffers causes a 1-byte out-of-bounds write when appending a null terminator if the read fills the buffer.
- [High] User-controlled stack buffer overflow via misused `strlcpy` in `parse_hwmon_filename()`.
--

commit 48628bf245e27619381dcbb4cf7723991e4f5a11
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 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>

[ ... ]

> 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 does this write out of bounds?

In hwmon_pmu__describe_items(), the code reads from sysfs directly into a
stack buffer (buf) using sizeof(buf) as the maximum length. If the read
fills the buffer exactly and lacks a trailing newline, read_len will
equal sizeof(buf).

Writing to buf[read_len] would then write one byte past the end of the
array, corrupting an adjacent byte on the stack with a null byte. Should
the read size be sizeof(buf) - 1 to safely null-terminate?

This same pattern appears to exist in evsel__hwmon_pmu_read() and
hwmon_pmu__read_events().

>  				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]);

[Severity: High]
This is a pre-existing issue, but I also noticed a potential stack buffer
overflow in parse_hwmon_filename() in this same file.

When copying an event prefix into fn_type:

tools/perf/util/hwmon_pmu.c:parse_hwmon_filename() {
    ...
	char fn_type[24];
    ...
	*alarm = false;
	fn_item_len = strlen(fn_item);
	if (fn_item_len > 6 && !strcmp(&fn_item[fn_item_len - 6], "_alarm")) {
		assert(strlen(LONGEST_HWMON_ITEM_STR) < sizeof(fn_type));
		strlcpy(fn_type, fn_item, fn_item_len - 5);
		fn_item = fn_type;
		*alarm = true;
	}
    ...
}

The third argument to strlcpy() is passed as fn_item_len - 5 instead of the
destination buffer capacity (sizeof(fn_type)). Since fn_item originates from
user-controlled event input, an arbitrarily long name ending in "_alarm"
could cause strlcpy() to copy more than 24 characters into fn_type,
overflowing the stack buffer.

Could this be exploited for memory corruption if perf is run with elevated
capabilities?

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

[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