Re: [PATCH 9/9] perf dso: Set standard errno on decompression failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Error paths in `dso__get_filename()` and its callees fail to initialize `errno` before returning `NULL`, which causes `__open_dso()` to evaluate `fd = -errno` as `0`. This mistakenly hijacks standard input (fd 0) as a valid DSO data file descriptor.
- [High] An integer underflow in `dso_cache__memcpy()` can occur when reading after a short read, causing an out-of-bounds heap memory access.
--

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

perf dso: Set standard errno on decompression failure

dso__get_filename() sets errno to a negative custom DSO_LOAD_ERRNO
value when kernel module decompression fails. This commit changes it
to set errno to EIO instead to avoid producing a valid-looking file
descriptor.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c

[ ... ]

> @@ -600,7 +600,13 @@ static char *dso__get_filename(struct dso *dso, const char *root_dir,
>  		size_t len = sizeof(newpath);
>  
>  		if (dso__decompress_kmodule_path(dso, name, newpath, len) < 0) {
> -			errno = *dso__load_errno(dso);
> +			/*
> +			 * Use a standard errno value, not the negative custom
> +			 * DSO_LOAD_ERRNO stored in dso__load_errno(dso):
> +			 * __open_dso() computes fd = -errno, so a negative
> +			 * errno produces a positive fd that looks valid.
> +			 */
> +			errno = EIO;
>  			goto out;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing how errno
is handled here, I noticed other error paths might return NULL without
initializing errno.

For example, earlier in dso__get_filename():

    if (dso__read_binary_type_filename(dso, dso__binary_type(dso),
                                root_dir, name, PATH_MAX))
        goto out;

If dso__get_filename() returns NULL and errno happens to be 0 from prior
execution, could this cause __open_dso() to accidentally hijack standard
input?

__open_dso() computes the file descriptor as:

    else
        fd = -errno;

And then open_dso() treats fd >= 0 as successful:

    if (fd >= 0) {
        dso__list_add(dso);

Would this result in fd 0 being registered in the global dso__data_open
list, causing subsequent pread() calls to read from stdin and eventually
incorrectly close(0)?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential bounds
checking problem in dso_cache__memcpy().

Is it possible for cache_offset to underflow in this sequence?

tools/perf/util/dso.c:dso_cache__memcpy() {
    ...
    u64 cache_offset = offset - cache->offset;
    u64 cache_size   = min(cache->size - cache_offset, size);

    if (out)
        memcpy(data, cache->data + cache_offset, cache_size);
    else
        memcpy(cache->data + cache_offset, data, cache_size);
    ...
}

If a previous read of the file chunk returned a short read, cache->size
will be smaller than DSO__DATA_CACHE_SIZE. If a subsequent read accesses
an offset past cache->size but within the same chunk, cache->size <
cache_offset becomes true.

Could this cause cache->size - cache_offset to underflow to a massive
positive value, causing min() to select the caller-provided size, and
leading to a heap buffer overflow during memcpy()?

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