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 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete Vulnerability Fix / TOCTOU Bypass on event->header.size
- [High] Type Confusion / Validation Bypass (TOCTOU on event->header.type)
- [High] TOCTOU / Bounds Check Bypass on Payload Array Counts
- [High] Incomplete TOCTOU Fix / Unprotected Reads in Other Event Paths
--

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

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

This commit attempts to fix a vulnerability where multiple reads of
event->header.size from MAP_SHARED memory could return different values.
It snapshots the header size into a local variable using READ_ONCE() to
ensure a consistent value is used for bounds calculations.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 6de665d3c9054..e2e821b77766d 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -2230,6 +2230,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  {
>  	struct ordered_events *oe = &session->ordered_events;
>  	const struct perf_tool *tool = session->tool;
> +	const u32 event_size = READ_ONCE(event->header.size);

[Severity: High]
This is a pre-existing issue, but since structural bounds validation is
performed earlier by callers like perf_session__process_event() calling
perf_event__too_small(), can the size be modified in MAP_SHARED memory
between that caller validation and this READ_ONCE() snapshot?

If the size is shrunk concurrently, could subsequent subtraction operations
like (event_size - sizeof(event->stat_config)) underflow, bypassing array
bounds checks?

[Severity: High]
This is a pre-existing issue, but does this function also need to protect
event->header.type with a READ_ONCE() snapshot?

If an attacker sets the type to a small struct to pass the size check in
perf_event__too_small(), and then concurrently overwrites it to a larger
struct type, will this function process it as the larger struct using a
size validated for the smaller one?

[ ... ]

> @@ -2271,7 +2272,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  		break;
>  	case PERF_RECORD_HEADER_BUILD_ID:
>  		if (!perf_event__check_nul(event->build_id.filename,
> -					   (void *)event + event->header.size,
> +					   (void *)event + event_size,

[Severity: High]
This is a pre-existing issue, but does this READ_ONCE() pattern need to be
applied to other execution paths that process memory-mapped events?

For example, perf_session__deliver_event() delegates to
machines__deliver_event(), which appears to still read event->header.size
directly from memory to calculate string boundaries before calling
perf_event__check_nul().

Could concurrent modification of the size allow the string checker's
strnlen() to scan out-of-bounds unmapped memory?

>  					   "HEADER_BUILD_ID")) {
>  			err = 0;
>  			break;

[ ... ]

> @@ -2304,14 +2305,14 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  	case PERF_RECORD_THREAD_MAP: {
>  		u64 max_nr;
>  
> -		if (event->header.size < sizeof(event->thread_map)) {
> +		if (event_size < sizeof(event->thread_map)) {
>  			pr_err("PERF_RECORD_THREAD_MAP: header.size (%u) too small\n",
> -			       event->header.size);
> +			       event_size);
>  			err = -EINVAL;
>  			break;
>  		}
>  
> -		max_nr = (event->header.size - sizeof(event->thread_map)) /
> +		max_nr = (event_size - sizeof(event->thread_map)) /
>  			 sizeof(event->thread_map.entries[0]);
>  		if (event->thread_map.nr > max_nr) {

[Severity: High]
This is a pre-existing issue, but while event_size is snapshotted, are
array count fields like event->thread_map.nr still vulnerable?

If event->thread_map.nr is validated here against max_nr, but downstream
handlers like thread_map__new_event() re-read the count directly from
MAP_SHARED memory, could an attacker overwrite it with a huge value after
this check to cause out-of-bounds heap allocations?

>  			pr_err("PERF_RECORD_THREAD_MAP: nr %" PRIu64 " exceeds max %" PRIu64 "\n",

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