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

Namhyung Kim
2026-04-09T01:51:49Z

On Wed, Apr 08, 2026 at 08:20:22AM +0000, sashiko-bot@kernel.org 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 <irogers@google.com>
>
> 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/20260408072601.210837-1-irogers@google.com?part=3