Re: [PATCH 24/29] perf tools: Harden compressed event processing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Double-fetch of event header size from a memory-mapped file leads to integer underflow and out-of-bounds read.
--

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

perf tools: Harden compressed event processing

This commit adds several hardening checks to the compressed event
decompression pipeline. It validates comp_mmap_len, checks for underflow
in decomp_last_rem, and ensures event header sizes are large enough to
contain the fixed struct fields.

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

[ ... ]

> @@ -47,14 +55,37 @@ static int perf_session__process_compressed_event(const struct perf_tool *tool _
>  		decomp->size = decomp_last_rem;
>  	}
>  
> +	/*
> +	 * Events are read directly from the mmap'd file; fields could
> +	 * theoretically change via a FUSE-backed file, but that applies
> +	 * to the entire event processing pipeline, not just here.
> +	 */
>  	if (event->header.type == PERF_RECORD_COMPRESSED) {
> +		if (event->header.size < sizeof(struct perf_record_compressed))
> +			goto err_decomp;
>  		src = (void *)event + sizeof(struct perf_record_compressed);
>  		src_size = event->pack.header.size - sizeof(struct perf_record_compressed);

[Severity: Medium]
Since event points to a memory-mapped file, can the size field be concurrently
modified between the bounds check and the subtraction here?

If the size is changed to a smaller value after passing the check, could this
double-fetch lead to an integer underflow, producing a massive src_size that
gets passed to zstd_decompress_stream() and causes an out-of-bounds read?

While the comment mentions FUSE modifications apply to the entire pipeline,
later commits reportedly fix this issue globally by snapshotting the size in
process_user_event(). It seems these newly introduced double-fetches in this
function miss that mitigation.

>  	} else if (event->header.type == PERF_RECORD_COMPRESSED2) {
> +		/*
> +		 * prefetch_event() only guarantees that the 8-byte
> +		 * event header fits; validate that header.size covers
> +		 * the data_size field before accessing it, otherwise a
> +		 * crafted event reads data_size from adjacent memory.
> +		 */
> +		if (event->header.size < sizeof(struct perf_record_compressed2))
> +			goto err_decomp;
>  		src = (void *)event + sizeof(struct perf_record_compressed2);
>  		src_size = event->pack2.data_size;
> +		/*
> +		 * data_size is independent of header.size (which
> +		 * includes padding); verify it doesn't exceed the
> +		 * actual payload to prevent out-of-bounds reads in
> +		 * zstd_decompress_stream().
> +		 */
> +		if (src_size > event->header.size - sizeof(struct perf_record_compressed2))

[Severity: Medium]
Does this also suffer from the same double-fetch problem?

If event->header.size is modified by a FUSE-backed file between the initial
check and this second evaluation, could the subtraction underflow and
erroneously bypass the out-of-bounds check for data_size?

> +			goto err_decomp;
>  	} else {
> -		return -1;
> +		goto err_decomp;
>  	}

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