Re: [PATCH 1/4] perf header: Add section bounds checking to the fd read path

[sashiko-bot@kernel.org]

Sashiko AI review found 6 potential issue(s):
- [Critical] Pre-existing: Stack buffer overflow and integer underflow in `perf_header__read_build_ids()`.
- [High] Uninitialized `ff.size` causes `perf report --header` to fail silently.
- [High] Signed bounds check bypass in `__do_read()` leads to heap buffer overflow on 32-bit platforms.
- [High] Pre-existing: Information leak due to missing null-termination in `perf_header__read_build_ids()`.
- [High] Pre-existing: Heap out-of-bounds read due to missing null-termination in `do_read_string()`.
- [Medium] Incomplete fix: `process_build_id()` bypasses the newly introduced bounds checking.
--

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

perf header: Add section bounds checking to the fd read path
    
This commit adds section bounds checking uniformly to both the fd and buf read
paths in __do_read(), replacing the previous check that was only in
__do_read_buf(). It also updates perf_file_section__process() to initialize
the file offset to 0 and changes process_build_id() to use lseek to discover
the file position instead of relying on the offset.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 9142a8ba40195..ca0c9e80345b2 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
[ ... ]
>  static int __do_read(struct feat_fd *ff, void *addr, ssize_t size)
>  {
> +	if (size > (ssize_t)ff->size - ff->offset)
> +		return -1;
> +

Can this signed bounds check be bypassed on 32-bit platforms? If size is
derived from an extremely large unsigned length in a malicious perf.data
file, size might implicitly cast to a negative ssize_t value. The check
would evaluate to false, and the negative size could then be cast to an
enormous size_t during the memcpy() or readn() operations, leading to a
heap buffer overflow.

Also, is it possible for ff->size to be uninitialized when called from
perf_file_section__fprintf_info()?

tools/perf/util/header.c:perf_file_section__fprintf_info() {
    ...
	ff = (struct  feat_fd) {
		.fd = fd,
		.ph = ph,
	};
    ...
}

Because the struct initialization omits the .size field, it will be
initialized to 0. This seems to cause __do_read() to fail silently when
trying to print features like HEADER_EVENT_DESC, which could break
commands like perf report --header.

>  	if (!ff->buf)
>  		return __do_read_fd(ff, addr, size);
>  	return __do_read_buf(ff, addr, size);
[ ... ]
> @@ -2655,7 +2655,12 @@ static int process_tracing_data(struct feat_fd *ff, void *data)
>  
>  static int process_build_id(struct feat_fd *ff, void *data __maybe_unused)
>  {
> -	if (perf_header__read_build_ids(ff->ph, ff->fd, ff->offset, ff->size))
> +	off_t offset = lseek(ff->fd, 0, SEEK_CUR);
> +
> +	if (offset == (off_t)-1)
> +		return -1;
> +
> +	if (perf_header__read_build_ids(ff->ph, ff->fd, offset, ff->size))
>  		pr_debug("Failed to read buildids, continuing...\n");
>  	return 0;
>  }

Does process_build_id() bypass the newly introduced bounds checking?

Because process_build_id() calls perf_header__read_build_ids(), which
reads directly from the file descriptor using readn() instead of utilizing
the __do_read() helper. This seems to bypass the bounds check added above,
allowing reads past the end of the section if the section size is
understated.

While looking at perf_header__read_build_ids(), there appear to be a few
pre-existing concerns.

Could a maliciously crafted perf.data file trigger a stack buffer overflow here?

tools/perf/util/header.c:perf_header__read_build_ids() {
    ...
	char filename[PATH_MAX];
    ...
		len = bev.header.size - sizeof(bev);
		if (readn(input, filename, len) != len)
			goto out;
    ...
}

If bev.header.size is smaller than sizeof(bev), the unsigned subtraction
could wrap to a huge positive size_t. This would then become a massive
size_t passed to readn(). Alternatively, a large user-controlled u16 value
could result in a large positive length. In either case, readn() would
write significantly more data than the 4096-byte stack buffer can hold.

Additionally, does this lack of null-termination leak information?

If the file payload doesn't contain a null byte, the stack-allocated
filename buffer will contain stack garbage at its end. When passed to
subsequent C-string functions like strdup() via
__event_process_build_id(), it could result in an out-of-bounds stack
read.

There appears to be a similar pre-existing issue in do_read_string():

tools/perf/util/header.c:do_read_string() {
    ...
	buf = malloc(len);
	if (!buf)
		return NULL;

	if (!__do_read(ff, buf, len)) {
    ...
}

Since this allocates a heap buffer of size len and reads exactly len bytes
without appending a null byte, could an attacker provide a string without
the expected zero padding? If so, the buffer would lack a null terminator,
leading to a heap out-of-bounds read when manipulated.

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