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

[sashiko-bot@kernel.org]

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