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

[sashiko-bot@kernel.org]

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