Re: [PATCH 14/28] 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:
- [High] Time-of-Check to Time-of-Use (TOCTOU) and double-fetch vulnerabilities in PERF_RECORD_BPF_METADATA parsing allow bounds check bypasses leading to out-of-bounds memory writes.
--

commit a0df5fd6c7ae94e18c39e39cde4b14a49db594f3
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 checking for PERF_RECORD_BPF_METADATA
events when processing cross-endian perf.data files. It validates the header
size, number of entries, and string NUL-termination to prevent out-of-bounds
reads.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index a2dba77c6a2b9..876e20c4ba8a7 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
[ ... ]
> @@ -942,6 +942,45 @@ 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);

Could this sequence lead to a Time-of-Check to Time-of-Use (TOCTOU)
vulnerability?

The perf.data file is mapped into memory using MAP_PRIVATE for cross-endian
files. MAP_PRIVATE pages share the underlying file's page cache until a
Copy-On-Write (COW) fault occurs.

If the file is modified concurrently after the size check but before the
write to event->bpf_metadata.nr_entries, the COW would freeze the new,
potentially malicious event->header.size into the process's private page.

> +
> +	/*
> +	 * Ensure NUL-termination on the cross-endian path where the
> +	 * mapping is writable (MAP_PRIVATE + PROT_WRITE).  Fixing
> +	 * the string in place is preferred over rejecting because it
> +	 * preserves the event for downstream processing — only the
> +	 * last byte is lost.
> +	 *
> +	 * The native-endian path (MAP_SHARED + PROT_READ) cannot
> +	 * write, so it validates and skips unterminated events in
> +	 * perf_session__process_user_event() instead.  The two
> +	 * strategies produce different outcomes for the same
> +	 * malformed input (fix vs skip), which is inherent in the
> +	 * writable-vs-read-only mapping model.
> +	 */
> +	event->bpf_metadata.prog_name[BPF_PROG_NAME_LEN - 1] = '\0';
> +
> +	nr = event->bpf_metadata.nr_entries;
> +	max_nr = (event->header.size - sizeof(event->bpf_metadata)) /
> +		 sizeof(event->bpf_metadata.entries[0]);

Since event->header.size is read again from the mapped memory, could a
concurrently modified, smaller size cause this calculation to underflow,
producing a massive max_nr?

> +	if (nr > max_nr)
> +		nr = max_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';
> +	}

Would this massive max_nr then bypass the bounds check and allow the loop
to write NUL bytes out-of-bounds across the process's memory?

> +	return 0;
> +}
[ ... ]
> @@ -2181,9 +2221,50 @@ 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 max_entries;
> +
> +		if (event->header.size < sizeof(event->bpf_metadata)) {
> +			pr_warning("WARNING: PERF_RECORD_BPF_METADATA: header.size (%u) too small, skipping\n",
> +				   event->header.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;
> +		}
> +
> +		max_entries = (event->header.size - sizeof(event->bpf_metadata)) /
> +			      sizeof(event->bpf_metadata.entries[0]);

Similarly on the native-endian path (using MAP_SHARED), could reading
event->header.size multiple times allow an attacker to underflow max_entries
by modifying the file concurrently?

> +		if (event->bpf_metadata.nr_entries > max_entries) {
> +			pr_warning("WARNING: PERF_RECORD_BPF_METADATA: nr_entries %" PRIu64 " exceeds max %" PRIu64 ", skipping\n",
> +				   (u64)event->bpf_metadata.nr_entries, max_entries);
> +			err = 0;
> +			break;
> +		}
> +
> +		for (u64 i = 0; i < event->bpf_metadata.nr_entries; i++) {

Could event->bpf_metadata.nr_entries also change between the check and this
loop, leading to out-of-bounds reads?

To prevent this, should event->header.size and event->bpf_metadata.nr_entries
be read into local variables once and validated before further use?

> +			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;
> +			}
> +		}

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