Re: [PATCH 15/29] perf session: Add byte-swap and bounds check for PERF_RECORD_BPF_METADATA events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Flawed TOCTOU mitigations in `perf_session__process_user_event` introduce a new double-fetch vulnerability and fail to protect downstream consumers.
--

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

perf session: Add byte-swap and bounds check for PERF_RECORD_BPF_METADATA events

This commit adds byte-swapping and bounds validation for PERF_RECORD_BPF_METADATA
events to prevent out-of-bounds reads when processing cross-endian perf.data files.
It handles writable cross-endian mappings by clamping values in place and validates
read-only native-endian mappings to skip malicious records.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 092fccbea8f80..95eb793026de6 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
[ ... ]
> @@ -2203,9 +2246,53 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  	case PERF_RECORD_FINISHED_INIT:
>  		err = tool->finished_init(tool, session, event);
>  		break;
> -	case PERF_RECORD_BPF_METADATA:
> +	case PERF_RECORD_BPF_METADATA: {
> +		u64 nr_entries, max_entries;
> +		u32 hdr_size = READ_ONCE(event->header.size);

[Severity: Medium]
Could the snapshotting of event->header.size introduce a double-fetch issue?

Since native-endian files are mapped as MAP_SHARED, it seems a concurrent
modification by a malicious file writer could inflate event->header.size in
the time window between the core bounds check and this READ_ONCE().

If hdr_size is inflated, the calculated max_entries would be larger than the
actual mmap region, which might cause the strnlen loop below to read out
of bounds.

> +
> +		if (hdr_size < sizeof(event->bpf_metadata)) {
> +			pr_warning("WARNING: PERF_RECORD_BPF_METADATA: header.size (%u) too small, skipping\n",
> +				   hdr_size);
> +			err = 0;
> +			break;
> +		}
[ ... ]
> +		/* Snapshot — event is mmap'd and could change between reads */
> +		nr_entries = READ_ONCE(event->bpf_metadata.nr_entries);
> +		max_entries = (hdr_size - sizeof(event->bpf_metadata)) /
> +			      sizeof(event->bpf_metadata.entries[0]);
> +		if (nr_entries > max_entries) {
> +			pr_warning("WARNING: PERF_RECORD_BPF_METADATA: nr_entries %" PRIu64 " exceeds max %" PRIu64 ", skipping\n",
> +				   nr_entries, max_entries);
> +			err = 0;
> +			break;
> +		}
> +
> +		for (u64 i = 0; i < nr_entries; i++) {
> +			if (strnlen(event->bpf_metadata.entries[i].key,
> +				    BPF_METADATA_KEY_LEN) == BPF_METADATA_KEY_LEN ||
> +			    strnlen(event->bpf_metadata.entries[i].value,
> +				    BPF_METADATA_VALUE_LEN) == BPF_METADATA_VALUE_LEN) {
> +				pr_warning("WARNING: PERF_RECORD_BPF_METADATA: entry %" PRIu64 " key/value not null-terminated, skipping\n", i);
> +				err = 0;
> +				goto out;
> +			}
> +		}
> +
>  		err = tool->bpf_metadata(tool, session, event);

[Severity: Medium]
Does passing the raw event pointer to downstream consumers bypass the
validation performed above?

Because the event is volatile, functions downstream might re-read
event->bpf_metadata.nr_entries directly from the memory mapping instead
of using the validated nr_entries snapshot.

Could this lead to an out-of-bounds read if the value changes after the check
but before the consumer uses it?

>  		break;
> +	}
>  	case PERF_RECORD_SCHEDSTAT_CPU:

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