Re: [PATCH 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing bounds check on `dst_size` during the first iteration causes unsigned underflow and out-of-bounds writes.
--

commit 05c1411c44b6c31eba9854745a0f8d5ce767f17a
Author: Dmitry Ilvokhin <d@ilvokhin.com>

perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push

With Zstd compression enabled, a single mmap push whose compressed output
exceeds the maximum record size emits several back-to-back compressed
records. The record__pushfn() function incorrectly rewrote only the first
record's header to describe the whole blob, which resulted in a bogus
truncated length and corrupted the perf.data file. This patch fixes the
issue by framing each record properly where it is produced, ensuring the
sizes remain valid.

[ ... ]

> diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
> index 57027e0ac7b65..1955fa2431d17 100644
> --- a/tools/perf/util/zstd.c
> +++ b/tools/perf/util/zstd.c
> @@ -30,7 +30,7 @@ int zstd_fini(struct zstd_data *data)
>  
>  ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_t dst_size,
>  				       void *src, size_t src_size, size_t max_record_size,
> -				       size_t process_header(void *record, size_t increment))
> +				       size_t process_header(void *record, size_t data_size))
>  {
>  	size_t ret, size, compressed = 0;
>  	ZSTD_inBuffer input = { src, src_size, 0 };

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a missing bounds check
on dst_size during the first iteration in this function?

Looking at zstd_compress_stream_to_records(), the bounds check explicitly
skips the first iteration because hdr_size is initialized to 0:

	while (input.pos < input.size) {
		if (hdr_size &&
		    dst_size < hdr_size + max_record_size + sizeof(u64)) {
			pr_err("failed to compress: output buffer too small\n");
			return -1;
		}
		record = dst;
		size = process_header(record, 0);
		hdr_size = size;
		compressed += size;
		dst += size;
		dst_size -= size;

If this function is called with an initially small dst_size (e.g., < 16),
process_header() will perform an out-of-bounds write of the 16-byte
perf_record_compressed2 header.

Following this, dst_size -= size will unconditionally unsigned underflow to a
massive value. This allows ZSTD_compressStream() to write up to
max_record_size bytes out-of-bounds into the dst buffer.

Could this be triggered in aio mode if perf_mmap__push() splits a wrap-around
kernel ring buffer, and the first compressed chunk leaves less than 16 bytes
in the aio->data buffer? The second call to zstd_compress() would then pass
a dst_size < 16, bypassing the bounds check on the first iteration and
triggering a buffer overflow.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1781797544.git.d@ilvokhin.com?part=1