Re: [PATCH 28/29] perf session: Snapshot event->header.size in process_user_event()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The patch attempts to fix a Time-of-Check to Time-of-Use (TOCTOU) race by snapshotting `event->header.size` into a local variable using `READ_ONCE()`, but takes the snapshot after the size has already been validated by the caller. This allows an attacker to concurrently shrink the size in shared memory, causing integer underflows in length calculations and bypassing bounds checks.
- [High] The patch leaves downstream event callbacks vulnerable to Time-of-Check to Time-of-Use (TOCTOU) attacks because it passes the original `event` pointer, allowing downstream functions to re-read loop bounds and data directly from untrusted shared memory after upstream validation.
--

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

perf session: Snapshot event->header.size in process_user_event()

This commit snapshots event->header.size into a local variable using
READ_ONCE() at function entry to mitigate Time-of-Check to Time-of-Use
(TOCTOU) issues. It then replaces various reads of event->header.size
throughout the function with the local snapshot.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 69eb404f805d8..d62cb21920e04 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
[ ... ]
> @@ -2378,8 +2379,8 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  		break;
>  	}
>  	case PERF_RECORD_STAT_CONFIG: {
> -		/* Cannot underflow: perf_event__min_size[] guarantees header.size >= sizeof */
> -		u64 max_nr = (event->header.size - sizeof(event->stat_config)) /
> +		/* Cannot underflow: perf_event__min_size[] guarantees event_size >= sizeof */
> +		u64 max_nr = (event_size - sizeof(event->stat_config)) /

[Severity: Critical]
This is a pre-existing issue, but is it safe to snapshot the size into
event_size at the start of this function after the caller has already
validated it?

If the event is read from shared memory, an attacker could concurrently
modify the size between the caller's validation and the READ_ONCE() at
function entry, causing it to shrink.

Since this new unvalidated event_size is used in later calculations, couldn't
event_size - sizeof(event->stat_config) underflow and bypass the bounds
check?

>  			     sizeof(event->stat_config.data[0]);
>  
[ ... ]
> @@ -2410,7 +2411,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  		 */
>  		memset(&session->time_conv, 0, sizeof(session->time_conv));
>  		memcpy(&session->time_conv, &event->time_conv,
> -		       min((size_t)event->header.size, sizeof(session->time_conv)));
> +		       min((size_t)event_size, sizeof(session->time_conv)));
>  		err = tool->time_conv(tool, session, event);

[Severity: High]
This isn't a bug introduced by this patch, but does passing the original
event pointer to downstream callbacks leave them vulnerable to Time-of-Check
to Time-of-Use (TOCTOU) attacks?

Since the event points directly to untrusted shared memory, couldn't
downstream functions like tool->time_conv() re-read loop bounds or data
directly from the event, bypassing the upstream bounds validation?

>  		break;

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