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

[sashiko-bot@kernel.org]

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