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:

Pre-existing issues:
- [High] The patch introduces a misleading comment claiming that `perf_session__process_compressed_event()` checks `decomp_len + sizeof(struct decomp)` against `SIZE_MAX` to prevent 32-bit overflows, but the check is missing from the implementation.
--

commit 8986e6b9eaf27b5fb5e29d70385ea196a5ce7f23
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 aims to prevent underflows, validate event
sizes, and verify the presence of the HEADER_COMPRESSED feature.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -3861,6 +3861,23 @@ static int process_compressed(struct feat_fd *ff,
>  	if (do_read_u32(ff, &(env->comp_mmap_len)))
>  		return -1;
>  
> +	/*
> +	 * FIXME: perf.data should record the recording system's page
> +	 * size — it affects mmap buffer alignment, sample addresses,
> +	 * and data_page_size/code_page_size interpretation.  Without
> +	 * it we assume 4K (the smallest Linux page size) as a safe
> +	 * minimum alignment for comp_mmap_len validation.
> +	 *
> +	 * No upper-bound cap: perf_session__process_compressed_event()
> +	 * checks decomp_len + sizeof(struct decomp) against SIZE_MAX
> +	 * before allocating, which handles 32-bit safety.
> +	 */

[Severity: High]
This is a pre-existing issue, but the comment added here claims that
perf_session__process_compressed_event() checks decomp_len +
sizeof(struct decomp) against SIZE_MAX. Is this check actually present in
the implementation?

Looking at perf_session__process_compressed_event(), decomp_len is a size_t
variable. When adding the 64-bit decomp_last_rem to decomp_len:

    decomp_last_rem = decomp_last->size - decomp_last->head;
    decomp_len += decomp_last_rem;

Could this silently wrap around on 32-bit architectures?

If decomp_len wraps around, the subsequent mmap_len calculation would wrap
to a small value, causing an undersized buffer allocation. Would the
downstream memcpy() and zstd_decompress_stream() functions then use the
pre-overflow sizes, resulting in massive out-of-bounds writes?

I notice this issue is fixed later in the series by commit "perf session:
Check for decompression buffer size overflow", but wanted to ask about the
comment claiming the check exists here.

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