Re: [PATCH 02/29] perf session: Bounds-check one_mmap event pointer in peek_event

[sashiko-bot@kernel.org]

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

New issues:
- [High] The calculation of `session->one_mmap_size` is vulnerable to integer underflow and lacks physical file size validation, allowing attackers to bypass the new bounds checks and trigger a crash.
- [High] The newly added bounds check is vulnerable to a TOCTOU race condition because it reads `event->header.size` directly from a `MAP_SHARED` memory mapping without caching the value.

Pre-existing issues:
- [High] The patch fixes an out-of-bounds pointer calculation in `perf_session__peek_event()` when `one_mmap` is used, but misses an identical unvalidated pointer calculation in `auxtrace_queues__add_buffer()`.
--

commit d7ee9e1cc395f66a07488630068264ff3c40712f
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf session: Bounds-check one_mmap event pointer in peek_event

This commit adds bounds checking to perf_session__peek_event() when one_mmap
is active. It adds one_mmap_size to the session struct and ensures both the
header and the full event fit within the mapped region to prevent
out-of-bounds reads.

[Severity: High]
This is a pre-existing issue, but looking at tools/perf/util/auxtrace.c
in auxtrace_queues__add_buffer(), there appears to be an identical unvalidated
pointer calculation:

buffer->data = buffer->data_offset - session->one_mmap_offset +
               session->one_mmap_addr;

Since buffer->data_offset originates from the untrusted perf.data file,
could an attacker provide a manipulated AUX trace event offset to yield an
out-of-bounds pointer, leading to an out-of-bounds memory read?

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 0523fd243e02c..c4cd8ad6d810a 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1887,12 +1887,27 @@ int perf_session__peek_event(struct perf_session *session, off_t file_offset,
>  	*event_ptr = NULL;
>  
>  	if (session->one_mmap && !session->header.needs_swap) {
> -		event = file_offset - session->one_mmap_offset +
> -			session->one_mmap_addr;
> +		u64 offset_in_mmap;
> +
> +		/* Validate offset with integer arithmetic to avoid pointer UB */
> +		if ((u64)file_offset < session->one_mmap_offset)
> +			return -1;
> +
> +		offset_in_mmap = (u64)file_offset - session->one_mmap_offset;
> +
> +		/* Use subtraction to avoid addition overflow */
> +		if (offset_in_mmap >= session->one_mmap_size ||
> +		    session->one_mmap_size - offset_in_mmap < sizeof(struct perf_event_header))
> +			return -1;
> +
> +		event = session->one_mmap_addr + offset_in_mmap;
>  
> -		/* Every event must at least contain its own header */
>  		if (event->header.size < sizeof(struct perf_event_header))
>  			return -1;
> +
> +		/* Ensure full event is within the mmap region */
> +		if (session->one_mmap_size - offset_in_mmap < event->header.size)
> +			return -1;

[Severity: High]
Does this introduce a time-of-check to time-of-use race condition?

Since the event structure is located in a MAP_SHARED memory mapping, the value
of event->header.size is read directly from memory and could be modified by a
concurrent attacker after this bounds check passes.

If the caller later re-reads the tampered event->header.size from memory,
could it allow the offset to advance out of bounds?

[ ... ]

> @@ -2560,6 +2575,14 @@ reader__mmap(struct reader *rd, struct perf_session *session)
>  	if (session->one_mmap) {
>  		session->one_mmap_addr = buf;
>  		session->one_mmap_offset = rd->file_offset;
> +		/*
> +		 * mmap_size was set to the full file extent (data_offset +
> +		 * data_size) but file_offset was shifted forward by
> +		 * page_offset for page alignment.  Reduce by page_offset
> +		 * so the bounds check reflects the file-backed portion
> +		 * of the mapping — pages beyond the file cause SIGBUS.
> +		 */
> +		session->one_mmap_size = rd->mmap_size - page_offset;

[Severity: High]
Is this calculation vulnerable to an integer underflow?

If rd->mmap_size is derived from an unvalidated data_size + data_offset, and
this addition overflows to a value smaller than page_offset, the subtraction
could result in a huge one_mmap_size that bypasses the new bounds checks.

Additionally, since rd->mmap_size relies on the untrusted data_size from the
file header rather than the physical file size, could an attacker specify a
32MB data_size for a 4KB file? The bounds check might pass, but accessing
unbacked pages would trigger a SIGBUS.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525010550.1100375-1-acme@kernel.org?part=2