Re: [PATCH 23/23] perf symbols: Add bounds checks to read_build_id() note iteration in minimal build

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Malicious ELF files with a crafted `p_filesz` of -1 bypass size checks and lead to a null pointer dereference or severe out-of-bounds memory access in `read_build_id()`.
- [High] Copy-paste typo clobbers the `p_filesz` field when byte-swapping 32-bit ELF program headers.
- [High] `sysfs__read_build_id()` fails to correctly read sysfs files due to reliance on exact `fstat()` sizes.
--

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

perf symbols: Add bounds checks to read_build_id() note iteration in minimal build

This commit adds bounds checking to read_build_id() in the minimal ELF parser.
It validates namesz and descsz individually against the remaining data before
advancing the pointer to prevent out-of-bounds reads from malformed ELF files.

> diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
> index 8221dc9868f7c..091071d06416e 100644
> --- a/tools/perf/util/symbol-minimal.c
> +++ b/tools/perf/util/symbol-minimal.c
[ ... ]
> @@ -44,7 +45,7 @@ static int read_build_id(void *note_data, size_t note_len, struct build_id *bid,
>  	ptr = note_data;
>  	while ((ptr + sizeof(*nhdr)) < (note_data + note_len)) {
>  		const char *name;
> -		size_t namesz, descsz;
> +		size_t namesz, descsz, remaining;
>  
>  		nhdr = ptr;
>  		if (need_swap) {

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under review,
but there is a severe out-of-bounds memory access or null pointer dereference
possible here.

If note_data is NULL and note_len is -1 (which casts to the maximum size), ptr
will be NULL and this code will deference a NULL pointer when reading
nhdr->n_namesz.

This state can be reached from filename__read_build_id() if a malicious ELF
file crafts p_filesz to be -1:

tools/perf/util/symbol-minimal.c:filename__read_build_id() {
    ...
    p_filesz = elf32 ? hdrs.phdr32[i].p_filesz : hdrs.phdr64[i].p_filesz;
    if (p_filesz > buf_size) {
        ...
        tmp = realloc(buf, buf_size);
        ...
    }
    ...
    if (read(fd, buf, p_filesz) != p_filesz)
        goto out_free;

    ret = read_build_id(buf, p_filesz, bid, need_swap);
    ...
}

Because p_filesz is evaluated as a signed ssize_t, -1 is less than buf_size
(which is 0 initially), bypassing the memory allocation and leaving buf as NULL.

The error check read(fd, buf, p_filesz) != p_filesz is also bypassed because
read(fd, NULL, -1) returns -1, which matches p_filesz.

This passes note_data = NULL and note_len = -1 into read_build_id().

Could we add stricter bounds checking on p_filesz in filename__read_build_id()
to prevent this?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a copy-paste typo in
filename__read_build_id() when byte-swapping 32-bit ELF program headers.

tools/perf/util/symbol-minimal.c:filename__read_build_id() {
    ...
    if (need_swap) {
        if (elf32) {
            hdrs.phdr32[i].p_type = bswap_32(hdrs.phdr32[i].p_type);
            hdrs.phdr32[i].p_offset = bswap_32(hdrs.phdr32[i].p_offset);
            hdrs.phdr32[i].p_filesz = bswap_32(hdrs.phdr32[i].p_offset);
        ...
    }
    ...
}

The code correctly swaps p_offset, but then assigns the swapped p_offset value
to p_filesz, instead of swapping the actual p_filesz field. This reverts the
byte swap and clobbers p_filesz with the original un-swapped p_offset.

Could this be corrected to swap hdrs.phdr32[i].p_filesz?


[Severity: High]
This is another pre-existing issue not introduced by this patch, but
sysfs__read_build_id() appears to rely on exact fstat() sizes.

tools/perf/util/symbol-minimal.c:sysfs__read_build_id() {
    ...
    if (fstat(fd, &stbuf) < 0)
        goto out;

    buf_size = stbuf.st_size;
    buf = malloc(buf_size);
    ...
    if (read(fd, buf, buf_size) != (ssize_t) buf_size)
        goto out_free;
    ...
}

Sysfs pseudo-files often report inaccurate lengths via st_size (like 0 or 4096),
regardless of their actual content size. 

By requiring the bytes read to strictly match buf_size, won't this
systematically fail for sysfs pseudo-files and incorrectly discard valid build
IDs?

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