perf symbol-minimal: Fix ehdr reading in filename__read_build_id

The e_ident is part of the ehdr and so reading it a second time would
mean the read ehdr was displaced by 16-bytes. Switch from stdio to
open/read/lseek syscalls for similarity with the symbol-elf version of
the function and so that later changes can alter then open flags.

Fixes: fef8f648bb47 ("perf symbol: Fix use-after-free in filename__read_build_id")
Signed-off-by: Ian Rogers <irogers@google.com>
Link: https://lore.kernel.org/r/20250823000024.724394-2-irogers@google.com
Signed-off-by: Namhyung Kim <namhyung@kernel.org>

diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index 7201494c5c20d91e3904b32990486ac1aee82305..8d41bd7842dfa31841d255eae6af190afb1891bc 100644 (file)
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -4,7 +4,6 @@
 
 #include <errno.h>
 #include <unistd.h>
-#include <stdio.h>
 #include <fcntl.h>
 #include <string.h>
 #include <stdlib.h>
@@ -88,11 +87,8 @@ int filename__read_debuglink(const char *filename __maybe_unused,
  */
 int filename__read_build_id(const char *filename, struct build_id *bid)
 {
-       FILE *fp;
-       int ret = -1;
+       int fd, ret = -1;
        bool need_swap = false, elf32;
-       u8 e_ident[EI_NIDENT];
-       int i;
        union {
                struct {
                        Elf32_Ehdr ehdr32;
@@ -103,28 +99,27 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
                        Elf64_Phdr *phdr64;
                };
        } hdrs;
-       void *phdr;
-       size_t phdr_size;
-       void *buf = NULL;
-       size_t buf_size = 0;
+       void *phdr, *buf = NULL;
+       ssize_t phdr_size, ehdr_size, buf_size = 0;
 
-       fp = fopen(filename, "r");
-       if (fp == NULL)
+       fd = open(filename, O_RDONLY);
+       if (fd < 0)
                return -1;
 
-       if (fread(e_ident, sizeof(e_ident), 1, fp) != 1)
+       if (read(fd, hdrs.ehdr32.e_ident, EI_NIDENT) != EI_NIDENT)
                goto out;
 
-       if (memcmp(e_ident, ELFMAG, SELFMAG) ||
-           e_ident[EI_VERSION] != EV_CURRENT)
+       if (memcmp(hdrs.ehdr32.e_ident, ELFMAG, SELFMAG) ||
+           hdrs.ehdr32.e_ident[EI_VERSION] != EV_CURRENT)
                goto out;
 
-       need_swap = check_need_swap(e_ident[EI_DATA]);
-       elf32 = e_ident[EI_CLASS] == ELFCLASS32;
+       need_swap = check_need_swap(hdrs.ehdr32.e_ident[EI_DATA]);
+       elf32 = hdrs.ehdr32.e_ident[EI_CLASS] == ELFCLASS32;
+       ehdr_size = (elf32 ? sizeof(hdrs.ehdr32) : sizeof(hdrs.ehdr64)) - EI_NIDENT;
 
-       if (fread(elf32 ? (void *)&hdrs.ehdr32 : (void *)&hdrs.ehdr64,
-                 elf32 ? sizeof(hdrs.ehdr32) : sizeof(hdrs.ehdr64),
-                 1, fp) != 1)
+       if (read(fd,
+                (elf32 ? (void *)&hdrs.ehdr32 : (void *)&hdrs.ehdr64) + EI_NIDENT,
+                ehdr_size) != ehdr_size)
                goto out;
 
        if (need_swap) {
@@ -138,14 +133,18 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
                        hdrs.ehdr64.e_phnum = bswap_16(hdrs.ehdr64.e_phnum);
                }
        }
-       phdr_size = elf32 ? hdrs.ehdr32.e_phentsize * hdrs.ehdr32.e_phnum
-                         : hdrs.ehdr64.e_phentsize * hdrs.ehdr64.e_phnum;
+       if ((elf32 && hdrs.ehdr32.e_phentsize != sizeof(Elf32_Phdr)) ||
+           (!elf32 && hdrs.ehdr64.e_phentsize != sizeof(Elf64_Phdr)))
+               goto out;
+
+       phdr_size = elf32 ? sizeof(Elf32_Phdr) * hdrs.ehdr32.e_phnum
+                         : sizeof(Elf64_Phdr) * hdrs.ehdr64.e_phnum;
        phdr = malloc(phdr_size);
        if (phdr == NULL)
                goto out;
 
-       fseek(fp, elf32 ? hdrs.ehdr32.e_phoff : hdrs.ehdr64.e_phoff, SEEK_SET);
-       if (fread(phdr, phdr_size, 1, fp) != 1)
+       lseek(fd, elf32 ? hdrs.ehdr32.e_phoff : hdrs.ehdr64.e_phoff, SEEK_SET);
+       if (read(fd, phdr, phdr_size) != phdr_size)
                goto out_free;
 
        if (elf32)
@@ -153,8 +152,8 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
        else
                hdrs.phdr64 = phdr;
 
-       for (i = 0; i < elf32 ? hdrs.ehdr32.e_phnum : hdrs.ehdr64.e_phnum; i++) {
-               size_t p_filesz;
+       for (int i = 0; i < (elf32 ? hdrs.ehdr32.e_phnum : hdrs.ehdr64.e_phnum); i++) {
+               ssize_t p_filesz;
 
                if (need_swap) {
                        if (elf32) {
@@ -180,8 +179,8 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
                                goto out_free;
                        buf = tmp;
                }
-               fseek(fp, elf32 ? hdrs.phdr32[i].p_offset : hdrs.phdr64[i].p_offset, SEEK_SET);
-               if (fread(buf, p_filesz, 1, fp) != 1)
+               lseek(fd, elf32 ? hdrs.phdr32[i].p_offset : hdrs.phdr64[i].p_offset, SEEK_SET);
+               if (read(fd, buf, p_filesz) != p_filesz)
                        goto out_free;
 
                ret = read_build_id(buf, p_filesz, bid, need_swap);
@@ -194,7 +193,7 @@ out_free:
        free(buf);
        free(phdr);
 out:
-       fclose(fp);
+       close(fd);
        return ret;
 }