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

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Critical] The newly added bounds check in `__do_read()` is vulnerable to an integer bypass on 32-bit architectures, allowing a severe heap buffer overflow.
- [Critical] Stack buffer overflow in `perf_header__read_build_ids` and `perf_header__read_build_ids_abi_quirk` via missing size bounds checks.
- [High] Missing null-termination enforcement in `do_read_string` causes an out-of-bounds heap read and information disclosure.
- [Medium] Adding the section bounds check to `__do_read()` breaks `perf report --header` functionality because `perf_file_section__fprintf_info()` leaves `ff.size` uninitialized (zero).
--

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

perf header: Add section bounds checking to the fd read path

This commit adds a bounds check to __do_read() to prevent reading past the end
of a section when using the file descriptor read path. It centralizes the
check for both buffer and fd paths. It also updates perf_file_section__process()
and process_build_id() to support this change.

> 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;
> +

In perf_file_section__fprintf_info(), the struct feat_fd ff is
instantiated using a compound literal that only initializes .fd and .ph,
leaving .size and .offset as 0.

When feature printing callbacks (like print_event_desc()) call into this
path, will this bounds check evaluate to true (e.g., 4 > 0 - 0) and cause the
read to fail? Could this break perf report --header functionality?

Also, on 32-bit architectures, ssize_t is a 32-bit signed integer. If a
crafted perf.data file provides a large 32-bit length for a dynamic feature
(such as data_len in process_bpf_prog_info()), does the size parameter
implicitly convert to a negative ssize_t (e.g., 0xFFFFFFF0 becomes -16)?

If so, could this negative value bypass the bounds check and cause the
subsequent readn() call in __do_read_fd() to overwrite memory past the
dynamically allocated heap buffer?

[ ... ]
> @@ -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;
>  }

While looking at the build ID processing updated here, does
perf_header__read_build_ids() safely handle crafted event sizes?

If a perf.data file provides an event where bev.header.size < sizeof(bev),
could the length calculation len = bev.header.size - sizeof(bev) underflow
to a large positive value?

If it exceeds sizeof(bev) + PATH_MAX, would the subsequent
readn(input, filename, len) call overflow the filename stack buffer?

Additionally, while reviewing the string reading operations in this file
for regressions, does do_read_string() enforce null-termination?

The function reads a length len from the file, allocates exactly len bytes,
and copies the data into the buffer. If a crafted perf.data file lacks a null
terminator for a string feature, could subsequent C string operations like
strdup or printf read past the allocated buffer and cause an out-of-bounds
heap read?

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