Re: [PATCH 14/15] perf symbols: Add O_NONBLOCK to DSO open() calls for untrusted paths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch fails to comprehensively mitigate indefinite hangs caused by crafted perf.data DSO paths pointing to FIFOs because it misses numerous fallback, compression, and auxiliary code paths.
- [Low] Type punning via pointer casting violates the strict aliasing rule for tools code.
--

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

perf symbols: Add O_NONBLOCK to DSO open() calls for untrusted paths

DSO paths from perf.data MMAP events could reference FIFOs, device
nodes, or other special files.  Opening such paths without O_NONBLOCK
can hang perf report/script indefinitely waiting for a writer or device.

Fixes: a08cae03f430b971 ("perf tools: Allow to close dso fd in case of open failure")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/finding/6
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

> Add O_NONBLOCK to all open() calls in the DSO symbol resolution paths:
> filename__read_build_id(), filename__read_debuglink(), symsrc__init(),
> do_open(), filename__decompress(), and __dso__read_symbol().

[Severity: High]
This isn't a bug introduced by this patch, but are there several other
DSO resolution paths missing this protection?

For example, decompression checks seem to perform blocking open() calls
before filename__decompress() is reached:

- tools/perf/util/zlib.c:gzip_is_compressed()
- tools/perf/util/lzma.c:lzma_is_compressed()

Additionally, blocking opens appear to still be used in:

- tools/perf/util/symbol-elf.c:sysfs__read_build_id()
- tools/perf/util/capstone.c:symbol__disassemble_capstone_powerpc()
- tools/perf/util/unwind-libunwind.c:read_unwind_spec_debug_frame()

Since crafted perf.data paths ending in .xz or .gz might hit these checks
first, will they still cause an indefinite hang?

> diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
> index ea2de3d50d33c..dc4a06b2a03ad 100644
> --- a/tools/perf/util/symbol-minimal.c
> +++ b/tools/perf/util/symbol-minimal.c

[ ... ]

> @@ -234,7 +234,7 @@ int sysfs__read_build_id(const char *filename, struct build_id *bid)
>  	char buf[BUFSIZ] __aligned(4);
>  	ssize_t len;
>  
> -	fd = open(filename, O_RDONLY);
> +	fd = open(filename, O_RDONLY | O_CLOEXEC | O_NONBLOCK);
>  	if (fd < 0)
>  		return -1;

[Severity: Low]
This is a pre-existing issue, but is it possible this function violates the
strict aliasing rule for tools code?

Later in this function, the char buffer is passed into read_build_id():

    len = read(fd, buf, sizeof(buf));
    if (len > 0)
        ret = read_build_id(buf, len, bid, false);

If read_build_id() casts this buffer to a u32 struct pointer to read the
ELF note header, this would be type punning. Although perf is built with
-fno-strict-aliasing, tools code assumes standard -fstrict-aliasing per
the subsystem guidelines.

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