Re: [PATCH 08/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 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Stack buffer overflow in `sysfs__read_build_id` due to integer overflow.
- [High] Incomplete fix for file descriptor leaks; `mkstemp()` calls still lack `O_CLOEXEC`.
- [High] Out-of-bounds memory read in `filename__read_debuglink` when parsing malformed ELF files.
--

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

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

This commit adds the O_CLOEXEC flag to open() calls in dso.c and
symbol-elf.c. The goal is to prevent file descriptors from leaking to
child processes spawned 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]
This isn't a bug introduced by this patch, but since the goal is to prevent
file descriptor leaks to child processes, should we also update the
mkstemp() calls?

Here in filename__decompress(), mkstemp() creates a temporary file
descriptor without the close-on-exec flag. This FD will be inherited by
spawned child processes. Could this be changed to use
mkostemp(tmpbuf, O_CLOEXEC)?

[ ... ]
> 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);

[Severity: Critical]
This is a pre-existing issue, but there appears to be an integer overflow
risk further down in sysfs__read_build_id() that could lead to a stack
buffer overflow.

The function reads nhdr.n_namesz and nhdr.n_descsz directly from the file.
Since these are 32-bit values, they can be arbitrarily large. The
calculation:

    int n = namesz + descsz;

can overflow the 32-bit signed integer n, resulting in a negative value.
The size check:

    if (n > (int)sizeof(bf)) {
        n = sizeof(bf);
        ...
    }

bypasses truncation because a negative number is smaller than sizeof(bf).
When read(fd, bf, n) is called, the negative n is interpreted as a massive
size_t count, potentially reading up to 2GB of data into the 8KB stack
buffer bf[BUFSIZ].

Could this be exploited if perf processes data from an untrusted root
directory?

>  	if (fd < 0)
>  		goto out;
>  
[ ... ]
> @@ -995,7 +995,7 @@ int filename__read_debuglink(const char *filename, char *debuglink,
>  	if (err >= 0)
>  		goto out;
>  
> -	fd = open(filename, O_RDONLY);
> +	fd = open(filename, O_RDONLY | O_CLOEXEC);

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of an
out-of-bounds read further down in filename__read_debuglink() when parsing
malformed ELF files?

Later in this function, the code blindly copies the section data:

    /* the start of this section is a zero-terminated string */
    strncpy(debuglink, data->d_buf, size);

If a corrupted ELF file provides a small .gnu_debuglink section without
a null terminator, strncpy() lacks bounds validation against the actual
section size (data->d_size) and could read past the end of the mapped
section data.

If this crosses into unmapped memory, could it cause a segmentation fault
and crash perf?

>  	if (fd < 0)
>  		goto out;
>  
[ ... ]
> @@ -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);

[Severity: High]
This is also a pre-existing issue, but the if (temp) branch uses mkstemp()
which lacks O_CLOEXEC. This creates an inconsistent state compared to the
else branch that correctly adds O_CLOEXEC.

Does this mkstemp() need to be replaced with mkostemp(..., O_CLOEXEC)
to fully prevent file descriptor exhaustion? (There is also another
mkstemp() call in read_gnu_debugdata() that exhibits the same behavior).

>  	if (kcore->fd == -1)
>  		return -1;
>

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