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

On Thu, Apr 16, 2026 at 12:59:15AM +0000, [email protected] 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 <[email protected]>
> 
> 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