Re: [PATCH v7 03/23] perf evsel: Refactor evsel tracepoint sample accessors perf_sample

On Wed, Apr 08, 2026 at 08:20:22AM +0000, [email protected] wrote:
> Sashiko AI review found 1 potential issue(s):
> - [Medium] Integer overflow and incomplete validation in the newly added tracepoint bounds checks allow out-of-bounds memory accesses.
> --
> 
> commit f7dda507e65d40381a27ba2e39e45e8a6050afff
> Author: Ian Rogers <[email protected]>
> 
> perf evsel: Refactor evsel tracepoint sample accessors perf_sample
> 
> This commit refactors tracepoint sample accessors to remove the evsel
> argument. It also adds bounds checks when reading values from tracepoint
> fields based on review feedback.
> 
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 2ee87fd84d3ea..b653e683d7a12 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -3687,15 +3687,20 @@ struct tep_format_field *evsel__common_field(struct evsel *evsel, const char *na
> >  	return tp_format ? tep_find_common_field(tp_format, name) : NULL;
> >  }
> >  
> > -void *evsel__rawptr(struct evsel *evsel, struct perf_sample *sample, const char *name)
> > +void *perf_sample__rawptr(struct perf_sample *sample, const char *name)
> >  {
> > -	struct tep_format_field *field = evsel__field(evsel, name);
> > +	struct tep_format_field *field = evsel__field(sample->evsel, name);
> >  	int offset;
> >  
> >  	if (!field)
> >  		return NULL;
> >  
> >  	offset = field->offset;
> > +	if ((u32)(offset + field->size) > sample->raw_size) {
> > +		pr_warning("Invalid trace point field offset %d for field of length %d in sample raw data of size %u\n",
> > +			   offset, field->size, sample->raw_size);
> > +		return NULL;
> > +	}
> 
> Can this bounds check be bypassed due to integer overflow? If a malformed
> perf.data file provides an exceptionally large offset (for example,
> 0xFFFFFFFC) and a size of 4, the sum could wrap around to 0. Would this
> allow an out-of-bounds read when accessing the memory?

I think it's better not to include any bounds check in the patch.
It's adding a lot of review noise and doesn't help making progress.
You can ignore security issues in this series as they are pre-existing,
and just make this patch simple, doing one thing - removing 'evsel'
argument.

The review comments can be addressed in a separate patch series
(preferrably) or different patches in this series.

Thanks,
Namhyung

> 
> >  	if (field->flags & TEP_FIELD_IS_DYNAMIC) {
> >  		offset = *(int *)(sample->raw_data + field->offset);
> >  		offset &= 0xffff;
> >  		if (tep_field_is_relative(field->flags))
> >  			offset += field->offset + field->size;
> >  	}
> > 
> >  	return sample->raw_data + offset;
> 
> Is the dynamically computed offset validated against sample->raw_size? It
> appears the initial check covers the initial field offset, but the newly
> computed offset for dynamic fields is added directly to sample->raw_data
> without a subsequent bounds check, which might lead to out-of-bounds memory
> accesses.
> 
> [ ... ]
> 
> > @@ -3713,6 +3718,12 @@ u64 format_field__intval(struct tep_format_field *field, struct perf_sample *sam
> >  	u64 value;
> >  	void *ptr = sample->raw_data + field->offset;
> >  
> > +	if ((u32)(field->offset + field->size) > sample->raw_size) {
> > +		pr_warning("Invalid trace point field offset %d for field of length %d in sample raw data of size %u\n",
> > +			   field->offset, field->size, sample->raw_size);
> > +		return 0;
> > +	}
> > +
> 
> Can this suffer from the same integer overflow issue? If field->offset and
> field->size wrap around when added together, it looks like an out-of-bounds
> pointer could be dereferenced when evaluating the field.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3