* [PATCH 1/9] perf machine: Propagate machine__init() error to callers
2026-06-16 2:27 [PATCHES v4 0/9] perf tools: Fix pre-existing bugs in machine, cs-etm, c2c, bpf, and dso Arnaldo Carvalho de Melo
@ 2026-06-16 2:27 ` Arnaldo Carvalho de Melo
2026-06-16 2:50 ` sashiko-bot
2026-06-16 2:27 ` [PATCH 2/9] perf machine: Use snprintf() for guestmount path construction Arnaldo Carvalho de Melo
` (7 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-06-16 2:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, sashiko-bot, Claude
From: Arnaldo Carvalho de Melo <acme@redhat.com>
machine__init() always returns 0 even when memory allocation fails,
because commit 81f981d7ec43ed93 ("perf machine: Free root_dir in
machine__init() error path") introduced 'int err = -ENOMEM' and an
error cleanup path but left the final 'return 0' instead of
'return err'.
Fix by returning err, check the return value in __machine__new_host()
which was ignoring it, and change machines__init() from void to int so
it too can propagate the error to perf_session__new(), aslr_tool__init()
and test callers.
The error cleanup also used zfree(&machine->kmaps), but kmaps is a
refcounted maps structure — use maps__zput() to properly drop the
reference, matching machine__exit().
Move dsos__init() and threads__init() before the first fallible
allocation (maps__new) so that machine__exit() is safe to call on
any machine struct that machine__init() touched, even on early failure.
Fixes: 81f981d7ec43ed93 ("perf machine: Free root_dir in machine__init() error path")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/tests/hists_cumulate.c | 3 ++-
tools/perf/tests/hists_filter.c | 3 ++-
tools/perf/tests/hists_link.c | 3 ++-
tools/perf/tests/hists_output.c | 3 ++-
tools/perf/tests/thread-maps-share.c | 2 +-
tools/perf/util/aslr.c | 12 +++++++++---
tools/perf/util/machine.c | 24 ++++++++++++++----------
tools/perf/util/machine.h | 2 +-
tools/perf/util/session.c | 7 ++++---
9 files changed, 37 insertions(+), 22 deletions(-)
diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c
index 267cbc24691acd77..9f4d4e281e11edaa 100644
--- a/tools/perf/tests/hists_cumulate.c
+++ b/tools/perf/tests/hists_cumulate.c
@@ -723,7 +723,8 @@ static int test__hists_cumulate(struct test_suite *test __maybe_unused, int subt
goto out;
err = TEST_FAIL;
- machines__init(&machines);
+ if (machines__init(&machines))
+ goto out;
/* setup threads/dso/map/symbols also */
machine = setup_fake_machine(&machines);
diff --git a/tools/perf/tests/hists_filter.c b/tools/perf/tests/hists_filter.c
index 002e3a4c1ca59b9d..fa47e995de82d427 100644
--- a/tools/perf/tests/hists_filter.c
+++ b/tools/perf/tests/hists_filter.c
@@ -131,7 +131,8 @@ static int test__hists_filter(struct test_suite *test __maybe_unused, int subtes
goto out;
err = TEST_FAIL;
- machines__init(&machines);
+ if (machines__init(&machines))
+ goto out;
/* setup threads/dso/map/symbols also */
machine = setup_fake_machine(&machines);
diff --git a/tools/perf/tests/hists_link.c b/tools/perf/tests/hists_link.c
index 996f5f0b3bd17fe5..05078dac93c4ec6b 100644
--- a/tools/perf/tests/hists_link.c
+++ b/tools/perf/tests/hists_link.c
@@ -303,7 +303,8 @@ static int test__hists_link(struct test_suite *test __maybe_unused, int subtest
goto out;
err = TEST_FAIL;
- machines__init(&machines);
+ if (machines__init(&machines))
+ goto out;
/* setup threads/dso/map/symbols also */
machine = setup_fake_machine(&machines);
diff --git a/tools/perf/tests/hists_output.c b/tools/perf/tests/hists_output.c
index fa683fd7b1e5ebb2..1ec51c15b1046eb4 100644
--- a/tools/perf/tests/hists_output.c
+++ b/tools/perf/tests/hists_output.c
@@ -610,7 +610,8 @@ static int test__hists_output(struct test_suite *test __maybe_unused, int subtes
goto out;
err = TEST_FAIL;
- machines__init(&machines);
+ if (machines__init(&machines))
+ goto out;
/* setup threads/dso/map/symbols also */
machine = setup_fake_machine(&machines);
diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c
index e9ecd30a5c058076..0431bff31b3a18c3 100644
--- a/tools/perf/tests/thread-maps-share.c
+++ b/tools/perf/tests/thread-maps-share.c
@@ -27,7 +27,7 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s
* other group (pid: 4, tids: 4, 5)
*/
- machines__init(&machines);
+ TEST_ASSERT_VAL("failed to init machines", machines__init(&machines) == 0);
machine = &machines.host;
/* create process with 4 threads */
diff --git a/tools/perf/util/aslr.c b/tools/perf/util/aslr.c
index a946fff2ac4dd4b4..6a7542e7db827d1b 100644
--- a/tools/perf/util/aslr.c
+++ b/tools/perf/util/aslr.c
@@ -1237,12 +1237,13 @@ void aslr_tool__strip_attr_event(union perf_event *event, struct evlist *evlist)
}
}
-static void aslr_tool__init(struct aslr_tool *aslr, struct perf_tool *delegate)
+static int aslr_tool__init(struct aslr_tool *aslr, struct perf_tool *delegate)
{
delegate_tool__init(&aslr->tool, delegate);
aslr->tool.tool.ordered_events = true;
- machines__init(&aslr->machines);
+ if (machines__init(&aslr->machines))
+ return -ENOMEM;
hashmap__init(&aslr->remap_addresses,
remap_addresses__hash, remap_addresses__equal,
@@ -1276,6 +1277,8 @@ static void aslr_tool__init(struct aslr_tool *aslr, struct perf_tool *delegate)
aslr->tool.tool.auxtrace = aslr_tool__process_auxtrace;
aslr->tool.tool.auxtrace_info = aslr_tool__process_auxtrace_info;
aslr->tool.tool.auxtrace_error = aslr_tool__process_auxtrace_error;
+
+ return 0;
}
struct perf_tool *aslr_tool__new(struct perf_tool *delegate)
@@ -1285,7 +1288,10 @@ struct perf_tool *aslr_tool__new(struct perf_tool *delegate)
if (!aslr)
return NULL;
- aslr_tool__init(aslr, delegate);
+ if (aslr_tool__init(aslr, delegate)) {
+ free(aslr);
+ return NULL;
+ }
return &aslr->tool.tool;
}
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 31715366e29ff704..9329d319bd033699 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -79,15 +79,14 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
int err = -ENOMEM;
memset(machine, 0, sizeof(*machine));
- machine->kmaps = maps__new(machine);
- if (machine->kmaps == NULL)
- return -ENOMEM;
-
RB_CLEAR_NODE(&machine->rb_node);
dsos__init(&machine->dsos);
-
threads__init(&machine->threads);
+ machine->kmaps = maps__new(machine);
+ if (machine->kmaps == NULL)
+ goto out;
+
machine->vdso_info = NULL;
machine->env = NULL;
@@ -124,11 +123,11 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
out:
if (err) {
- zfree(&machine->kmaps);
+ maps__zput(machine->kmaps);
zfree(&machine->root_dir);
zfree(&machine->mmap_name);
}
- return 0;
+ return err;
}
static struct machine *__machine__new_host(struct perf_env *host_env, bool kernel_maps)
@@ -138,7 +137,10 @@ static struct machine *__machine__new_host(struct perf_env *host_env, bool kerne
if (!machine)
return NULL;
- machine__init(machine, "", HOST_KERNEL_ID);
+ if (machine__init(machine, "", HOST_KERNEL_ID) != 0) {
+ free(machine);
+ return NULL;
+ }
if (kernel_maps && machine__create_kernel_maps(machine) < 0) {
free(machine);
@@ -231,10 +233,12 @@ void machine__delete(struct machine *machine)
}
}
-void machines__init(struct machines *machines)
+int machines__init(struct machines *machines)
{
- machine__init(&machines->host, "", HOST_KERNEL_ID);
+ int err = machine__init(&machines->host, "", HOST_KERNEL_ID);
+
machines->guests = RB_ROOT_CACHED;
+ return err;
}
void machines__exit(struct machines *machines)
diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index aaddfb70ea665452..26f9827062f5eb5b 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -152,7 +152,7 @@ struct machines {
struct rb_root_cached guests;
};
-void machines__init(struct machines *machines);
+int machines__init(struct machines *machines);
void machines__exit(struct machines *machines);
void machines__process_guests(struct machines *machines,
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 1a9a008ddda35120..f391a822480db001 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -160,11 +160,12 @@ struct perf_session *__perf_session__new(struct perf_data *data,
session->decomp_data.zstd_decomp = &session->zstd_data;
session->active_decomp = &session->decomp_data;
INIT_LIST_HEAD(&session->auxtrace_index);
- machines__init(&session->machines);
+ perf_env__init(&session->header.env);
+ if (machines__init(&session->machines))
+ goto out_delete;
+
ordered_events__init(&session->ordered_events,
ordered_events__deliver_event, NULL);
-
- perf_env__init(&session->header.env);
if (data) {
ret = perf_data__open(data);
if (ret < 0)
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/9] perf machine: Propagate machine__init() error to callers
2026-06-16 2:27 ` [PATCH 1/9] perf machine: Propagate machine__init() error to callers Arnaldo Carvalho de Melo
@ 2026-06-16 2:50 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-16 2:50 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The return value of the newly fallible `machine__init()` function is ignored in tests, leading to potential null pointer dereferences on initialization failure.
--
commit 75ef0ba718fac4d2482673c59e4b0fde21e0bfe2
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
perf machine: Propagate machine__init() error to callers
machine__init() always returns 0 even when memory allocation fails,
because commit 81f981d7ec43ed93 introduced an error cleanup path but
left the final 'return 0' instead of 'return err'.
Fix by returning err, check the return value in __machine__new_host()
which was ignoring it, and change machines__init() from void to int so
it too can propagate the error.
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 31715366e29ff..9329d319bd033 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
[ ... ]
> @@ -124,11 +123,11 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
>
> out:
> if (err) {
> - zfree(&machine->kmaps);
> + maps__zput(machine->kmaps);
> zfree(&machine->root_dir);
> zfree(&machine->mmap_name);
> }
> - return 0;
> + return err;
> }
[Severity: Medium]
Since this patch changes machine__init() to return its internal error instead
of returning 0 on failure, do we also need to update the direct callers in the
tests to check this return value?
For example, test__kallsyms_split() ignores the return value:
tools/perf/tests/kallsyms-split.c:test__kallsyms_split() {
...
machine__init(&m, root_dir, HOST_KERNEL_ID);
if (machine__create_kernel_maps(&m) < 0) {
...
}
And test__vmlinux_matches_kallsyms() does as well:
tools/perf/tests/vmlinux-kallsyms.c:test__vmlinux_matches_kallsyms() {
...
machine__init(&args.kallsyms, "", HOST_KERNEL_ID);
machine__init(&vmlinux, "", HOST_KERNEL_ID);
...
}
If machine__init() fails, it cleans up and leaves machine.kmaps as NULL.
Could this lead to a NULL pointer dereference in functions like
machine__create_kernel_maps() that expect kmaps to be valid?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616022715.5739-1-acme@kernel.org?part=1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/9] perf machine: Use snprintf() for guestmount path construction
2026-06-16 2:27 [PATCHES v4 0/9] perf tools: Fix pre-existing bugs in machine, cs-etm, c2c, bpf, and dso Arnaldo Carvalho de Melo
2026-06-16 2:27 ` [PATCH 1/9] perf machine: Propagate machine__init() error to callers Arnaldo Carvalho de Melo
@ 2026-06-16 2:27 ` Arnaldo Carvalho de Melo
2026-06-16 2:40 ` sashiko-bot
2026-06-16 2:27 ` [PATCH 3/9] perf cs-etm: Validate num_cpu before metadata allocation Arnaldo Carvalho de Melo
` (6 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-06-16 2:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, sashiko-bot, Zhang, Yanmin, Claude
From: Arnaldo Carvalho de Melo <acme@redhat.com>
machines__findnew() and machines__create_guest_kernel_maps() use
sprintf() to build paths by prepending symbol_conf.guestmount.
Both write into PATH_MAX stack buffers, but guestmount comes from
user configuration and is not length-checked. A guestmount path
at or near PATH_MAX causes a stack buffer overflow.
Switch to snprintf() with sizeof() to prevent overflow. The
subsequent access()/fopen() calls will fail on a truncated path.
Fixes: a1645ce12adb6c9c ("perf: 'perf kvm' tool for monitoring guest performance from host")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Zhang, Yanmin <yanmin_zhang@linux.intel.com>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/machine.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 9329d319bd033699..0d2ebf6a84bcf880 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -333,7 +333,7 @@ struct machine *machines__findnew(struct machines *machines, pid_t pid)
if ((pid != HOST_KERNEL_ID) &&
(pid != DEFAULT_GUEST_KERNEL_ID) &&
(symbol_conf.guestmount)) {
- sprintf(path, "%s/%d", symbol_conf.guestmount, pid);
+ snprintf(path, sizeof(path), "%s/%d", symbol_conf.guestmount, pid);
if (access(path, R_OK)) {
static struct strlist *seen;
@@ -1260,9 +1260,9 @@ int machines__create_guest_kernel_maps(struct machines *machines)
namelist[i]->d_name);
continue;
}
- sprintf(path, "%s/%s/proc/kallsyms",
- symbol_conf.guestmount,
- namelist[i]->d_name);
+ snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
+ symbol_conf.guestmount,
+ namelist[i]->d_name);
ret = access(path, R_OK);
if (ret) {
pr_debug("Can't access file %s\n", path);
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/9] perf machine: Use snprintf() for guestmount path construction
2026-06-16 2:27 ` [PATCH 2/9] perf machine: Use snprintf() for guestmount path construction Arnaldo Carvalho de Melo
@ 2026-06-16 2:40 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-16 2:40 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Aborting processing of all remaining guest maps when a single directory lacks proc/kallsyms.
- [High] Uninitialized errno checked against ERANGE after strtol() causes valid directory names to be erroneously skipped.
--
commit 33df4f15ea6c660ee8ff4b243d151622bd2d9271
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
perf machine: Use snprintf() for guestmount path construction
This commit prevents a potential stack buffer overflow by replacing
sprintf() with snprintf() when building paths using the user-provided
guestmount configuration.
Fixes: a1645ce12adb6c9c ("perf: 'perf kvm' tool for monitoring guest performance from host")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Zhang, Yanmin <yanmin_zhang@linux.intel.com>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 9329d319bd033..0d2ebf6a84bcf 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
[ ... ]
> @@ -1260,9 +1260,9 @@ int machines__create_guest_kernel_maps(struct machines *machines)
> namelist[i]->d_name);
> continue;
> }
[Severity: High]
This isn't a bug introduced by this patch, but does the strtol() parsing in
machines__create_guest_kernel_maps() handle errno correctly?
Since errno is not explicitly cleared to 0 before the strtol() call right
above this continue statement, if a previous directory name caused an
overflow, errno will remain set to ERANGE.
This would cause the (errno == ERANGE) check to evaluate to true for all
subsequent valid directory PIDs, erroneously skipping them. Could we set
errno = 0 before parsing?
> - sprintf(path, "%s/%s/proc/kallsyms",
> - symbol_conf.guestmount,
> - namelist[i]->d_name);
> + snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
> + symbol_conf.guestmount,
> + namelist[i]->d_name);
> ret = access(path, R_OK);
> if (ret) {
> pr_debug("Can't access file %s\n", path);
[Severity: High]
This is also a pre-existing issue, but does this access failure in
machines__create_guest_kernel_maps() intend to abort the entire
initialization loop?
If access() fails here, the code executes a goto failure. This means one
inaccessible guest directory or missing proc/kallsyms will abort the loop
and skip all subsequent valid guest map directories.
Earlier in the loop, invalid directory names use continue to safely skip
just that entry. Would it be better to continue instead of aborting the
processing of all remaining maps?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616022715.5739-1-acme@kernel.org?part=2
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/9] perf cs-etm: Validate num_cpu before metadata allocation
2026-06-16 2:27 [PATCHES v4 0/9] perf tools: Fix pre-existing bugs in machine, cs-etm, c2c, bpf, and dso Arnaldo Carvalho de Melo
2026-06-16 2:27 ` [PATCH 1/9] perf machine: Propagate machine__init() error to callers Arnaldo Carvalho de Melo
2026-06-16 2:27 ` [PATCH 2/9] perf machine: Use snprintf() for guestmount path construction Arnaldo Carvalho de Melo
@ 2026-06-16 2:27 ` Arnaldo Carvalho de Melo
2026-06-16 2:40 ` sashiko-bot
2026-06-16 2:27 ` [PATCH 4/9] perf cs-etm: Require full global header in auxtrace_info size check Arnaldo Carvalho de Melo
` (5 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-06-16 2:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, sashiko-bot, James Clark, Leo Yan,
Tor Jeremiassen, Claude
From: Arnaldo Carvalho de Melo <acme@redhat.com>
cs_etm__process_auxtrace_info_full() reads num_cpu from untrusted
perf.data and uses it to allocate the metadata pointer array:
metadata = zalloc(sizeof(*metadata) * num_cpu);
On 32-bit, sizeof(*metadata) is 4, so num_cpu = 0x40000000 overflows
the multiplication to 0, causing zalloc(0) to return a valid zero-sized
allocation followed by out-of-bounds writes in the population loop.
Fix by computing priv_size early and using it to bound num_cpu: each
CPU needs at least one u64 metadata entry, so num_cpu cannot exceed
the total number of u64 entries in the event's private data area.
Fixes: cd8bfd8c973eaff8 ("perf tools: Add processing of coresight metadata")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: James Clark <james.clark@arm.com>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Tor Jeremiassen <tor@ti.com>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/cs-etm.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 0927b0b9c06b1504..d121c8f22028d5ba 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -3431,6 +3431,18 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
/* First the global part */
ptr = (u64 *) auxtrace_info->priv;
num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff;
+
+ /*
+ * Bound num_cpu by the event size: the global header consumes
+ * CS_ETM_HEADER_SIZE bytes, and each CPU needs at least one u64
+ * metadata entry after that.
+ */
+ priv_size = total_size - event_header_size - INFO_HEADER_SIZE -
+ CS_ETM_HEADER_SIZE;
+ if (num_cpu <= 0 || priv_size <= 0 ||
+ num_cpu > priv_size / (int)sizeof(u64))
+ return -EINVAL;
+
metadata = zalloc(sizeof(*metadata) * num_cpu);
if (!metadata)
return -ENOMEM;
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/9] perf cs-etm: Validate num_cpu before metadata allocation
2026-06-16 2:27 ` [PATCH 3/9] perf cs-etm: Validate num_cpu before metadata allocation Arnaldo Carvalho de Melo
@ 2026-06-16 2:40 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-16 2:40 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Insufficient bounds checking in the per-CPU metadata parsing loop allows arbitrary out-of-bounds reads based on untrusted event data.
- [Medium] Memory allocated for `etm->queues` array elements (`etmq`) is leaked when initialization fails on the error path.
--
commit 3997f8cbed57aaf438aeb6594523932b36c0d9d5
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
perf cs-etm: Validate num_cpu before metadata allocation
This commit computes priv_size early to bound num_cpu against the private
data area size. This prevents a 32-bit multiplication overflow and
subsequent out-of-bounds writes when allocating the metadata array.
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 0927b0b9c06b1..d121c8f22028d 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -3431,6 +3431,18 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
> /* First the global part */
> ptr = (u64 *) auxtrace_info->priv;
> num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff;
> +
> + /*
> + * Bound num_cpu by the event size: the global header consumes
> + * CS_ETM_HEADER_SIZE bytes, and each CPU needs at least one u64
> + * metadata entry after that.
> + */
> + priv_size = total_size - event_header_size - INFO_HEADER_SIZE -
> + CS_ETM_HEADER_SIZE;
> + if (num_cpu <= 0 || priv_size <= 0 ||
> + num_cpu > priv_size / (int)sizeof(u64))
> + return -EINVAL;
> +
[Severity: High]
This is a pre-existing issue, but does the new validation fully prevent
out-of-bounds reads during metadata parsing?
While the check ensures a minimum of one 8-byte entry per CPU, it doesn't
protect the inner parsing loop. When cs_etm__create_meta_blk() is called,
it reads nr_in_params directly from the untrusted perf.data file payload:
tools/perf/util/cs-etm.c:cs_etm__create_meta_blk()
...
nr_in_params = buff_in[i + CS_ETM_NR_TRC_PARAMS];
...
for (k = 0; k < nr_in_params; k++)
metadata[k] = buff_in[i + k];
...
Could a maliciously crafted perf.data file provide an arbitrarily large
nr_in_params, causing an out-of-bounds read before the index i is finally
validated against priv_size at the end of the function?
> metadata = zalloc(sizeof(*metadata) * num_cpu);
> if (!metadata)
> return -ENOMEM;
[Severity: Medium]
This is a pre-existing issue, but does the error handling in this function
leak memory?
During initialization, cs_etm__setup_queue() allocates a struct
cs_etm_queue and assigns it to the .priv pointer. If initialization fails
later, the code branches here:
tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_info_full()
...
err_free_queues:
auxtrace_queues__free(&etm->queues);
Since auxtrace_queues__free() is a generic helper that doesn't clean up
subsystem-specific .priv contexts, will these dynamically allocated
queue structures be leaked when the error path is taken?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616022715.5739-1-acme@kernel.org?part=3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/9] perf cs-etm: Require full global header in auxtrace_info size check
2026-06-16 2:27 [PATCHES v4 0/9] perf tools: Fix pre-existing bugs in machine, cs-etm, c2c, bpf, and dso Arnaldo Carvalho de Melo
` (2 preceding siblings ...)
2026-06-16 2:27 ` [PATCH 3/9] perf cs-etm: Validate num_cpu before metadata allocation Arnaldo Carvalho de Melo
@ 2026-06-16 2:27 ` Arnaldo Carvalho de Melo
2026-06-16 2:43 ` sashiko-bot
2026-06-16 2:27 ` [PATCH 5/9] perf cs-etm: Bounds-check CPU in cs_etm__get_queue() Arnaldo Carvalho de Melo
` (4 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-06-16 2:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, sashiko-bot, James Clark, Leo Yan,
Claude
From: Arnaldo Carvalho de Melo <acme@redhat.com>
cs_etm__process_auxtrace_info() checks that header.size covers
event_header_size + INFO_HEADER_SIZE (16 bytes total), but then
accesses ptr[CS_PMU_TYPE_CPUS] at offset 24 from the start of the
event. A crafted 16-byte auxtrace_info event passes the size check
but reads out-of-bounds.
Include CS_ETM_HEADER_SIZE in the minimum size check so that the
global header entries (version, pmu_type_cpus, snapshot) are
guaranteed to fit within the event.
Fixes: 55c1de9973d66516 ("perf cs-etm: Print auxtrace info even if OpenCSD isn't linked")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: James Clark <james.clark@arm.com>
Cc: Leo Yan <leo.yan@linaro.org>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/cs-etm-base.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/cs-etm-base.c b/tools/perf/util/cs-etm-base.c
index 4abe416e3febd2cc..aebef71d3a0a1d7b 100644
--- a/tools/perf/util/cs-etm-base.c
+++ b/tools/perf/util/cs-etm-base.c
@@ -170,7 +170,9 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
u64 *ptr = NULL;
u64 hdr_version;
- if (auxtrace_info->header.size < (event_header_size + INFO_HEADER_SIZE))
+ /* Ensure priv[] is large enough for the global header entries */
+ if (auxtrace_info->header.size < (event_header_size + INFO_HEADER_SIZE +
+ CS_ETM_HEADER_SIZE))
return -EINVAL;
/* First the global part */
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/9] perf cs-etm: Require full global header in auxtrace_info size check
2026-06-16 2:27 ` [PATCH 4/9] perf cs-etm: Require full global header in auxtrace_info size check Arnaldo Carvalho de Melo
@ 2026-06-16 2:43 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-16 2:43 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Medium] Unbounded memory accesses during per-CPU metadata parsing and printing allow out-of-bounds reads on maliciously crafted perf.data files.
--
commit bac918b1be09ad8eb36712cc82561391a2657983
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
perf cs-etm: Require full global header in auxtrace_info size check
cs_etm__process_auxtrace_info() checks that header.size covers
event_header_size + INFO_HEADER_SIZE (16 bytes total), but then
accesses ptr[CS_PMU_TYPE_CPUS] at offset 24 from the start of the
event. A crafted 16-byte auxtrace_info event passes the size check
but reads out-of-bounds.
Include CS_ETM_HEADER_SIZE in the minimum size check so that the
global header entries (version, pmu_type_cpus, snapshot) are
guaranteed to fit within the event.
> diff --git a/tools/perf/util/cs-etm-base.c b/tools/perf/util/cs-etm-base.c
> index 4abe416e3febd..aebef71d3a0a1 100644
> --- a/tools/perf/util/cs-etm-base.c
> +++ b/tools/perf/util/cs-etm-base.c
> @@ -170,7 +170,9 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
> u64 *ptr = NULL;
> u64 hdr_version;
>
> - if (auxtrace_info->header.size < (event_header_size + INFO_HEADER_SIZE))
> + /* Ensure priv[] is large enough for the global header entries */
> + if (auxtrace_info->header.size < (event_header_size + INFO_HEADER_SIZE +
> + CS_ETM_HEADER_SIZE))
> return -EINVAL;
[ ... ]
> if (dump_trace) {
> num_cpu = ptr[CS_PMU_TYPE_CPUS] & 0xffffffff;
> cs_etm__print_auxtrace_info(ptr, num_cpu);
> }
>
> return cs_etm__process_auxtrace_info_full(event, session);
> }
[Severity: Medium]
This is a pre-existing issue, but does this still leave an out-of-bounds read
if the event size only covers the global header, and the parsed num_cpu value
requires more data?
Looking at the dump_trace path above, cs_etm__print_auxtrace_info() uses
num_cpu as a loop boundary:
tools/perf/util/cs-etm-base.c:cs_etm__print_auxtrace_info() {
...
for (i = CS_HEADER_VERSION_MAX; cpu < num; cpu++) {
...
}
Could this read and print memory out of bounds if the remaining event size
isn't checked to ensure it contains all the variable-length per-CPU
parameters?
Additionally, in cs_etm__process_auxtrace_info_full(), num_cpu is checked
against the total size:
tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_info_full() {
...
if (num_cpu > priv_size / (int)sizeof(u64))
return -EINVAL;
...
}
Does this check only guarantee a single 64-bit word per CPU? If
cs_etm__create_meta_blk() later consumes multiple words per CPU and
dynamically advances the stream offset based on untrusted payload counts like
nr_in_params, could this also result in reading past the end of the event
payload before the final size match validation catches it?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616022715.5739-1-acme@kernel.org?part=4
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/9] perf cs-etm: Bounds-check CPU in cs_etm__get_queue()
2026-06-16 2:27 [PATCHES v4 0/9] perf tools: Fix pre-existing bugs in machine, cs-etm, c2c, bpf, and dso Arnaldo Carvalho de Melo
` (3 preceding siblings ...)
2026-06-16 2:27 ` [PATCH 4/9] perf cs-etm: Require full global header in auxtrace_info size check Arnaldo Carvalho de Melo
@ 2026-06-16 2:27 ` Arnaldo Carvalho de Melo
2026-06-16 2:48 ` sashiko-bot
2026-06-16 2:27 ` [PATCH 6/9] perf c2c: Free format list entries when c2c_hists__init() fails Arnaldo Carvalho de Melo
` (3 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-06-16 2:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, sashiko-bot, James Clark, Leo Yan,
Claude
From: Arnaldo Carvalho de Melo <acme@redhat.com>
cs_etm__get_queue() indexes etm->queues.queue_array[cpu] without
validating that cpu is within nr_queues. When processing
AUX_OUTPUT_HW_ID events, the cpu value comes from untrusted perf.data
trace payload and flows through cs_etm__process_trace_id_v0_1() and
cs_etm__queue_aux_fragment() without bounds checking, allowing an
out-of-bounds read with a crafted file.
Add a bounds check in cs_etm__get_queue() and NULL checks in all
callers.
Also add NULL checks for queue_array[i].priv in the queue iteration
loops in cs_etm__map_trace_id_v0() and cs_etm__process_trace_id_v0_1()
— after auxtrace_queues__grow() new entries are zero-initialized so
.priv can be NULL. Add a get_cpu_data() NULL check in
cs_etm__process_trace_id_v0_1(), matching the existing check in
cs_etm__process_trace_id_v0().
Fixes: 77c123f53e97ad4b ("perf: cs-etm: Move traceid_list to each queue")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: James Clark <james.clark@arm.com>
Cc: Leo Yan <leo.yan@linaro.org>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/cs-etm.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index d121c8f22028d5ba..5d0664ff73b79122 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -292,8 +292,11 @@ static struct cs_etm_queue *cs_etm__get_queue(struct cs_etm_auxtrace *etm, int c
{
if (etm->per_thread_decoding)
return etm->queues.queue_array[0].priv;
- else
- return etm->queues.queue_array[cpu].priv;
+
+ if (cpu < 0 || cpu >= (int)etm->queues.nr_queues)
+ return NULL;
+
+ return etm->queues.queue_array[cpu].priv;
}
static int cs_etm__map_trace_id_v0(struct cs_etm_auxtrace *etm, u8 trace_chan_id,
@@ -306,6 +309,9 @@ static int cs_etm__map_trace_id_v0(struct cs_etm_auxtrace *etm, u8 trace_chan_id
* queue associated with that CPU so only one decoder is made.
*/
etmq = cs_etm__get_queue(etm, cpu_metadata[CS_ETM_CPU]);
+ if (!etmq)
+ return -EINVAL;
+
if (etmq->format == UNFORMATTED)
return cs_etm__insert_trace_id_node(etmq, trace_chan_id,
cpu_metadata);
@@ -318,6 +324,9 @@ static int cs_etm__map_trace_id_v0(struct cs_etm_auxtrace *etm, u8 trace_chan_id
int ret;
etmq = etm->queues.queue_array[i].priv;
+ if (!etmq)
+ continue;
+
ret = cs_etm__insert_trace_id_node(etmq, trace_chan_id,
cpu_metadata);
if (ret)
@@ -358,6 +367,9 @@ static int cs_etm__process_trace_id_v0_1(struct cs_etm_auxtrace *etm, int cpu,
u32 sink_id = FIELD_GET(CS_AUX_HW_ID_SINK_ID_MASK, hw_id);
u8 trace_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id);
+ if (!etmq)
+ return -EINVAL;
+
/*
* Check sink id hasn't changed in per-cpu mode. In per-thread mode,
* let it pass for now until an actual overlapping trace ID is hit. In
@@ -375,6 +387,9 @@ static int cs_etm__process_trace_id_v0_1(struct cs_etm_auxtrace *etm, int cpu,
for (unsigned int i = 0; i < etm->queues.nr_queues; ++i) {
struct cs_etm_queue *other_etmq = etm->queues.queue_array[i].priv;
+ if (!other_etmq)
+ continue;
+
/* Different sinks, skip */
if (other_etmq->sink_id != etmq->sink_id)
continue;
@@ -396,6 +411,9 @@ static int cs_etm__process_trace_id_v0_1(struct cs_etm_auxtrace *etm, int cpu,
}
cpu_data = get_cpu_data(etm, cpu);
+ if (!cpu_data)
+ return -EINVAL;
+
ret = cs_etm__insert_trace_id_node(etmq, trace_id, cpu_data);
if (ret)
return ret;
@@ -3144,6 +3162,9 @@ static int cs_etm__queue_aux_fragment(struct perf_session *session, off_t file_o
aux_offset + aux_size <= auxtrace_event->offset + auxtrace_event->size) {
struct cs_etm_queue *etmq = cs_etm__get_queue(etm, auxtrace_event->cpu);
+ if (!etmq)
+ return -EINVAL;
+
/*
* If this AUX event was inside this buffer somewhere, create a new auxtrace event
* based on the sizes of the aux event, and queue that fragment.
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/9] perf cs-etm: Bounds-check CPU in cs_etm__get_queue()
2026-06-16 2:27 ` [PATCH 5/9] perf cs-etm: Bounds-check CPU in cs_etm__get_queue() Arnaldo Carvalho de Melo
@ 2026-06-16 2:48 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-16 2:48 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Integer overflow in auxtrace_queues__queue_buffer() leads to out-of-bounds memory access.
- [Medium] Null pointer dereference in cs_etm__dump_event() when processing a crafted perf.data file.
- [Medium] Out-of-bounds read in cs_etm__create_meta_blk() when parsing CPU metadata blocks.
--
commit 6ff616c3e2c2577ec55eda8099842f63dcc43ed7
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
perf cs-etm: Bounds-check CPU in cs_etm__get_queue()
cs_etm__get_queue() indexes etm->queues.queue_array[cpu] without
validating that cpu is within nr_queues. When processing
AUX_OUTPUT_HW_ID events, the cpu value comes from untrusted perf.data
trace payload and flows through cs_etm__process_trace_id_v0_1() and
cs_etm__queue_aux_fragment() without bounds checking, allowing an
out-of-bounds read with a crafted file.
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index d121c8f22028d..5d0664ff73b79 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
[ ... ]
> @@ -318,6 +324,9 @@ static int cs_etm__map_trace_id_v0(struct cs_etm_auxtrace *etm, u8 trace_chan_id
> int ret;
>
> etmq = etm->queues.queue_array[i].priv;
> + if (!etmq)
> + continue;
> +
[Severity: Medium]
This is a pre-existing issue, but since we are adding NULL checks for
zero-initialized queues here, could this also cause a null pointer
dereference in cs_etm__dump_event()?
If dump_trace is enabled, cs_etm__process_auxtrace_event() and
dump_queued_data() can pass the new queue's priv pointer directly:
tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_event() {
...
cs_etm__dump_event(etm->queues.queue_array[idx].priv, buffer);
...
}
And cs_etm__dump_event() unconditionally dereferences it:
tools/perf/util/cs-etm.c:cs_etm__dump_event() {
...
cs_etm_decoder__get_name(etmq->decoder)
...
}
Should cs_etm__dump_event() also have a NULL check for etmq?
[ ... ]
> @@ -3144,6 +3162,9 @@ static int cs_etm__queue_aux_fragment(struct perf_session *session, off_t file_o
> aux_offset + aux_size <= auxtrace_event->offset + auxtrace_event->size) {
> struct cs_etm_queue *etmq = cs_etm__get_queue(etm, auxtrace_event->cpu);
>
> + if (!etmq)
> + return -EINVAL;
> +
[Severity: High]
This is a pre-existing issue, but I noticed a potential integer overflow in
auxtrace_queues__queue_buffer() that could lead to out-of-bounds memory access.
When processing an untrusted perf.data file, if the idx field of a
PERF_RECORD_AUXTRACE event is UINT_MAX, does the idx + 1 calculation
overflow to 0?
tools/perf/util/auxtrace.c:auxtrace_queues__queue_buffer() {
...
if (idx >= queues->nr_queues) {
err = auxtrace_queues__grow(queues, idx + 1);
if (err)
return err;
}
queue = &queues->queue_array[idx];
...
}
Since auxtrace_queues__grow() evaluates nr_queues < new_nr_queues as false
when new_nr_queues is 0, it returns without growing the array. Does this
cause the subsequent array access at queues->queue_array[UINT_MAX] to
write out of bounds?
[Severity: Medium]
This is a pre-existing issue, but there appears to be a potential out-of-bounds
read in cs_etm__create_meta_blk() when parsing CPU metadata blocks.
When parsing a PERF_RECORD_AUXTRACE_INFO event, does
cs_etm__process_auxtrace_info_full() validate that the payload contains
enough bytes before calling cs_etm__create_meta_blk()?
tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_info_full() {
...
metadata[j] =
cs_etm__create_meta_blk(ptr, &i,
CS_ETMV4_PRIV_MAX,
CS_ETMV4_NR_TRC_PARAMS_V0);
...
}
A crafted file with a large num_cpu and small auxtrace_info.header.size
might bypass the num_cpu > priv_size / 8 check. cs_etm__create_meta_blk()
then unconditionally reads up to CS_ETMV4_NR_TRC_PARAMS_V0 elements:
tools/perf/util/cs-etm.c:cs_etm__create_meta_blk() {
...
metadata[k + 1] = buff_in[i + k];
...
}
Since the length validation (i * 8 != priv_size) only happens after the
loop finishes, can this cause a heap buffer over-read of buff_in?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616022715.5739-1-acme@kernel.org?part=5
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/9] perf c2c: Free format list entries when c2c_hists__init() fails
2026-06-16 2:27 [PATCHES v4 0/9] perf tools: Fix pre-existing bugs in machine, cs-etm, c2c, bpf, and dso Arnaldo Carvalho de Melo
` (4 preceding siblings ...)
2026-06-16 2:27 ` [PATCH 5/9] perf cs-etm: Bounds-check CPU in cs_etm__get_queue() Arnaldo Carvalho de Melo
@ 2026-06-16 2:27 ` Arnaldo Carvalho de Melo
2026-06-16 2:27 ` [PATCH 7/9] perf c2c: Fix hist entry and format list leaks in c2c_he_free() Arnaldo Carvalho de Melo
` (2 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-06-16 2:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, sashiko-bot, Claude
From: Arnaldo Carvalho de Melo <acme@redhat.com>
When c2c_hists__init() fails partway through hpp_list__parse(),
dynamically allocated format structures that were already added to
hists->list are leaked because he__get_c2c_hists() frees the hists
container without first unregistering the format entries.
Call perf_hpp__reset_output_field() before freeing the hists container
on the error path, matching what c2c_he_free() already does on the
normal destruction path.
Fixes: 17a7c5946d79a12c ("perf c2c report: Decode c2c_stats for hist entries")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-c2c.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 07c7e8fb315e6cf3..eabb922ef295ef86 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -226,6 +226,7 @@ he__get_c2c_hists(struct hist_entry *he,
ret = c2c_hists__init(hists, sort, nr_header_lines, env);
if (ret) {
+ perf_hpp__reset_output_field(&hists->list);
c2c_he->hists = NULL;
free(hists);
return NULL;
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 7/9] perf c2c: Fix hist entry and format list leaks in c2c_he_free()
2026-06-16 2:27 [PATCHES v4 0/9] perf tools: Fix pre-existing bugs in machine, cs-etm, c2c, bpf, and dso Arnaldo Carvalho de Melo
` (5 preceding siblings ...)
2026-06-16 2:27 ` [PATCH 6/9] perf c2c: Free format list entries when c2c_hists__init() fails Arnaldo Carvalho de Melo
@ 2026-06-16 2:27 ` Arnaldo Carvalho de Melo
2026-06-16 2:27 ` [PATCH 8/9] perf bpf: Validate array presence before casting BPF prog info pointers Arnaldo Carvalho de Melo
2026-06-16 2:27 ` [PATCH 9/9] perf dso: Set standard errno on decompression failure Arnaldo Carvalho de Melo
8 siblings, 0 replies; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-06-16 2:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, sashiko-bot, Claude
From: Arnaldo Carvalho de Melo <acme@redhat.com>
c2c_he_free() calls hists__delete_entries() which only walks the
output-sorted entries tree. During c2c resort, when cacheline entries
are merged and the redundant entry is freed, the inner hists have not
been output-resorted yet, so hists->entries is empty. The actual inner
hist_entry objects live in entries_in_array[] and entries_collapsed,
which are never walked, leaking all inner hist_entry objects for every
merged cacheline.
Additionally, the dynamically allocated format entries on hists->list
are never unregistered or freed.
Fix both issues by switching to hists__delete_all_entries() which walks
all rb_root trees, and calling perf_hpp__reset_output_field() to clean
up format entries.
Fixes: bf0e0d407ea09ce5 ("perf c2c report: Add sample processing")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-c2c.c | 2 +-
tools/perf/util/hist.c | 2 +-
tools/perf/util/hist.h | 1 +
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index eabb922ef295ef86..c9584dbedf77afe8 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -184,7 +184,7 @@ static void c2c_he_free(void *he)
c2c_he = container_of(he, struct c2c_hist_entry, he);
if (c2c_he->hists) {
- hists__delete_entries(&c2c_he->hists->hists);
+ hists__delete_all_entries(&c2c_he->hists->hists);
perf_hpp__reset_output_field(&c2c_he->hists->list);
zfree(&c2c_he->hists);
}
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index df978c996b6c2262..c93915625ee75de1 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -3041,7 +3041,7 @@ static void hists__delete_remaining_entries(struct rb_root_cached *root)
}
}
-static void hists__delete_all_entries(struct hists *hists)
+void hists__delete_all_entries(struct hists *hists)
{
hists__delete_entries(hists);
hists__delete_remaining_entries(&hists->entries_in_array[0]);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 8fb89d81ef069d95..b830cbe7f95bf597 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -391,6 +391,7 @@ int hists__collapse_resort(struct hists *hists, struct ui_progress *prog);
void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel);
void hists__delete_entries(struct hists *hists);
+void hists__delete_all_entries(struct hists *hists);
void hists__output_recalc_col_len(struct hists *hists, int max_rows);
struct hist_entry *hists__get_entry(struct hists *hists, int idx);
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 8/9] perf bpf: Validate array presence before casting BPF prog info pointers
2026-06-16 2:27 [PATCHES v4 0/9] perf tools: Fix pre-existing bugs in machine, cs-etm, c2c, bpf, and dso Arnaldo Carvalho de Melo
` (6 preceding siblings ...)
2026-06-16 2:27 ` [PATCH 7/9] perf c2c: Fix hist entry and format list leaks in c2c_he_free() Arnaldo Carvalho de Melo
@ 2026-06-16 2:27 ` Arnaldo Carvalho de Melo
2026-06-16 2:27 ` [PATCH 9/9] perf dso: Set standard errno on decompression failure Arnaldo Carvalho de Melo
8 siblings, 0 replies; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-06-16 2:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, sashiko-bot, Song Liu, Claude
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Several functions cast bpf_prog_info fields (jited_ksyms,
jited_func_lens, jited_prog_insns) from u64 to pointers and
dereference them. These fields are only valid pointers if
bpil_offs_to_addr() converted their file offsets to addresses, which
only happens when the corresponding PERF_BPIL_* bits are set in
info_linear->arrays.
A crafted perf.data can leave these bits unset while setting non-zero
counts and offset values, causing the functions to dereference raw file
offsets as pointers.
Add array bitmask validation to all perf.data processing paths:
- __bpf_event__print_bpf_prog_info(): check JITED_KSYMS and
JITED_FUNC_LENS (changed to take struct perf_bpil *)
- machine__process_bpf_event_load(): check JITED_KSYMS
- bpf_read(): check JITED_INSNS before memcpy from jited_prog_insns
- dso__disassemble_filename(): check JITED_INSNS before returning
jited_prog_insns pointer
Fixes: f8dfeae009effc0b ("perf bpf: Show more BPF program info in print_bpf_prog_info()")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Song Liu <songliubraving@fb.com>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/bpf-event.c | 20 +++++++++++++++++---
tools/perf/util/bpf-event.h | 4 ++--
tools/perf/util/dso.c | 10 ++++++++++
| 3 +--
4 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
index 57d53ba848359e12..fa3ebc8ea7f09cdd 100644
--- a/tools/perf/util/bpf-event.c
+++ b/tools/perf/util/bpf-event.c
@@ -59,6 +59,10 @@ static int machine__process_bpf_event_load(struct machine *machine,
return 0;
info_linear = info_node->info_linear;
+ /* jited_ksyms is only valid if bpil_offs_to_addr() converted it */
+ if (!(info_linear->arrays & (1UL << PERF_BPIL_JITED_KSYMS)))
+ return 0;
+
for (i = 0; i < info_linear->info.nr_jited_ksyms; i++) {
u64 *addrs = (u64 *)(uintptr_t)(info_linear->info.jited_ksyms);
u64 addr = addrs[i];
@@ -959,12 +963,15 @@ int evlist__add_bpf_sb_event(struct evlist *evlist, struct perf_env *env)
return evlist__add_sb_event(evlist, &attr, bpf_event__sb_cb, env);
}
-void __bpf_event__print_bpf_prog_info(struct bpf_prog_info *info,
+void __bpf_event__print_bpf_prog_info(struct perf_bpil *info_linear,
struct perf_env *env,
FILE *fp)
{
- __u32 *prog_lens = (__u32 *)(uintptr_t)(info->jited_func_lens);
- __u64 *prog_addrs = (__u64 *)(uintptr_t)(info->jited_ksyms);
+ struct bpf_prog_info *info = &info_linear->info;
+ __u64 required_arrays = (1UL << PERF_BPIL_JITED_KSYMS) |
+ (1UL << PERF_BPIL_JITED_FUNC_LENS);
+ __u32 *prog_lens;
+ __u64 *prog_addrs;
char name[KSYM_NAME_LEN];
struct btf *btf = NULL;
u32 sub_prog_cnt, i;
@@ -974,6 +981,13 @@ void __bpf_event__print_bpf_prog_info(struct bpf_prog_info *info,
sub_prog_cnt != info->nr_jited_func_lens)
return;
+ /* Ensure the arrays were present and converted by bpil_offs_to_addr() */
+ if ((info_linear->arrays & required_arrays) != required_arrays)
+ return;
+
+ prog_lens = (__u32 *)(uintptr_t)(info->jited_func_lens);
+ prog_addrs = (__u64 *)(uintptr_t)(info->jited_ksyms);
+
if (info->btf_id) {
struct btf_node *node;
diff --git a/tools/perf/util/bpf-event.h b/tools/perf/util/bpf-event.h
index 60d2c6637af5d6eb..da4eeb4a1a73208c 100644
--- a/tools/perf/util/bpf-event.h
+++ b/tools/perf/util/bpf-event.h
@@ -40,7 +40,7 @@ struct btf_node {
int machine__process_bpf(struct machine *machine, union perf_event *event,
struct perf_sample *sample);
int evlist__add_bpf_sb_event(struct evlist *evlist, struct perf_env *env);
-void __bpf_event__print_bpf_prog_info(struct bpf_prog_info *info,
+void __bpf_event__print_bpf_prog_info(struct perf_bpil *info_linear,
struct perf_env *env,
FILE *fp);
void bpf_metadata_free(struct bpf_metadata *metadata);
@@ -58,7 +58,7 @@ static inline int evlist__add_bpf_sb_event(struct evlist *evlist __maybe_unused,
return 0;
}
-static inline void __bpf_event__print_bpf_prog_info(struct bpf_prog_info *info __maybe_unused,
+static inline void __bpf_event__print_bpf_prog_info(struct perf_bpil *info_linear __maybe_unused,
struct perf_env *env __maybe_unused,
FILE *fp __maybe_unused)
{
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 1a2fc6d18da74d6c..79f1a30f3683d6b3 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -880,6 +880,12 @@ static ssize_t bpf_read(struct dso *dso, u64 offset, char *data)
return -1;
}
+ /* jited_prog_insns is only valid if bpil_offs_to_addr() converted it */
+ if (!(node->info_linear->arrays & (1UL << PERF_BPIL_JITED_INSNS))) {
+ dso__data(dso)->status = DSO_DATA_STATUS_ERROR;
+ return -1;
+ }
+
len = node->info_linear->info.jited_prog_len;
buf = (u8 *)(uintptr_t)node->info_linear->info.jited_prog_insns;
@@ -1995,6 +2001,10 @@ const u8 *dso__read_symbol(struct dso *dso, const char *symfs_filename,
return NULL;
}
info_linear = info_node->info_linear;
+ if (!(info_linear->arrays & (1UL << PERF_BPIL_JITED_INSNS))) {
+ errno = SYMBOL_ANNOTATE_ERRNO__BPF_MISSING_BTF;
+ return NULL;
+ }
assert(len <= info_linear->info.jited_prog_len);
*out_buf_len = len;
return (const u8 *)(uintptr_t)(info_linear->info.jited_prog_insns);
--git a/tools/perf/util/header.c b/tools/perf/util/header.c
index d7f41db7322cbcb4..091d8f7f6bd2c9d5 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2107,8 +2107,7 @@ static void print_bpf_prog_info(struct feat_fd *ff __maybe_unused, FILE *fp)
node = rb_entry(next, struct bpf_prog_info_node, rb_node);
next = rb_next(&node->rb_node);
- __bpf_event__print_bpf_prog_info(&node->info_linear->info,
- env, fp);
+ __bpf_event__print_bpf_prog_info(node->info_linear, env, fp);
}
up_read(&env->bpf_progs.lock);
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 9/9] perf dso: Set standard errno on decompression failure
2026-06-16 2:27 [PATCHES v4 0/9] perf tools: Fix pre-existing bugs in machine, cs-etm, c2c, bpf, and dso Arnaldo Carvalho de Melo
` (7 preceding siblings ...)
2026-06-16 2:27 ` [PATCH 8/9] perf bpf: Validate array presence before casting BPF prog info pointers Arnaldo Carvalho de Melo
@ 2026-06-16 2:27 ` Arnaldo Carvalho de Melo
2026-06-16 2:44 ` sashiko-bot
8 siblings, 1 reply; 20+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-06-16 2:27 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ingo Molnar, Thomas Gleixner, James Clark, Jiri Olsa, Ian Rogers,
Adrian Hunter, Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, sashiko-bot, Claude
From: Arnaldo Carvalho de Melo <acme@redhat.com>
dso__get_filename() sets errno to a negative custom DSO_LOAD_ERRNO
value when kernel module decompression fails:
errno = *dso__load_errno(dso); /* e.g. -9996 */
The caller __open_dso() then computes fd = -errno, producing a large
positive value (9996) that looks like a valid file descriptor. This
can cause close_data_fd() to close an unrelated fd used by another
subsystem.
Set errno to EIO instead. The detailed error code is already stored
in dso__load_errno(dso) for diagnostic messages.
Fixes: 1d6b3c9ba756a513 ("perf tools: Decompress kernel module when reading DSO data")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/dso.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 79f1a30f3683d6b3..2309196d8df3111c 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -600,7 +600,13 @@ static char *dso__get_filename(struct dso *dso, const char *root_dir,
size_t len = sizeof(newpath);
if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
- errno = *dso__load_errno(dso);
+ /*
+ * Use a standard errno value, not the negative custom
+ * DSO_LOAD_ERRNO stored in dso__load_errno(dso):
+ * __open_dso() computes fd = -errno, so a negative
+ * errno produces a positive fd that looks valid.
+ */
+ errno = EIO;
goto out;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 9/9] perf dso: Set standard errno on decompression failure
2026-06-16 2:27 ` [PATCH 9/9] perf dso: Set standard errno on decompression failure Arnaldo Carvalho de Melo
@ 2026-06-16 2:44 ` sashiko-bot
0 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-06-16 2:44 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] If `dso__get_filename()` returns NULL without explicitly setting `errno` (e.g., when the target path is a directory and `is_regular_file()` returns false without altering `errno`, or when `dso__has_build_id()` is false), `__open_dso()` computes `fd = -errno`, resulting in `fd = 0`. The subsequent check `if (fd >= 0)` in `open_dso()` incorrectly treats this as a successful open and registers standard input (fd 0) as the DSO data file descriptor.
- [High] Under `REFCNT_CHECKING`, `dso__list_del()` removes a DSO from the open list and temporarily unlocks `_dso__data_open_lock` before decrementing `dso__data_open_cnt`. This breaks the invariant between the list length and the counter. If a concurrent thread calls `close_first_dso()`, it observes `dso__data_open_cnt > 0` but reads from an empty list.
--
commit 6181e27c050bdb8f633dfde54842ef5d8d538b5a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
perf dso: Set standard errno on decompression failure
dso__get_filename sets errno to a negative custom DSO_LOAD_ERRNO value
when kernel module decompression fails. The caller __open_dso then computes
fd = -errno, producing a large positive value that looks like a valid file
descriptor. This commit sets errno to EIO instead to avoid incorrectly
closing unrelated file descriptors.
Fixes: 1d6b3c9ba756a513 ("perf tools: Decompress kernel module when reading DSO data")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 79f1a30f3683d..2309196d8df31 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -600,7 +600,13 @@ static char *dso__get_filename(struct dso *dso, const char *root_dir,
> size_t len = sizeof(newpath);
>
> if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
> - errno = *dso__load_errno(dso);
> + /*
> + * Use a standard errno value, not the negative custom
> + * DSO_LOAD_ERRNO stored in dso__load_errno(dso):
> + * __open_dso() computes fd = -errno, so a negative
> + * errno produces a positive fd that looks valid.
> + */
> + errno = EIO;
> goto out;
> }
[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing errno
handling here, could a similar issue occur earlier in dso__get_filename
when it fails without explicitly setting errno?
If stat succeeds on a directory, errno is 0 but S_ISREG fails.
is_regular_file returns false and errno remains 0, jumping to out:
tools/perf/util/dso.c:dso__get_filename() {
...
if (!is_regular_file(name)) {
char *new_name;
if (errno != ENOENT || dso__nsinfo(dso) == NULL)
goto out;
...
}
In this case, __open_dso computes fd = -errno, resulting in fd = 0:
tools/perf/util/dso.c:__open_dso() {
...
name = dso__get_filename(dso, machine ? machine->root_dir : "", &decomp);
if (name)
fd = do_open(name);
else
fd = -errno;
...
}
Then open_dso incorrectly treats this as a successful open and registers
standard input as the DSO data file descriptor:
tools/perf/util/dso.c:open_dso() {
...
fd = __open_dso(dso, machine);
if (fd >= 0) {
dso__list_add(dso);
...
}
Could this lead to resource hijacking or blocking on stdin?
[Severity: High]
This is another pre-existing issue, but I noticed a potential race condition
with the DSO lists.
Under REFCNT_CHECKING, dso__list_del removes a DSO from the open list and
temporarily unlocks dso__data_open_lock before decrementing the counter:
tools/perf/util/dso.c:dso__list_del() {
...
list_del_init(&dso__data(dso)->open_entry);
#ifdef REFCNT_CHECKING
mutex_unlock(dso__data_open_lock());
dso__put(dso__data(dso)->dso);
mutex_lock(dso__data_open_lock());
#endif
WARN_ONCE(dso__data_open_cnt <= 0, ...);
dso__data_open_cnt--;
...
}
If a concurrent thread calls close_first_dso, it checks the counter rather
than checking if the list is empty:
tools/perf/util/dso.c:close_first_dso() {
...
dso_data = list_first_entry(&dso__data_open, struct dso_data, open_entry);
#ifdef REFCNT_CHECKING
dso = dso_data->dso;
#else
...
close_dso(dso);
...
}
Because dso__data_open_cnt is greater than zero even when the list is
empty, list_first_entry computes an invalid pointer from the list head.
This causes dso_data->dso to evaluate to the address of
dso__data_open_cnt, treating the integer as a struct pointer and
dereferencing it.
Can this result in a concurrent segmentation fault under REFCNT_CHECKING
builds?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260616022715.5739-1-acme@kernel.org?part=9
^ permalink raw reply [flat|nested] 20+ messages in thread