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

New issues:
- [High] A double-fetch of `event->header.size` in `perf_event__bpf_metadata_swap()` allows an attacker to underflow `max_nr`, bypassing clamping logic and causing massive out-of-bounds writes.

Pre-existing issues:
- [High] The bounds checking and string validation introduced for PERF_RECORD_BPF_METADATA are bypassed by downstream consumers due to a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability.
- [High] In `perf_session__process_user_event()`, fetching `event->header.size` via `READ_ONCE()` can retrieve a size larger than what the caller validated, leading to out-of-bounds reads past the memory mapping.
--

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

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

PERF_RECORD_BPF_METADATA has no entry in perf_event__swap_ops[], so its
nr_entries field is never byte-swapped when reading a cross-endian
perf.data file. Downstream processing in perf_event__fprintf_bpf_metadata()
loops over nr_entries, so a foreign-endian value causes out-of-bounds reads.

> 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
> @@ -961,6 +961,48 @@ static int perf_event__time_conv_swap(union perf_event *event,
>  	return 0;
>  }
>  
> +static int perf_event__bpf_metadata_swap(union perf_event *event,
> +					 bool sample_id_all __maybe_unused)
> +{
> +	u64 i, nr, max_nr;
> +
> +	/* Fixed header must fit before accessing nr_entries or prog_name */
> +	if (event->header.size < sizeof(event->bpf_metadata))
> +		return -1;
> +
> +	event->bpf_metadata.nr_entries = bswap_64(event->bpf_metadata.nr_entries);

[ ... ]

> +	nr = event->bpf_metadata.nr_entries;
> +	max_nr = (event->header.size - sizeof(event->bpf_metadata)) /
> +		 sizeof(event->bpf_metadata.entries[0]);

[Severity: High]
Can this double-fetch of event->header.size allow an attacker to bypass the
clamping logic?

Because event points to a memory-mapped file, the file can be modified
concurrently. If event->header.size is modified to a value smaller than
sizeof(event->bpf_metadata) after the initial size check above, but before this
calculation, wouldn't the subtraction underflow and yield a massive unsigned
value for max_nr?

This would bypass the nr > max_nr check below, allowing the loop to write NUL
bytes out-of-bounds. The commit message mentions fixing a double-fetch via
READ_ONCE(), but it appears it was only applied to process_user_event(),
leaving the swap path vulnerable.

> +	if (nr > max_nr) {
> +		/* Persist clamped value so the native path processes entries, not skips */
> +		nr = max_nr;
> +		event->bpf_metadata.nr_entries = nr;
> +	}
> +
> +	for (i = 0; i < nr; i++) {
> +		event->bpf_metadata.entries[i].key[BPF_METADATA_KEY_LEN - 1] = '\0';
> +		event->bpf_metadata.entries[i].value[BPF_METADATA_VALUE_LEN - 1] = '\0';
> +	}
> +	return 0;
> +}

[ ... ]

> @@ -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: High]
This is a pre-existing issue, but by re-fetching event->header.size via
READ_ONCE() here, could we retrieve a size larger than what the caller
originally validated?

If a malicious writer enlarges the size between the caller's check against the
memory mapping bounds and this READ_ONCE(), max_entries would be calculated
based on the enlarged size. This would cause the strnlen validations below
to read memory past the end of the memory-mapped file.

> +
> +		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;
> +		}
> +
> +		/*
> +		 * Native-endian files are mmap'd read-only — validate
> +		 * NUL-termination instead of writing.
> +		 */
> +		if (strnlen(event->bpf_metadata.prog_name,
> +			    BPF_PROG_NAME_LEN) == BPF_PROG_NAME_LEN) {
> +			pr_warning("WARNING: PERF_RECORD_BPF_METADATA: prog_name not null-terminated, skipping\n");
> +			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: High]
This is a pre-existing issue, but is the validation mitigation bypassed here?

After verifying the string lengths and entry bounds locally, the raw
memory-mapped event pointer is passed directly to the downstream callback.
If downstream handlers read event->bpf_metadata.nr_entries and the payload
strings from the shared memory again, an attacker could concurrently modify
the mmap'd file after these checks, leading to out-of-bounds reads.

>  		break;
> +	}
>  	case PERF_RECORD_SCHEDSTAT_CPU:
>  		err = tool->schedstat_cpu(tool, session, event);
>  		break;

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