Re: [PATCH 07/27] perf session: Add validated swap infrastructure with null-termination checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf_session__peek_events() incorrectly advances the file offset by an unaligned or garbage event size on critical parsing errors.
- [Medium] TOCTOU race in native-endian event processing leads to integer underflow and massive out-of-bounds read in the newly added perf_event__check_nul().
--

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

perf session: Add validated swap infrastructure with null-termination checks

This commit changes the swap handlers to return an int instead of void, allowing
them to propagate errors. It introduces perf_event__check_nul() to ensure string
fields are null-terminated before processing. Several bounds checks and size
validations are also added to various swap handlers to improve safety.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 415693a3450a7..8c5b3323d0ee0 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1481,6 +1603,25 @@ static int session__flush_deferred_samples(struct perf_session *session,
>  	return ret;
>  }
>  
> +/*
> + * Return true if the string field is properly null-terminated
> + * within the event boundary.  Native-endian files are mapped
> + * read-only (MAP_SHARED + PROT_READ) so we cannot write a
> + * null byte in place; skip the event instead.
> + */
> +static bool perf_event__check_nul(const char *str, const void *end, const char *event_name)
> +{
> +	size_t max_len = (const char *)end - str;
> +

[Severity: Medium]
Since native-endian files are mapped using MAP_SHARED and a concurrent writer
could shrink event->header.size after the initial validation, could end
become less than str?

If that happens, could this regression cause max_len to underflow to a
massive value, leading strnlen() to perform a huge out-of-bounds read?
Would it be safer to check if ((const void *)str >= end) here to prevent this?

> +	if (max_len == 0 || strnlen(str, max_len) == max_len) {
> +		pr_warning("WARNING: PERF_RECORD_%s: string not null-terminated, skipping event\n",
> +			   event_name);
> +		return false;
> +	}
> +
> +	return true;
> +}

[ ... ]

> @@ -1976,11 +2176,25 @@ int perf_session__peek_events(struct perf_session *session, u64 offset,
>  	int err;
>  
>  	do {
> +		event = NULL;
>  		err = perf_session__peek_event(session, offset, buf,
>  					       PERF_SAMPLE_MAX_SIZE, &event,
>  					       NULL);
> -		if (err)
> -			return err;
> +		if (err) {
> +			/*
> +			 * peek_event sets event_ptr when it read enough
> +			 * to know the event size (min_sz and swap failures).
> +			 * If event is NULL or size is 0, we can't advance
> +			 * and must abort.  Otherwise skip past this entry.
> +			 */
> +			if (event && event->header.size) {
> +				offset += event->header.size;

[Severity: Medium]
If perf_session__peek_event() fails and returns -1 because the size is
unaligned (e.g., event->header.size % sizeof(u64) != 0), wouldn't this
regression add the unaligned size to the offset?

If the offset becomes unaligned, doesn't it guarantee that all subsequent
reads will interpret the middle of events as headers, leading to a cascade
of parsing corruption?

> +				err = 0;
> +			} else {
> +				return err;
> +			}
> +			continue;
> +		}

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