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 newly introduced `session->one_mmap_size` calculation can underflow, defeating the bounds check and leaving the out-of-bounds memory read vulnerability exploitable.

Pre-existing issues:
- [High] The main event parsing loop in fetch_mmaped_event() remains vulnerable to the exact same SIGBUS crash the patch attempted to fix.
- [High] The bounds check on `event->header.size` can be bypassed via concurrent file modification (TOCTOU), leading to potential out-of-bounds reads or infinite loops.
--

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

perf session: Bounds-check one_mmap event pointer in peek_event

This patch adds one_mmap_size to the session struct and validates that
both the header and the full event fit within the mapped region before
dereferencing. This prevents out-of-bounds memory reads on corrupted
perf.data files.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> --- 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]
This is a pre-existing issue, but can the bounds check on event->header.size
be bypassed via concurrent file modification?

Since the file is mapped using a shared mapping, this function returns a
direct pointer into shared memory. Any subsequent reads of event->header.size
by the caller perform a second fetch from that same memory.

If an attacker modifies the perf.data file concurrently after this bounds
check passes, could they change the size to a very large value or to 0,
leading to out-of-bounds reads or infinite loops in downstream callbacks?

>  	} else {
>  		if (perf_data__is_pipe(session->data))

[ ... ]

> @@ -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]
Can session->one_mmap_size underflow here?

If a corrupted perf.data file header provides extremely large values for
data_offset and data_size, their sum could wrap around to a small value,
causing rd->mmap_size to be smaller than page_offset.

If this subtraction underflows, session->one_mmap_size will become a
massive value, which would bypass the new bounds checks in
perf_session__peek_event() and still allow out-of-bounds memory access.

[Severity: High]
This isn't a bug introduced by this patch, but does the main event parsing
loop in fetch_mmaped_event() remain vulnerable to this same crash?

While this patch adjusts session->one_mmap_size by page_offset to avoid
unbacked pages, it appears rd->mmap_size itself is left unadjusted.

If fetch_mmaped_event() still bounds-checks events against the unadjusted
rd->mmap_size, could a crafted file supply an event size that passes the
check but lands in the unbacked pages at the tail of the mapping, causing
a crash when rd->process() attempts to read it?

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