[PATCH 1/3] perf build-id: add shared is_valid_elf() helper

[PATCH 1/3] perf build-id: add shared is_valid_elf() helper

[Athira Rajeev]

Add is_valid_elf() to build-id.c and declare it in build-id.h so that
multiple perf subsystems can verify ELF magic without duplicating the
check.  The helper opens the file, reads the first SELFMAG (4) bytes,
and compares them against ELFMAG using memcmp().

Both <elf.h> and <fcntl.h> are already included in build-id.c, so no
new dependencies are added.

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 | 14 ++++++++++++++
 tools/perf/util/build-id.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index eb95ab90f974..28b6b3f8d5d3 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)
+{
+	unsigned char magic[SELFMAG];
+	int fd = open(filename, O_RDONLY);
+	bool valid = false;
+
+	if (fd < 0)
+		return false;
+	if (read(fd, magic, sizeof(magic)) == (ssize_t)sizeof(magic))
+		valid = (memcmp(magic, ELFMAG, SELFMAG) == 0);
+	close(fd);
+	return valid;
+}
+
 static int mark_dso_hit_callback(struct callchain_cursor_node *node, void *data __maybe_unused)
 {
 	struct map *map = node->ms.map;
diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
index 73bad90b06f9..dd5b4e3a4d3e 100644
--- a/tools/perf/util/build-id.h
+++ b/tools/perf/util/build-id.h
@@ -76,4 +76,6 @@ extern char buildid_dir[];
 void set_buildid_dir(const char *dir);
 void disable_buildid_cache(void);
 
+bool is_valid_elf(const char *filename);
+
 #endif
-- 
2.43.0

[PATCH 2/3] perf annotate: fall back to original binary if build-id cache entry is not a valid ELF

[Athira Rajeev]

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

[PATCH 3/3] perf buildid-cache: skip caching non-ELF files and treat unreadable build-id as mismatch

[Athira Rajeev]

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