Re: [PATCH v11 31/33] perf evsel: Add bounds checking to trace point raw data accessors

sashiko-bot
2026-04-13T01:59:43Z

Sashiko AI review found 3 potential issue(s):
- [High] Missing NULL return value handling causes segmentation faults
- [Medium] Missing byte-swapping for dynamic fields breaks cross-endian trace parsing
- [Medium] Unbounded `pr_warning` causes console flood for malformed samples
--

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

perf evsel: Add bounds checking to trace point raw data accessors

This commit introduces bounds checking for tracepoint fields to prevent out
of bounds memory accesses. It adds the out_of_bounds() helper and validates
field offsets and sizes in perf_sample__rawptr() and format_field__intval().

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index bb48568b8101a..a23bd28579b55 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3700,22 +3700,59 @@ struct tep_format_field *evsel__common_field(struct evsel *evsel, const char *na
> return tp_format ? tep_find_common_field(tp_format, name) : NULL;
> }
>
> +static bool out_of_bounds(const struct tep_format_field *field, int offset, int size, u32 raw_size)
> +{
> + if (offset < 0) {
> + pr_warning("Negative trace point field offset %d in %s\n",
> + offset, field->name);
> + return true;
> + }
> + if (size < 0) {
> + pr_warning("Negative trace point field size %d in %s\n",
> + size, field->name);
> + return true;
> + }
> + if ((u32)offset + (u32)size > raw_size) {
> + pr_warning("Out of bound tracepoint field (%s) offset %d size %d in %u\n",
> + field->name, offset, size, raw_size);
> + return true;
> + }
> + return false;
> +}

These checks happen in the hot path of event processing. If a trace contains
millions of malformed samples, will these unbounded pr_warning() calls flood
the user's console?

Would it be safer to use pr_warning_once() or a rate-limited equivalent to
prevent severe performance degradation?

> void *perf_sample__rawptr(struct perf_sample *sample, const char *name)
> {
> struct tep_format_field *field = evsel__field(sample->evsel, name);
> - int offset;
> + int offset, size;
>
> if (!field)
> return NULL;
>
> offset = field->offset;
> -
> + size = field->size;
> if (field->flags & TEP_FIELD_IS_DYNAMIC) {
> - offset = *(int *)(sample->raw_data + field->offset);
> - offset &= 0xffff;
> - if (tep_field_is_relative(field->flags))
> + int dynamic_data;
> +
> + if (out_of_bounds(field, offset, 4, sample->raw_size))
> + return NULL;

When out_of_bounds() fails, this returns NULL. Callers of
perf_sample__strval(), which wraps perf_sample__rawptr(), expect a valid
string pointer if the field exists.

For example, in tools/perf/builtin-lock.c, functions like
report_lock_acquire_event() pass this return value directly to
lock_stat_findnew(), which then calls strdup().

Could returning NULL here turn a malformed sample into a segmentation fault?

> +
> + dynamic_data = *(int *)(sample->raw_data + field->offset);
> + offset = dynamic_data & 0xffff;
> + size = (dynamic_data >> 16) & 0xffff;

The sample->raw_data buffer is in the target architecture's byte order. If
the trace was recorded on a machine with a different endianness, does
dynamic_data need to be byte-swapped before extracting the offset and size?

Without swapping, could valid cross-endian traces extract scrambled values
and fail the new bounds check, potentially leading to the NULL return
mentioned above?

[ ... ]

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