Re: [PATCH 16/23] perf tools: Use snprintf() in dso__read_running_kernel_build_id()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Heap buffer overflow when replacing the DSO path with a decompressed file path in `dso__get_filename`.
- [High] Unmitigated stack buffer overflows in sibling functions formatting `PATH_MAX` paths.
- [Medium] Infinite CPU-spinning loop when reading ELF notes from a zero-filled stream.
--


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

perf tools: Use snprintf() in dso__read_running_kernel_build_id()

This commit replaces sprintf() with snprintf() in 
dso__read_running_kernel_build_id() to prevent a stack buffer overflow 
when appending /sys/kernel/notes to a root_dir that is close to PATH_MAX 
in length.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 6a34717c9f31f..5d01797587381 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1779,7 +1779,7 @@ void dso__read_running_kernel_build_id(struct dso *dso, struct machine *machine)
>  
>  	if (machine__is_default_guest(machine))
>  		return;
> -	sprintf(path, "%s/sys/kernel/notes", machine->root_dir);
> +	snprintf(path, sizeof(path), "%s/sys/kernel/notes", machine->root_dir);

[Severity: High]
This is a pre-existing issue, but while we are fixing sprintf() overflows 
in PATH_MAX buffers, are there similar unmitigated buffer overflows in 
sibling functions?

For instance, in tools/perf/util/machine.c:get_kernel_version():

    sprintf(version, "%s/proc/version", root_dir);

And in tools/perf/util/symbol.c:dso__load_guest_kernel_sym():

    sprintf(path, "%s/proc/kallsyms", machine->root_dir);

If the root_dir or symbol_conf.guestmount paths are close to PATH_MAX, 
appending these suffix strings could write past the stack bounds in those 
locations as well.

>  	sysfs__read_build_id(path, &bid);

[Severity: Medium]
This is a pre-existing issue, but does sysfs__read_build_id() contain an
infinite loop if the target stream is filled with zeros?

In tools/perf/util/symbol-elf.c:sysfs__read_build_id():

    } else {
        n = namesz + descsz;
    }
    if (read(fd, bf, n) != (ssize_t)n)
        break;

If read(fd, &nhdr, sizeof(nhdr)) reads zeroed headers from a stream, 
namesz and descsz evaluate to 0, making n = 0. The subsequent read(fd, bf, 0)
would succeed and return 0, bypassing the break condition and potentially
causing a CPU hang.

>  	dso__set_build_id(dso, &bid);
>  }

[Severity: High]
This is a pre-existing issue, but could there be a heap buffer overflow
in dso__get_filename() when dealing with decompressed modules?

In tools/perf/util/dso.c:dso__get_filename():

    if (newpath[0] != '\0') {
        *decomp = true;
        strcpy(name, newpath);
    }

Earlier in the function, dso__filename_with_chroot() might allocate the name
buffer using asprintf(), making it exactly the length of the chrooted
filename.

If the module requires decompression, dso__decompress_kmodule_path()
creates a temporary file like /tmp/perf-kmod-XXXXXX.

If the original dynamically allocated name string was shorter than the
newpath string length, the strcpy() call will overwrite the heap buffer.

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