* [PATCH 2/3] perf annotate: fall back to original binary if build-id cache entry is not a valid ELF
2026-09-07 3:45 [PATCH 1/3] perf build-id: add shared is_valid_elf() helper Athira Rajeev
@ 2026-09-07 3:45 ` Athira Rajeev
2026-09-07 3:59 ` sashiko-bot
2026-09-07 3:45 ` [PATCH 3/3] perf buildid-cache: skip caching non-ELF files and treat unreadable build-id as mismatch Athira Rajeev
2026-09-07 3:53 ` [PATCH 1/3] perf build-id: add shared is_valid_elf() helper sashiko-bot
2 siblings, 1 reply; 6+ messages in thread
From: Athira Rajeev @ 2026-09-07 3:45 UTC (permalink / raw)
To: acme, jolsa, adrian.hunter, maddy, irogers, namhyung
Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, tejas05,
tshah, venkat88, narnalli, vpuliyal
When perf annotate looks up the build-id cache to find the binary to
pass to objdump, it currently only checks that the cache file exists
and is readable (access(filename, R_OK)). It does not verify that the
file is actually an ELF.
If the cache entry was populated with a non-ELF file (e.g. a shell
script placeholder left by a test harness such as SPEC CPU), objdump
receives a non-ELF file, produces no output, and perf reports an
error:
Couldn't annotate <symbol>: Internal error: Invalid -1 error code
Fix this by calling the shared is_valid_elf() helper (introduced in
the previous patch) before accepting the cache entry. If the check
fails, emit a warning and fall back to the original binary path, which
is the same behaviour as when the cache entry is missing or unreadable.
Reported-by: Narendra Nalli <narnalli@in.ibm.com>
Reported-by: Vijay Puliyala <vpuliyal@in.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
tools/perf/util/disasm.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 6cfdbabbb8c7..e263a2a8715d 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1201,6 +1201,12 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
if (len < 0)
goto fallback;
+ if (!is_valid_elf(filename)) {
+ pr_warning("build-id cache file is not a valid ELF, falling back to original binary: %s\n",
+ filename);
+ goto fallback;
+ }
+
linkname[len] = '\0';
if (strstr(linkname, DSO__NAME_KALLSYMS) ||
access(filename, R_OK)) {
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/3] perf annotate: fall back to original binary if build-id cache entry is not a valid ELF
2026-09-07 3:45 ` [PATCH 2/3] perf annotate: fall back to original binary if build-id cache entry is not a valid ELF Athira Rajeev
@ 2026-09-07 3:59 ` sashiko-bot
0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-09-07 3:59 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Spurious warning emitted for missing build-id cache files and valid kallsyms cache entries.
--
commit 4a56442e475b7351d0cee0b5ef94464e2d112fe2
Author: Athira Rajeev <atrajeev@linux.ibm.com>
perf annotate: fall back to original binary if build-id cache entry is not a valid ELF
This commit adds a check to verify that a build-id cache file is actually a
valid ELF before passing it to objdump. If the file is not a valid ELF, it
emits a warning and falls back to the original binary path.
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 6cfdbabbb8c7d..e263a2a8715dd 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -1201,6 +1201,12 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
> if (len < 0)
> goto fallback;
>
> + if (!is_valid_elf(filename)) {
> + pr_warning("build-id cache file is not a valid ELF, falling back to original binary: %s\n",
> + filename);
> + goto fallback;
> + }
> +
[Severity: Medium]
Will this emit spurious warnings for missing build-id cache files or valid
kallsyms cache entries?
When perf annotate looks up a build-id and finds a kallsyms entry (which is a
plain text copy of /proc/kallsyms, not an ELF) or a dangling symlink (missing
file), the newly added is_valid_elf() check evaluates to false.
Because this check is placed before the original access() and
strstr(..., DSO__NAME_KALLSYMS) checks, it preempts the expected silent
fallback paths and prints a warning.
> linkname[len] = '\0';
> if (strstr(linkname, DSO__NAME_KALLSYMS) ||
> access(filename, R_OK)) {
> fallback:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907034554.714-1-atrajeev@linux.ibm.com?part=2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] perf buildid-cache: skip caching non-ELF files and treat unreadable build-id as mismatch
2026-09-07 3:45 [PATCH 1/3] perf build-id: add shared is_valid_elf() helper Athira Rajeev
2026-09-07 3:45 ` [PATCH 2/3] perf annotate: fall back to original binary if build-id cache entry is not a valid ELF Athira Rajeev
@ 2026-09-07 3:45 ` Athira Rajeev
2026-09-07 4:01 ` sashiko-bot
2026-09-07 3:53 ` [PATCH 1/3] perf build-id: add shared is_valid_elf() helper sashiko-bot
2 siblings, 1 reply; 6+ messages in thread
From: Athira Rajeev @ 2026-09-07 3:45 UTC (permalink / raw)
To: acme, jolsa, adrian.hunter, maddy, irogers, namhyung
Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, tejas05,
tshah, venkat88, narnalli, vpuliyal
Two related hardening fixes for the build-id cache population path
(perf_session__cache_build_ids, called at the end of perf record):
1. build_id_cache__add(): Before hard-linking or copying a file into
the cache, verify it is a valid ELF using the shared is_valid_elf()
helper (introduced in patch 1/3). If it is not, emit a warning and
skip the cache write.
This prevents a race condition that can occur with test harnesses
such as SPEC CPU, which briefly rename the benchmark binary to a
.used.<pid> path and write a shell script placeholder there during
run-directory cleanup. If perf's cache write races with that window
-- seeing the renamed path in /proc/<pid>/maps and reading the
build-id from the still-valid inode, then copying the file after the
placeholder has been written -- the cache ends up containing the
shell script instead of the ELF. The ELF magic check catches this
at copy time.
2. dso__build_id_mismatch(): Previously, if filename__read_build_id_ns()
failed (e.g. the file is not an ELF, or has been replaced), the
function returned false (no mismatch), allowing caching to proceed
with whatever file happened to be at that path. Change the default
return value to true (mismatch) so that an unreadable build-id is
treated conservatively as a mismatch and caching is skipped.
Together these ensure that only genuine ELF binaries with a verifiable
build-id matching what perf recorded are written into the cache.
Reported-by: Narendra Nalli <narnalli@in.ibm.com>
Reported-by: Vijay Puliyala <vpuliyal@in.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
tools/perf/util/build-id.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 28b6b3f8d5d3..32ddbd5e61f6 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -684,6 +684,10 @@ build_id_cache__add(const char *sbuild_id, const char *name, const char *realnam
if (is_kallsyms) {
if (copyfile("/proc/kallsyms", filename))
goto out_free;
+ } else if (!is_valid_elf(realname)) {
+ pr_warning("build-id cache: skipping non-ELF file: %s\n",
+ realname);
+ goto out_free;
} else if (nsi && nsinfo__need_setns(nsi)) {
if (copyfile_ns(name, filename, nsi))
goto out_free;
@@ -874,7 +878,11 @@ static int filename__read_build_id_ns(const char *filename,
static bool dso__build_id_mismatch(struct dso *dso, const char *name)
{
struct build_id bid = { .size = 0, };
- bool ret = false;
+ /*
+ * Default to mismatch: if we cannot read the build-id (e.g. file
+ * replaced or not an ELF), treat it conservatively as a mismatch.
+ */
+ bool ret = true;
mutex_lock(dso__lock(dso));
if (filename__read_build_id_ns(name, &bid, dso__nsinfo(dso)) >= 0)
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 3/3] perf buildid-cache: skip caching non-ELF files and treat unreadable build-id as mismatch
2026-09-07 3:45 ` [PATCH 3/3] perf buildid-cache: skip caching non-ELF files and treat unreadable build-id as mismatch Athira Rajeev
@ 2026-09-07 4:01 ` sashiko-bot
0 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-09-07 4:01 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `is_valid_elf()` check operates in the host mount namespace on a container-namespaced path, causing false rejections of valid container ELFs.
- [High] The strict ELF magic number check in `is_valid_elf()` incorrectly rejects Portable Executable (PE) files, breaking symbol resolution for Windows binaries.
- [High] Returning an error (`-1`) when skipping a non-ELF file inadvertently aborts the entire build-id cache population for all remaining DSOs.
- [Medium] Path-based ELF validation is vulnerable to a TOCTOU race condition, undermining the patch's intended fix.
- [Low] Missing explicit POSIX header inclusions for standard libc functions used in `is_valid_elf()`, causing musl compilation failures.
--
commit f4dea4bc65d83ac2422c7f8d5b38053c12d6437c
Author: Athira Rajeev <atrajeev@linux.ibm.com>
perf buildid-cache: skip caching non-ELF files and treat unreadable build-id as mismatch
This patch hardens the build-id cache population by ensuring only valid ELF
files are cached and treating unreadable files as mismatches.
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 28b6b3f8d5d35..32ddbd5e61f65 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -684,6 +684,10 @@ build_id_cache__add(const char *sbuild_id, const char *name, const char *realnam
> if (is_kallsyms) {
> if (copyfile("/proc/kallsyms", filename))
> goto out_free;
> + } else if (!is_valid_elf(realname)) {
[Severity: High]
Does is_valid_elf() operate in the host mount namespace on a
container-namespaced path?
When profiling a workload inside a container, nsinfo__realpath() returns an
absolute path valid only within the container's mount namespace. Since
is_valid_elf() attempts to open this path in the host's mount namespace, won't
this falsely reject valid container ELFs and break symbol resolution for them?
[Severity: High]
Does this strict ELF check reject valid Windows Portable Executable (PE) files?
is_valid_elf() checks for the ELF magic:
if (read(fd, magic, sizeof(magic)) == (ssize_t)sizeof(magic))
valid = (memcmp(magic, ELFMAG, SELFMAG) == 0);
If libbfd__read_build_id() successfully extracts a build ID from a PE file,
this strict ELF magic check rejects it. Could this break profiling and symbol
resolution for Windows binaries (e.g., via Wine) because valid PE files are
rejected from the build-id cache?
[Severity: Medium]
Is this path-based ELF validation vulnerable to a TOCTOU race condition?
The validation is performed statelessly by path in is_valid_elf(realname),
completely separate from the subsequent copyfile_ns() or link() operations.
If a test harness (like SPEC CPU) replaces the binary with a placeholder in
the split-second window between is_valid_elf() closing its file descriptor
and the copy/link reopening the path, could the build-id cache still end up
containing a shell script placeholder instead of the genuine ELF binary?
[Severity: Low]
Does is_valid_elf() lack explicit POSIX header inclusions for standard libc
functions?
The is_valid_elf() helper uses libc calls like open(), read(), and close(),
but the exact POSIX headers (<fcntl.h>, <unistd.h>) appear to be missing
from the file. Could this cause build failures on musl libc systems due to
implicit header reliance?
> + pr_warning("build-id cache: skipping non-ELF file: %s\n",
> + realname);
> + goto out_free;
[Severity: High]
Will this error path abort the entire build-id cache population for all
remaining DSOs?
When build_id_cache__add() starts, err is initialized to -1. By jumping to
out_free here without clearing err, -1 is returned.
This propagates up to dso__cache_build_id(), causing the dsos__for_each_dso()
loop to immediately terminate. If a single non-ELF file (like a shell script
placeholder) is encountered, are all subsequent valid DSOs in the session left
uncached?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907034554.714-1-atrajeev@linux.ibm.com?part=3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] perf build-id: add shared is_valid_elf() helper
2026-09-07 3:45 [PATCH 1/3] perf build-id: add shared is_valid_elf() helper Athira Rajeev
2026-09-07 3:45 ` [PATCH 2/3] perf annotate: fall back to original binary if build-id cache entry is not a valid ELF Athira Rajeev
2026-09-07 3:45 ` [PATCH 3/3] perf buildid-cache: skip caching non-ELF files and treat unreadable build-id as mismatch Athira Rajeev
@ 2026-09-07 3:53 ` sashiko-bot
2 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-09-07 3:53 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The helper `is_valid_elf` is vulnerable to a Time-of-Check to Time-of-Use (TOCTOU) race condition because it verifies the file by name and immediately closes it.
- [Low] The patch uses POSIX libc functions and ELF constants without explicitly including their required standard headers, violating subsystem musl compatibility guidelines.
--
commit 61718d5b137531f7814b491a94911627f3ad9015
Author: Athira Rajeev <atrajeev@linux.ibm.com>
perf build-id: add shared is_valid_elf() helper
This patch adds an is_valid_elf() helper to build-id.c and build-id.h
to allow multiple perf subsystems to verify elf magic without duplicating
the check. The function opens the file, checks the SELFMAG bytes, and
closes the file descriptor.
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index eb95ab90f9741..28b6b3f8d5d35 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -43,6 +43,20 @@
>
> static bool no_buildid_cache;
>
> +bool is_valid_elf(const char *filename)
[Severity: Medium]
Is this helper vulnerable to a time-of-check to time-of-use race condition?
By verifying the file by name and immediately closing the file descriptor,
a local user could replace the target file between this check and when the
caller reopens or links it. This might allow invalid files to bypass the
validation and pollute the build-id cache.
Would it be safer to change the function signature to operate on a stable,
already-opened file descriptor instead of passing a filename?
> +{
> + unsigned char magic[SELFMAG];
> + int fd = open(filename, O_RDONLY);
> + bool valid = false;
[Severity: Low]
Does this code introduce missing explicit header inclusions for musl
compatibility? The subsystem guidelines require that files using libc
functions or constants directly include the standard headers where those
symbols are declared.
While the commit message notes that elf.h and fcntl.h are already included
in build-id.c, this function introduces uses of open, O_RDONLY, and
SELFMAG, without guaranteeing that fcntl.h and elf.h are explicitly and
directly included at the top of the file.
> +
> + if (fd < 0)
> + return false;
> + if (read(fd, magic, sizeof(magic)) == (ssize_t)sizeof(magic))
> + valid = (memcmp(magic, ELFMAG, SELFMAG) == 0);
> + close(fd);
[Severity: Low]
Similarly, read, close, and memcmp are used here without explicitly
including unistd.h and string.h. Can we add direct includes for these
to ensure strict header separation?
> + return valid;
> +}
> +
> static int mark_dso_hit_callback(struct callchain_cursor_node *node, void *data __maybe_unused)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907034554.714-1-atrajeev@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread