[PATCHES 0/4] More perf.data header validation

[PATCHES 0/4] More perf.data header validation

[Arnaldo Carvalho de Melo]

Hi,

	This is picking up from what was reported in the previous
series, pre-existing lack of perf.data file validation, processing files
and buffers in header.c in a similar fashion.

	There is more to process in the trace data, but that is a
different can of worms that needs to be dealt with in a similar,
upcoming patch series,

	This is probably 7.2 material, but if feeling this can still
sneak into 7.1, feel free to do it :-)

	Now lets see what Sashiko discovers while I still don't have it
running locally right after Claude, before submitting it publicly, which
will soon happen :-)

- Arnaldo

Arnaldo Carvalho de Melo (4):
  perf header: Add section bounds checking to the fd read path
  perf header: Validate string length before allocating in do_read_string()
  perf header: Sanity check HEADER_EVENT_DESC
  perf header: Validate bitmap size before allocating in do_read_bitmap()

 tools/perf/util/header.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

-- 
2.53.0

[PATCH 1/4] perf header: Add section bounds checking to the fd read path

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

__do_read_buf() validates reads against ff->size (section size), but
__do_read_fd() had no such check, so a malformed perf.data with an
understated section size could cause reads past the end of the current
section into the next section's data.

Add the bounds check in __do_read(), the common caller of both helpers,
so it is enforced uniformly for both the fd and buf paths.

This requires two supporting changes:

- perf_file_section__process(): initialize ff->offset to 0 so it
  tracks bytes consumed from the section start (consistent with the
  buf path), rather than the absolute file position.

- process_build_id(): use lseek(ff->fd, 0, SEEK_CUR) to discover
  the current file position instead of reading ff->offset.

Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude Code:claude-opus-4-6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index f30e48eb3fc32da2..13bbf8df15f66cab 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -213,23 +213,23 @@ static int __do_read_fd(struct feat_fd *ff, void *addr, ssize_t size)
 
 	if (ret != size)
 		return ret < 0 ? (int)ret : -1;
