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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing bounds checking on the 'nr' array lengths during event swapping in namespaces, thread_map, and cpu_map events allows malicious files to trigger massive out-of-bounds memory writes.
- [Critical] Missing size validation for `attr.size` in `perf_event__hdr_attr_swap` leads to an integer underflow when calculating the remaining event size, resulting in a massive out-of-bounds byte-swap.
- [High] In `perf_session__peek_events()`, the `err` variable is not cleared after recovering from a malformed event, causing a spurious failure if the skipped event is the last one in the processing range.
--

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

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

Change swap callbacks from void to int return so handlers can propagate errors.
Adds null-termination enforcement on the common event delivery path.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c

[ ... ]

> @@ -458,18 +506,25 @@ static void perf_event__namespaces_swap(union perf_event *event,
>  
>  	if (sample_id_all)
>  		swap_sample_id_all(event, &event->namespaces.link_info[i]);
> +	return 0;
>  }

Is there a missing bounds check on the nr array lengths during event
swapping here and in thread_map/cpu_map events? It seems a large nr value
could force the loop to write out-of-bounds and corrupt adjacent memory.
I see this is addressed in the subsequent patch "perf session: Validate nr
fields against event size on both swap and common paths".

[ ... ]

> @@ -560,30 +625,34 @@ static void perf_event__hdr_attr_swap(union perf_event *event,
>  	size = event->header.size;
>  	size -= perf_record_header_attr_id(event) - (void *)event;
>  	mem_bswap_64(perf_record_header_attr_id(event), size);
> +	return 0;
>  }

Could a maliciously large attr.size cause the size calculation to underflow,
resulting in a massive out-of-bounds byte-swap? I note this is also fixed
later in the series by the patch "perf session: Validate HEADER_ATTR
alignment and attr.size before swapping".

[ ... ]

> @@ -1968,15 +2169,34 @@ int perf_session__peek_events(struct perf_session *session, u64 offset,
>  {
>  	u64 max_offset = offset + size;
>  	char buf[PERF_SAMPLE_MAX_SIZE];
> -	union perf_event *event;
> +	/*
> +	 * Initialized to NULL so the first-iteration error path
> +	 * doesn't dereference stack garbage.  On subsequent failures
> +	 * event may point into buf from a prior read — peek_event
> +	 * sets *event_ptr on min_sz and swap failures so the header
> +	 * is from the current (failed) event, not a stale one.
> +	 */
> +	union perf_event *event = NULL;
>  	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;
> +			else
> +				return err;
> +			continue;
> +		}

If we successfully recover from a malformed event by advancing the offset
and calling continue, but it happens to be the last event in the requested
range (offset >= max_offset), does the loop exit and return the stale
failure code from peek_event instead of 0?

It seems the err variable retains the -1 failure code from the last
iteration which might cause a spurious failure in the caller.

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