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

Arnaldo Carvalho de Melo
2026-04-16T14:43:35Z

On Thu, Apr 16, 2026 at 12:59:15AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Will fix.

> 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)?

Will fix.

> 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?

will check

> 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?

I think Ian Rogers is working on this one.

- Arnaldo