+	ff->offset += size;
 	return 0;
 }
 
 static int __do_read_buf(struct feat_fd *ff, void *addr, ssize_t size)
 {
-	if (size > (ssize_t)ff->size - ff->offset)
-		return -1;
-
 	memcpy(addr, ff->buf + ff->offset, size);
 	ff->offset += size;
 
 	return 0;
-
 }
 
 static int __do_read(struct feat_fd *ff, void *addr, ssize_t size)
 {
+	if (size > (ssize_t)ff->size - ff->offset)
+		return -1;
+
 	if (!ff->buf)
 		return __do_read_fd(ff, addr, size);
 	return __do_read_buf(ff, addr, size);
@@ -2713,7 +2713,12 @@ static int process_tracing_data(struct feat_fd *ff __maybe_unused, void *data __
 
 static int process_build_id(struct feat_fd *ff, void *data __maybe_unused)
 {
-	if (perf_header__read_build_ids(ff->ph, ff->fd, ff->offset, ff->size))
+	off_t offset = lseek(ff->fd, 0, SEEK_CUR);
+
+	if (offset == (off_t)-1)
+		return -1;
+
+	if (perf_header__read_build_ids(ff->ph, ff->fd, offset, ff->size))
 		pr_debug("Failed to read buildids, continuing...\n");
 	return 0;
 }
@@ -4687,7 +4692,7 @@ static int perf_file_section__process(struct perf_file_section *section,
 		.fd	= fd,
 		.ph	= ph,
 		.size	= section->size,
-		.offset	= section->offset,
+		.offset	= 0,
 	};
 
 	if (lseek(fd, section->offset, SEEK_SET) == (off_t)-1) {
-- 
2.53.0

[PATCH 2/4] perf header: Validate string length before allocating in do_read_string()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

do_read_string() reads a u32 length from the file and immediately
allocates that many bytes. A crafted perf.data could claim a huge
string length, triggering a large allocation that would only be freed
moments later when __do_read() rejects the read against the section
bounds.

Check len against the remaining section size before the malloc().

Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude Code:claude-opus-4-6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 13bbf8df15f66cab..c27ed90727ea6629 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -269,6 +269,9 @@ static char *do_read_string(struct feat_fd *ff)
 	if (do_read_u32(ff, &len))
 		return NULL;
 
+	if (len > ff->size - ff->offset)
+		return NULL;
+
 	buf = malloc(len);
 	if (!buf)
 		return NULL;
-- 
2.53.0

[PATCH 3/4] perf header: Sanity check HEADER_EVENT_DESC

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

read_event_desc() reads nre (event count), sz (attr size), and nr
(IDs per event) from the file and uses them to control allocations
and loops without validating them against the section size.

A crafted perf.data could trigger large allocations or many loop
iterations before __do_read() eventually rejects the reads.

Add bounds checks:
- Reject sz == 0 or sz exceeding the section size.
- Check that nre events fit in the remaining section, using the
  minimum per-event footprint of sz + sizeof(u32).
- Check that nr IDs fit in the remaining section before allocating.

Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude Code:claude-opus-4-6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index c27ed90727ea6629..3302748bac786fdf 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2141,6 +2141,13 @@ static struct evsel *read_event_desc(struct feat_fd *ff)
 	if (do_read_u32(ff, &sz))
 		goto error;
 
+	/*
+	 * The minimum section footprint per event is sz bytes for the attr
+	 * plus a u32 for the id count, check that nre events fit.
+	 */
+	if (sz == 0 || sz > ff->size || nre > (ff->size - ff->offset) / (sz + sizeof(u32)))
+		goto error;
+
 	/* buffer to hold on file attr struct */
 	buf = malloc(sz);
 	if (!buf)
@@ -2186,6 +2193,9 @@ static struct evsel *read_event_desc(struct feat_fd *ff)
 		if (!nr)
 			continue;
 
+		if (nr > (ff->size - ff->offset) / sizeof(*id))
+			goto error;
+
 		id = calloc(nr, sizeof(*id));
 		if (!id)
 			goto error;
-- 
2.53.0

[PATCH 4/4] perf header: Validate bitmap size before allocating in do_read_bitmap()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

do_read_bitmap() reads a u64 bit count from the file and passes it
to bitmap_zalloc() without checking it against the remaining section
size. A crafted perf.data could trigger a large allocation that would
only fail later when the per-element reads exceed section bounds.

Check that the data needed (BITS_TO_U64(size) u64 values) fits in
the remaining section before allocating.

Currently used by process_mem_topology() for HEADER_MEM_TOPOLOGY.

Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude Code:claude-opus-4-6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 3302748bac786fdf..f2d0b8408cc29744 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -300,6 +300,9 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
 	if (ret)
 		return ret;
 
+	if (BITS_TO_U64(size) > (ff->size - ff->offset) / sizeof(u64))
+		return -1;
+
 	set = bitmap_zalloc(size);
 	if (!set)
 		return -ENOMEM;
-- 
2.53.0

Re: [PATCH 1/4] perf header: Add section bounds checking to the fd read path

[sashiko-bot]

Sashiko AI review found 6 potential issue(s):
- [Critical] Pre-existing: Stack buffer overflow and integer underflow in `perf_header__read_build_ids()`.
- [High] Uninitialized `ff.size` causes `perf report --header` to fail silently.
- [High] Signed bounds check bypass in `__do_read()` leads to heap buffer overflow on 32-bit platforms.
- [High] Pre-existing: Information leak due to missing null-termination in `perf_header__read_build_ids()`.
- [High] Pre-existing: Heap out-of-bounds read due to missing null-termination in `do_read_string()`.
- [Medium] Incomplete fix: `process_build_id()` bypasses the newly introduced bounds checking.
--

commit 557f37ee3d214171c6512a20a7fd8580d9ddaad8
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf header: Add section bounds checking to the fd read path
    
This commit adds section bounds checking uniformly to both the fd and buf read
paths in __do_read(), replacing the previous check that was only in
__do_read_buf(). It also updates perf_file_section__process() to initialize
the file offset to 0 and changes process_build_id() to use lseek to discover
the file position instead of relying on the offset.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 9142a8ba40195..ca0c9e80345b2 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
[ ... ]
>  static int __do_read(struct feat_fd *ff, void *addr, ssize_t size)
>  {
> +	if (size > (ssize_t)ff->size - ff->offset)
> +		return -1;
> +

Can this signed bounds check be bypassed on 32-bit platforms? If size is
derived from an extremely large unsigned length in a malicious perf.data
file, size might implicitly cast to a negative ssize_t value. The check
would evaluate to false, and the negative size could then be cast to an
enormous size_t during the memcpy() or readn() operations, leading to a
heap buffer overflow.

Also, is it possible for ff->size to be uninitialized when called from
perf_file_section__fprintf_info()?

tools/perf/util/header.c:perf_file_section__fprintf_info() {
    ...
	ff = (struct  feat_fd) {
		.fd = fd,
		.ph = ph,
	};
    ...
}

Because the struct initialization omits the .size field, it will be
initialized to 0. This seems to cause __do_read() to fail silently when
trying to print features like HEADER_EVENT_DESC, which could break
commands like perf report --header.

>  	if (!ff->buf)
>  		return __do_read_fd(ff, addr, size);
>  	return __do_read_buf(ff, addr, size);
[ ... ]
> @@ -2655,7 +2655,12 @@ static int process_tracing_data(struct feat_fd *ff, void *data)
>  
>  static int process_build_id(struct feat_fd *ff, void *data __maybe_unused)
>  {
> -	if (perf_header__read_build_ids(ff->ph, ff->fd, ff->offset, ff->size))
> +	off_t offset = lseek(ff->fd, 0, SEEK_CUR);
> +
> +	if (offset == (off_t)-1)
> +		return -1;
> +
> +	if (perf_header__read_build_ids(ff->ph, ff->fd, offset, ff->size))
>  		pr_debug("Failed to read buildids, continuing...\n");
>  	return 0;
>  }

Does process_build_id() bypass the newly introduced bounds checking?

Because process_build_id() calls perf_header__read_build_ids(), which
reads directly from the file descriptor using readn() instead of utilizing
the __do_read() helper. This seems to bypass the bounds check added above,
allowing reads past the end of the section if the section size is
understated.

While looking at perf_header__read_build_ids(), there appear to be a few
pre-existing concerns.

Could a maliciously crafted perf.data file trigger a stack buffer overflow here?

tools/perf/util/header.c:perf_header__read_build_ids() {
    ...
	char filename[PATH_MAX];
    ...
		len = bev.header.size - sizeof(bev);
		if (readn(input, filename, len) != len)
			goto out;
    ...
}

If bev.header.size is smaller than sizeof(bev), the unsigned subtraction
could wrap to a huge positive size_t. This would then become a massive
size_t passed to readn(). Alternatively, a large user-controlled u16 value
could result in a large positive length. In either case, readn() would
write significantly more data than the 4096-byte stack buffer can hold.

Additionally, does this lack of null-termination leak information?

If the file payload doesn't contain a null byte, the stack-allocated
filename buffer will contain stack garbage at its end. When passed to
subsequent C-string functions like strdup() via
__event_process_build_id(), it could result in an out-of-bounds stack
read.

There appears to be a similar pre-existing issue in do_read_string():

tools/perf/util/header.c:do_read_string() {
    ...
	buf = malloc(len);
	if (!buf)
		return NULL;

	if (!__do_read(ff, buf, len)) {
    ...
}

Since this allocates a heap buffer of size len and reads exactly len bytes
without appending a null byte, could an attacker provide a string without
the expected zero padding? If so, the buffer would lack a null terminator,
leading to a heap out-of-bounds read when manipulated.

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

Re: [PATCH 2/4] perf header: Validate string length before allocating in do_read_string()

[sashiko-bot]

Sashiko AI review found 2 potential issue(s):
- [Critical] `do_read_string()` fails to ensure null-termination, leading to out-of-bounds reads and heap buffer overflows.
- [Medium] The newly added size check is trivially bypassable because `ff->size` is unvalidated.
--

commit 1c3491f2bf9f038ae68f7007db4323e57b4d8132
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf header: Validate string length before allocating in do_read_string()

This commit attempts to prevent large memory allocations by verifying the
requested string length against the remaining section size before calling
malloc.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index ca0c9e80345b2..f8acd06ae9235 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -259,6 +259,9 @@ static char *do_read_string(struct feat_fd *ff)
>  	if (do_read_u32(ff, &len))
>  		return NULL;
>  
> +	if (len > ff->size - ff->offset)
> +		return NULL;

Is ff->size validated against the actual physical file size before this check?

If ff->size is initialized directly from the user-controlled section size in
the perf.data headers, couldn't a crafted file provide a maliciously large
section size?

This would allow the bounds check to pass, triggering the large allocation
this code was intended to prevent.

> +
>  	buf = malloc(len);
>  	if (!buf)
>  		return NULL;
>  
>  	if (!__do_read(ff, buf, len)) {
>  		/*
>  		 * strings are padded by zeroes
>  		 * thus the actual strlen of buf
>  		 * may be less than len
>  		 */
>  		return buf;
>  	}

Does this guarantee the returned string is null-terminated?

If a crafted perf.data provides a string without a null byte, or if len is 0,
wouldn't callers invoking strlen(buf) read out-of-bounds into adjacent heap
memory?

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

Re: [PATCH 3/4] perf header: Sanity check HEADER_EVENT_DESC

[sashiko-bot]

Sashiko AI review found 3 potential issue(s):
- [Critical] The patch rejects `sz == 0` but fails to enforce a minimum safe length for `sz`, leading to a critical heap buffer overflow during attribute byte-swapping. This problem wasn't introduced by this patch, but the patch's attempt to sanitize `sz` is incomplete.
- [Critical] The newly added bounds check for `nre` can be trivially bypassed, allowing a 32-bit integer overflow when allocating the `events` array that results in a massive heap out-of-bounds write. This problem wasn't introduced by this patch, but the patch completely fails to prevent it.
- [High] The newly added bounds check introduces a division-by-zero vulnerability on 32-bit architectures.
--

commit 49409abdc9adae8ec3fbd6033ac0123dfbe6b50b
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf header: Sanity check HEADER_EVENT_DESC

read_event_desc() reads nre, sz, and nr from the file and uses them to
control allocations and loops. This commit adds bounds checks to reject
sz == 0, sz exceeding section size, and checks that nre and nr fit in
the remaining section.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index f8acd06ae9235..696f19739074e 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2101,6 +2101,13 @@ static struct evsel *read_event_desc(struct feat_fd *ff)
>  	if (do_read_u32(ff, &sz))
>  		goto error;
>  
> +	/*
> +	 * The minimum section footprint per event is sz bytes for the attr
> +	 * plus a u32 for the id count, check that nre events fit.
> +	 */
> +	if (sz == 0 || sz > ff->size || nre > (ff->size - ff->offset) / (sz + sizeof(u32)))

Could this regression cause a division by zero on 32-bit architectures?

Since sz is a 32-bit unsigned integer read directly from the file, if it is
set to 0xFFFFFFFC, sz + sizeof(u32) will overflow to 0 on 32-bit systems where
sizeof() evaluates to a 32-bit size_t. This would evaluate to:
(ff->size - ff->offset) / 0

Also, does this code allow a small sz value to cause an out-of-bounds access
later during byte-swapping?

If sz is set to a small value between 1 and 7 (e.g., 4), buf = malloc(sz) will
allocate a tiny chunk. If the file requires byte-swapping, the subsequent call
to perf_event__attr_swap() will unconditionally read attr->size (bytes 4-7).
This would read out of bounds and potentially use heap garbage to byte-swap
adjacent memory.

Finally, is ff->size validated against the actual physical file size before
this check?

If ff->size is an unvalidated value from the file header, this bounds check
can be bypassed with a huge value. If a file then sets nre to 0xFFFFFFFF,
it would cause the subsequent allocation to overflow to 0:

events = calloc(nre + 1, sizeof(*events));

This would return a valid minimal chunk, but the following loop:

for (i = 0, evsel = events; i < nre; evsel++, i++) {
    evsel->core.idx = i;
    ...

would iterate 0xFFFFFFFF times, overwriting the heap array.

> +		goto error;
> +
>  	/* buffer to hold on file attr struct */
>  	buf = malloc(sz);
>  	if (!buf)

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

Re: [PATCH 4/4] perf header: Validate bitmap size before allocating in do_read_bitmap()

[sashiko-bot]

Sashiko AI review found 2 potential issue(s):
- [Critical] Integer truncation in `bitmap_zalloc()` leads to a massive heap buffer overflow. While the added bounds check ensures `BITS_TO_U64(size)` elements fit in the declared section size, `bitmap_zalloc(int nbits)` explicitly takes a 32-bit signed integer. A crafted `size` (e.g., `0x100000040`) passes the 64-bit bounds check but truncates to 64 when passed to `bitmap_zalloc()`, allocating only 8 bytes. The un-truncated 64-bit size is then used as the upper bound for the read loop, causing `do_read_u64()` to write millions of elements sequentially past the 8-byte allocation until the physical end of the file is reached.
- [High] Architecture-dependent heap buffer overflow on 32-bit systems due to mismatched allocation and read sizes. On 32-bit architectures, `bitmap_zalloc(size)` allocates memory based on `sizeof(unsigned long)`, which is 4 bytes. However, `do_read_bitmap()` casts the allocated buffer to a `u64 *` and reads 8-byte chunks using `do_read_u64()`. For small bit counts like `size = 1`, `bitmap_zalloc()` allocates exactly 4 bytes, but the read loop executes once and writes 8 bytes, causing a 4-byte out-of-bounds heap write.
--

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

perf header: Validate bitmap size before allocating in do_read_bitmap()

This commit adds a bounds check in do_read_bitmap() to prevent large
allocations that exceed the remaining section size. It validates that
the requested bit count fits within the file data before calling
bitmap_zalloc().

> 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
> @@ -290,6 +290,9 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
>  	if (ret)
>  		return ret;
>  
> +	if (BITS_TO_U64(size) > (ff->size - ff->offset) / sizeof(u64))
> +		return -1;
> +
>  	set = bitmap_zalloc(size);

Does this code truncate the allocation size?

The size variable is a u64, but bitmap_zalloc() takes a 32-bit signed integer
for its nbits parameter. If a crafted file provides a large size value like
0x100000040, the 64-bit bounds check above would pass if the section is
large enough. 

However, the value would be truncated to 64 when passed to bitmap_zalloc(),
allocating only an 8 byte buffer.

[ ... ]

>  	p = (u64 *) set;
>  
>  	for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
>  		ret = do_read_u64(ff, p + i);

Could this loop write out of bounds on 32-bit architectures?

The bitmap_zalloc() function allocates memory based on sizeof(unsigned long),
which is 4 bytes on 32-bit architectures. For a small bit count like size = 1,
the allocation would be exactly 4 bytes.

Since the code casts the buffer to a u64 pointer and reads 8-byte chunks using
do_read_u64(), the first read would write 8 bytes, overflowing the 4-byte
allocation.

Could this loop also cause a heap buffer overflow due to the truncation
mentioned earlier?

If size is truncated by bitmap_zalloc() to a very small allocation, this
loop still uses the un-truncated 64-bit size as its upper bound. It would
sequentially write millions of elements past the small allocation until it
reaches the physical end of the file.

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

Re: [PATCHES 0/4] More perf.data header validation

[James Clark]

On 14/04/2026 21:48, Arnaldo Carvalho de Melo wrote:
> Hi,
> 
> 	This is picking up from what was reported in the previous
> series, pre-existing lack of perf.data file validation, processing files
> and buffers in header.c in a similar fashion.
> 
> 	There is more to process in the trace data, but that is a
> different can of worms that needs to be dealt with in a similar,
> upcoming patch series,
> 
> 	This is probably 7.2 material, but if feeling this can still
> sneak into 7.1, feel free to do it :-)
> 
> 	Now lets see what Sashiko discovers while I still don't have it
> running locally right after Claude, before submitting it publicly, which
> will soon happen :-)

If only checkpatch.pl ran Sashiko automatically... It would be one less 
box to tick to get it right, of the many many boxes.

> 
> - Arnaldo
> 
> Arnaldo Carvalho de Melo (4):
>    perf header: Add section bounds checking to the fd read path
>    perf header: Validate string length before allocating in do_read_string()
>    perf header: Sanity check HEADER_EVENT_DESC
>    perf header: Validate bitmap size before allocating in do_read_bitmap()
> 
>   tools/perf/util/header.c | 33 +++++++++++++++++++++++++++------
>   1 file changed, 27 insertions(+), 6 deletions(-)
>