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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Tue, Jun 09, 2026 at 01:16:26AM +0000, sashiko-bot@kernel.org wrote:
> 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?

This is fixed in the next patch series.
 
> [ ... ]
> > 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?

Ditto, as several other issues you reported in this series and in other
series.

- Arnaldo
 
> [ ... ]
> > @@ -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