Re: [PATCH v2 1/2] perf evsel: Validate branch stack before byte swapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds read in `__evsel__parse_sample` when parsing truncated `PERF_SAMPLE_CALLCHAIN` events.
- [High] Out-of-bounds read in `__set_offcpu_sample` via unvalidated `data->callchain->nr`.
--

commit 49a6711ef34085909cadd51b1b19965d5f56040c
Author: Mark Amirkan <markdamirkan@gmail.com>

perf evsel: Validate branch stack before byte swapping

When perf reads an opposite-endian branch stack, __evsel__parse_sample()
swaps each entry before checking whether all entries fit in the event. A
truncated sample can therefore make the swap loop read and write past the
event boundary.

Fixes: 63c12ae2f246 ("perf evsel: Add bitfield_swap() to handle branch_stack endian issue")
Cc: stable@vger.kernel.org
Assisted-by: Symbolic
Signed-off-by: Mark Amirkan <markdamirkan@gmail.com>

[ ... ]

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d4cb455f4a7d4..cc0bc08577540 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3639,6 +3639,8 @@ int __evsel__parse_sample(struct evsel *evsel, union perf_event *event,
>  			e = (struct branch_entry *)&data->branch_stack->hw_idx;
>  		}
>  
> +		OVERFLOW_CHECK(array, sz, max_size);
> +

[Severity: High]
This isn't a bug introduced by this patch, but while we are looking at
out-of-bounds reads and OVERFLOW_CHECK() in __evsel__parse_sample(), could
we also fix a similar issue when parsing PERF_SAMPLE_CALLCHAIN events?

tools/perf/util/evsel.c:__evsel__parse_sample() {
...
		if (evsel->core.attr.defer_callchain && callchain_nr >= 2 &&
		    data->callchain->ips[callchain_nr - 2] == PERF_CONTEXT_USER_DEFERRED) {
			data->deferred_cookie = data->callchain->ips[callchain_nr - 1];
			data->deferred_callchain = true;
		}
		OVERFLOW_CHECK(array, sz, max_size);
...
}

If a user processes a crafted perf.data file containing a
PERF_SAMPLE_CALLCHAIN event with a large callchain_nr but a truncated
payload size, doesn't this access data->callchain->ips[] before
OVERFLOW_CHECK() validates the payload length?

[Severity: High]
This is a pre-existing issue as well, but there appears to be another
out-of-bounds read in __set_offcpu_sample() via an unvalidated
data->callchain->nr:

tools/perf/util/evsel.c:__set_offcpu_sample() {
...
	OVERFLOW_CHECK(array, data->callchain->nr * sizeof(u64), max_size);
	data->ip = data->callchain->ips[1];
	array += data->callchain->nr;
...
}

If a crafted perf.data file contains an off-cpu event where
data->callchain->nr is 0 or 1, and the payload is artificially small,
doesn't this unconditionally access data->callchain->ips[1]?

Since it doesn't verify that nr >= 2, wouldn't this read past the validated
nr * sizeof(u64) boundary?

>  		if (swapped) {
>  			/*
>  			 * struct branch_flag does not have endian
> @@ -3654,7 +3656,6 @@ int __evsel__parse_sample(struct evsel *evsel, union perf_event *event,
>  				e->flags.value = evsel__bitfield_swap_branch_flags(e->flags.value);
>  		}
>  
> -		OVERFLOW_CHECK(array, sz, max_size);
>  		array = (void *)array + sz;
>  
>  		if (evsel__has_branch_counters(evsel)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-sympwn-linux-002-final-v2-v2-0-0aee1fca1f95@gmail.com?part=1