Re: [PATCH v8 23/29] perf evsel: Don't pass evsel with sample

sashiko-bot
2026-04-11T07:25:40Z

Sashiko AI review found 1 potential issue(s):
- [Medium] Double-fetch of `event->header.size` from shared memory can lead to an integer underflow and arbitrary out-of-bounds read.
--

commit feaf06e92fc012fceb6bf075c90c4ab1eff00681
Author: Ian Rogers <irogers@google.com>

perf evsel: Don't pass evsel with sample

Arrange for the sample to contain the evsel and so it is unnecessary
to pass the evsel as well. This is done for uniformity, although
parsing of the sample is arguably a special case. Add missing bound
check in perf_evsel__parse_id_sample.

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
[ ... ]
> -static int perf_evsel__parse_id_sample(const struct evsel *evsel,
> - const union perf_event *event,
> +static int perf_evsel__parse_id_sample(const union perf_event *event,
> struct perf_sample *sample)
> {
> + const struct evsel *evsel = sample->evsel;
> u64 type = evsel->core.attr.sample_type;
> - const __u64 *array = event->sample.array;
> + const __u64 *array, *array_begin = event->sample.array;
> bool swapped = evsel->needs_swap;
> union u64_swap u;
[ ... ]
> + if (event->header.size < sizeof(event->header) + sizeof(u64))
> + return -EFAULT;
>
> + array = array_begin + ((event->header.size - sizeof(event->header)) / sizeof(u64)) - 1;

Is there a risk of a double-fetch here? Since the event can point directly
into memory mapped from perf.data, could a concurrent modification to the
file alter event->header.size between the bounds check and this calculation?

If event->header.size is modified to 0 after the check passes, would the
expression (0 - sizeof(event->header)) undergo an unsigned underflow since
sizeof returns a size_t? This could result in a massive positive offset
being added to array_begin.

> if (type & PERF_SAMPLE_IDENTIFIER) {
> + if (array < array_begin)
> + return -EFAULT;

Could the compiler optimize this fallback check away? Because the offset
added to array_begin is evaluated as an unsigned size_t, compilers like
GCC (at -O2) often apply strict pointer overflow rules and assume the
resulting pointer is always greater than or equal to the base pointer.

If the check is evaluated as dead code, could this bypass the bounds
checking completely and lead to an out-of-bounds read on the array pointer?

> +
> sample->id = *array;
> array--;
> }

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260411065718.372240-1-irogers@google.com?part=23