Re: [PATCH 07/11] perf tools: Add O_CLOEXEC to open() calls in DSO and ELF code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Incomplete fix for file descriptor leaks in DSO and ELF code.

Pre-existing issues:
- [Critical] Stack buffer overflow in sysfs__read_build_id when skipping non-matching GNU notes.
--

commit 66274b4049c9a2092defa7caf237fe0295853d9c
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf tools: Add O_CLOEXEC to open() calls in DSO and ELF code

The commit adds O_CLOEXEC to open() calls in dso.c and symbol-elf.c to
prevent leaking file descriptors to child processes during symbol
resolution.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 7dced896c64ea..fb2e78fe2aa8e 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -344,7 +344,7 @@ int filename__decompress(const char *name, char *pathname,
>  	 * descriptor to the uncompressed file.
>  	 */
>  	if (!compressions[comp].is_compressed(name))
> -		return open(name, O_RDONLY);
> +		return open(name, O_RDONLY | O_CLOEXEC);
>  
>  	fd = mkstemp(tmpbuf);

[Severity: High]
Since the goal of this patch is to prevent leaking file descriptors to child
processes, should we also address the mkstemp() calls in these files?

mkstemp() creates file descriptors without O_CLOEXEC. For instance, here in
filename__decompress(), the temporary file descriptor will still be leaked.
Should mkostemp(tmpbuf, O_CLOEXEC) be used instead?

[ ... ]
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 186e6d92ac3d7..c2bdfd0003df2 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
[ ... ]
> @@ -935,7 +935,7 @@ int sysfs__read_build_id(const char *filename, struct build_id *bid)
>  	size_t size = sizeof(bid->data);
>  	int fd, err = -1;
>  
> -	fd = open(filename, O_RDONLY);
> +	fd = open(filename, O_RDONLY | O_CLOEXEC);
>  	if (fd < 0)
>  		goto out;

[Severity: Critical]
This is a pre-existing issue, but while reviewing this function, could
an unbounded read() happen later in sysfs__read_build_id() when skipping
non-matching GNU notes?

If a crafted sysfs note has a maliciously large nhdr.n_descsz, the code does:

    } else if (read(fd, bf, descsz) != (ssize_t)descsz)

Since bf is a fixed-size stack array (char bf[BUFSIZ]), could this read past
the end of bf and overwrite the stack frame if descsz exceeds BUFSIZ?

[ ... ]
> @@ -1985,7 +1985,7 @@ static int kcore__init(struct kcore *kcore, char *filename, int elfclass,
>  	if (temp)
>  		kcore->fd = mkstemp(filename);
>  	else
> -		kcore->fd = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0400);
> +		kcore->fd = open(filename, O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0400);
>  	if (kcore->fd == -1)
>  		return -1;

[Severity: High]
Similarly to the issue in filename__decompress(), if temp is true here,
mkstemp() is called without O_CLOEXEC.

There is also a similar mkstemp() call in read_gnu_debugdata() in this file
that extracts debug data without O_CLOEXEC. Should all of these be updated
to prevent leaking file descriptors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609010526.1998472-1-acme@kernel.org?part=7