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

[sashiko-bot@kernel.org]

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?

>  	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