[PATCH v4 0/5] perf trace-event: Fix overflow, loop and cleanup bugs

[PATCH v4 0/5] perf trace-event: Fix overflow, loop and cleanup bugs

[Tanushree Shah]

This series fixes five security issues in trace-event-read.c and
trace-event.c:
1. Stack buffer overflow in read_string() when a string exceeds
   BUFSIZ, due to a missing bounds check.
2. Integer truncation when passing 64-bit sizes into do_read() and
   skip(), which use 'int' parameters, causing uninitialized memory
   to be dumped and parsers to read out of bounds.
3. Double free / use-after-free in trace_event__cleanup(): it frees
   t->pevent but never clears the pointer, so calling it twice on
   the same trace_event touches already-freed memory. Also fixes a
   related leak in trace_event__init(), which overwrites
   t->pevent/t->plugin_list without releasing any existing handle
   if called more than once on the same struct.
4. Heap buffer overflow in read_ftrace_printk() and
   read_saved_cmdline(): size + 1 can overflow to 0 in malloc(),
   allocating a tiny buffer while a huge read is still attempted
   into it.
5. Infinite loop in skip(): it does not check do_read()'s return
   value, so a crafted size can spin the loop indefinitely.

Patches 4 and 5 are latent bugs that patch 2's type fix exposes by
removing accidental truncation that had been masking them.

Tanushree Shah (5):
  perf trace-event: Fix buffer overflow in read_string()
  perf trace-event: Fix integer truncation in do_read() and skip()
  perf trace-event: Avoid double free and leak in
    trace_event__cleanup()/trace_event__init()
  perf trace-event: Fix heap overflows in
    read_ftrace_printk()/read_saved_cmdline()
  perf trace-event: Fix infinite loop in skip()

 tools/perf/util/trace-event-read.c | 54 ++++++++++++++++++++----------
 tools/perf/util/trace-event.c      | 11 +++++-
 2 files changed, 47 insertions(+), 18 deletions(-)

---
Changes in v4:
- Fixed a leak in trace_event__init(): repeated calls overwrite
  t->pevent/t->plugin_list without releasing the existing handle.
- Fixed the same size + 1 overflow in read_saved_cmdline() as
  read_ftrace_printk() (size == ULLONG_MAX wraps to 0 in malloc()).
- Added <limits.h> for the UINT_MAX/ULLONG_MAX checks.

Changes in v3:
- Added new patch to fix double free in trace_event__cleanup().
- Added new patch to fix heap buffer overflow in read_ftrace_printk().
- Added new patch to fix infinite loop in skip().

Changes in v2:
- Added new patch to fix integer truncation in do_read() and skip().
- Organized as patch series to separate the two security fixes.

v1: https://lore.kernel.org/linux-perf-users/20260722113552.191143-3-tshah@linux.ibm.com/

-- 
2.47.3

[PATCH v4 1/5] perf trace-event: Fix buffer overflow in read_string()

[Tanushree Shah]

read_string() writes into buf[BUFSIZ] one byte at a time without
checking 'size' against the buffer bound before each write. A
string longer than BUFSIZ in the input overflows the stack buffer.

Add a bounds check before each write to prevent overflow. On
overflow the function returns NULL, matching its other error paths.

Fixes: 9215545e99d8 ("perf: Convert perf tracing data into a tracing_data event")
Signed-off-by: Tanushree Shah <tshah@linux.ibm.com>
---
 tools/perf/util/trace-event-read.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
index ecbbb93f0185..afd458cf1387 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -127,6 +127,11 @@ static char *read_string(void)
 			}
 		}
 
+		if (size >= (int)sizeof(buf) - 1) {
+			pr_debug("string too long (max %zu bytes)", sizeof(buf) - 1);
+			goto out;
+		}
+
 		buf[size++] = c;
 
 		if (!c)
-- 
2.47.3

Re: [PATCH v4 1/5] perf trace-event: Fix buffer overflow in read_string()

[sashiko-bot]

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

Pre-existing issues:
- [High] Dangling pointer dereference of session->tevent.pevent on malformed perf.data due to error check bypass.
- [High] Implicit truncation of 64-bit sizes to 32-bit signed integers in `do_read` and `skip`, and integer overflows in allocations, lead to stack/heap buffer overflows and uninitialized memory disclosures.
--

--- Patch [1]: [PATCH v4 1/5] perf trace-event: Fix buffer overflow in read_string() ---
commit b0a22e84b8d80d3f8d078756fde29ba8352194d0
Author: Tanushree Shah <tshah@linux.ibm.com>

perf trace-event: Fix buffer overflow in read_string()

This commit fixes a stack buffer overflow in read_string() by adding a bounds
check before writing each character into the buffer. If the string from the
input is longer than BUFSIZ, it now prevents the overflow and returns NULL,
matching other error paths.

> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
> index ecbbb93f01853..afd458cf1387d 100644
> --- a/tools/perf/util/trace-event-read.c
> +++ b/tools/perf/util/trace-event-read.c
> @@ -127,6 +127,11 @@ static char *read_string(void)
>  			}
>  		}
>  
> +		if (size >= (int)sizeof(buf) - 1) {
> +			pr_debug("string too long (max %zu bytes)", sizeof(buf) - 1);
> +			goto out;
> +		}
> +
>  		buf[size++] = c;
>  
>  		if (!c)

[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing buffer
overflows in this file, are there similar truncation issues in do_read() and
skip()?

Looking at do_read() and skip() in tools/perf/util/trace-event-read.c, they
take size as a signed int:

static int do_read(void *data, int size)
static void skip(int size)

When a large 32-bit or 64-bit size is read from the trace data and passed to
these functions, it can be truncated to a negative integer. In skip(), could
this negative value cause the computed read amount to become negative,
triggering do_read() and eventually the read() system call to attempt reading a
massive amount of data into the local stack buffer?

Similarly, in read_ftrace_printk() and read_saved_cmdline(), can the
malloc(size + 1) calculation overflow to 0 when a large size is provided,
creating a tiny allocation before do_read() is called with the truncated
negative size, leading to a heap overflow?

[Severity: High]
This is a pre-existing issue, but can a malformed perf.data file cause a
dangling pointer dereference during error handling?

In tools/perf/util/header.c:perf_event__process_tracing_data(), if
trace_report() returns an error (-1), size_read becomes -1. The subsequent
padding calculation:

	padding = PERF_ALIGN(-1, 8) - (-1);

evaluates to 1. This means size_read + padding equals 0. If the provided event
size is 0, the size mismatch check:

	if (size_read + padding != size)

evaluates to 0 != 0, which is false, bypassing the error check.

Since trace_report() triggers trace_event__cleanup() on error paths, which
frees t->pevent without setting it to NULL:

tools/perf/util/trace-event.c:trace_event__cleanup() {
	tep_unload_plugins(t->plugin_list, t->pevent);
	tep_free(t->pevent);
}

Does this bypass lead to evlist__prepare_tracepoint_events() being called with
a dangling session->tevent.pevent pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725184953.234759-1-tshah@linux.ibm.com?part=1

[PATCH v4 2/5] perf trace-event: Fix integer truncation in do_read() and skip()

[Tanushree Shah]

The do_read() and skip() functions use 'int' for size parameters,
truncating 64-bit sizes from callers. This causes two issues:

1. Uninitialized memory dump: do_read() reads fewer bytes than
   allocated, leaving uninitialized heap memory that gets written
   to output files.

2. Out-of-bounds read: Parsing functions process the full 64-bit
   size while only partial data was read into the buffer.

Change do_read(), __do_read(), and skip() to use size_t for size
parameters and ssize_t for return values (where applicable), matching
read()/write() system calls.
Update callers to use ssize_t for storing return values.

Fixes: 454c407ec17a ("perf tools: Get rid of read_or_die() in trace-event-read.c")
Signed-off-by: Tanushree Shah <tshah@linux.ibm.com>
---
 tools/perf/util/trace-event-read.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
index afd458cf1387..52ed496d92c3 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -25,18 +25,18 @@ static int input_fd;
 static ssize_t trace_data_size;
 static bool repipe;
 
-static int __do_read(int fd, void *buf, int size)
+static ssize_t __do_read(int fd, void *buf, size_t size)
 {
-	int rsize = size;
+	size_t rsize = size;
 
 	while (size) {
-		int ret = read(fd, buf, size);
+		ssize_t ret = read(fd, buf, size);
 
 		if (ret <= 0)
 			return -1;
 
 		if (repipe) {
-			int retw = write(STDOUT_FILENO, buf, ret);
+			ssize_t retw = write(STDOUT_FILENO, buf, ret);
 
 			if (retw <= 0 || retw != ret) {
 				pr_debug("repiping input file");
@@ -51,13 +51,13 @@ static int __do_read(int fd, void *buf, int size)
 	return rsize;
 }
 
-static int do_read(void *data, int size)
+static ssize_t do_read(void *data, size_t size)
 {
-	int r;
+	ssize_t r;
 
 	r = __do_read(input_fd, data, size);
 	if (r <= 0) {
-		pr_debug("reading input file (size expected=%d received=%d)",
+		pr_debug("reading input file (size expected=%zu received=%zd)",
 			 size, r);
 		return -1;
 	}
@@ -68,10 +68,10 @@ static int do_read(void *data, int size)
 }
 
 /* If it fails, the next read will report it */
-static void skip(int size)
+static void skip(size_t size)
 {
 	char buf[BUFSIZ];
-	int r;
+	size_t r;
 
 	while (size) {
 		r = size > BUFSIZ ? BUFSIZ : size;
@@ -202,7 +202,7 @@ static int read_header_files(struct tep_handle *pevent)
 	unsigned long long size;
 	char *header_page;
 	char buf[BUFSIZ];
-	int ret = 0;
+	ssize_t ret = 0;
 
 	if (do_read(buf, 12) < 0)
 		return -1;
@@ -250,7 +250,7 @@ static int read_header_files(struct tep_handle *pevent)
 
 static int read_ftrace_file(struct tep_handle *pevent, unsigned long long size)
 {
-	int ret;
+	ssize_t ret;
 	char *buf;
 
 	buf = malloc(size);
@@ -276,7 +276,7 @@ static int read_ftrace_file(struct tep_handle *pevent, unsigned long long size)
 static int read_event_file(struct tep_handle *pevent, char *sys,
 			   unsigned long long size)
 {
-	int ret;
+	ssize_t ret;
 	char *buf;
 
 	buf = malloc(size);
@@ -322,7 +322,7 @@ static int read_event_files(struct tep_handle *pevent)
 	int systems;
 	int count;
 	int i,x;
-	int ret;
+	ssize_t ret;
 
 	systems = read4(pevent);
 
@@ -350,7 +350,7 @@ static int read_saved_cmdline(struct tep_handle *pevent)
 {
 	unsigned long long size;
 	char *buf;
-	int ret;
+	ssize_t ret;
 
 	/* it can have 0 size */
 	size = read8(pevent);
-- 
2.47.3

Re: [PATCH v4 2/5] perf trace-event: Fix integer truncation in do_read() and skip()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Integer overflow in read_saved_cmdline() leads to zero-sized allocation and massive out-of-bounds heap write.
- [High] The skip() function still truncates 64-bit sizes on 32-bit architectures, leaving the file pointer misaligned.
--

--- Patch [2]: [PATCH v4 2/5] perf trace-event: Fix integer truncation in do_read() and skip() ---
commit ceab52608d4a89e2c3da800798280df9bb428d4e
Author: Tanushree Shah <tshah@linux.ibm.com>

perf trace-event: Fix integer truncation in do_read() and skip()

The do_read() and skip() functions use int for size parameters,
truncating 64-bit sizes from callers. This patch changes do_read(),
__do_read(), and skip() to use size_t for size parameters and ssize_t
for return values.

> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
> index afd458cf1387d..52ed496d92c3b 100644
> --- a/tools/perf/util/trace-event-read.c
> +++ b/tools/perf/util/trace-event-read.c
[ ... ]
> @@ -68,10 +68,10 @@ static int do_read(void *data, int size)
>  }
>  
>  /* If it fails, the next read will report it */
> -static void skip(int size)
> +static void skip(size_t size)

[Severity: High]
This isn't a bug introduced by this patch, but does skip() still truncate
64-bit sizes on 32-bit architectures?

Since size_t is 32-bit on 32-bit platforms, passing an unsigned long long
from read8() into skip() can truncate values larger than 4GB.

Could a crafted perf.data file providing a massive header event size cause
skip() to truncate the value, leaving the file pointer misaligned?

>  {
>  	char buf[BUFSIZ];
> -	int r;
> +	size_t r;
[ ... ]
> @@ -350,7 +350,7 @@ static int read_saved_cmdline(struct tep_handle *pevent)
>  {
>  	unsigned long long size;
>  	char *buf;
> -	int ret;
> +	ssize_t ret;
>  
>  	/* it can have 0 size */
>  	size = read8(pevent);

[Severity: Critical]
This isn't a bug introduced by this patch, but does this code overflow
size + 1 during memory allocation?

If a crafted perf.data file sets the saved cmdline size to ULLONG_MAX
(or ULONG_MAX on 32-bit architectures):

tools/perf/util/trace-event-read.c:read_saved_cmdline() {
    ...
    size = read8(pevent);
    ...
    buf = malloc(size + 1);
    if (buf == NULL) {
        pr_debug("memory allocation failure\n");
        return -1;
    }

    ret = do_read(buf, size);
    ...
}

The size + 1 calculation overflows to 0, causing malloc(0) to return a
small valid heap chunk.

When the original massive size is then passed to do_read(), could the read()
system call overwrite the heap with out-of-bounds file data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725184953.234759-1-tshah@linux.ibm.com?part=2

[PATCH v4 3/5] perf trace-event: Avoid double free and leak in trace_event__cleanup()/trace_event__init()

[Tanushree Shah]

trace_event__cleanup() frees t->pevent but never clears the
pointer. It can be called twice on the same trace_event: once
from trace_report()'s error path, and again from
perf_session__delete() during session teardown, resulting in a
double free / use-after-free.

Separately, trace_event__init() overwrites t->pevent/t->plugin_list
without releasing any existing handle, leaking memory if it's
called more than once on the same struct. eg. via a perf.data
file with multiple PERF_RECORD_HEADER_TRACING_DATA headers.

Guard against re-entry by returning early if t->pevent is already
NULL, and clear it after cleanup so a repeat call is a safe no-op.
Call trace_event__cleanup() at the start of trace_event__init(),
so a repeated init releases any existing handle before allocating
a new one.

Signed-off-by: Tanushree Shah <tshah@linux.ibm.com>
---
 tools/perf/util/trace-event.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index 6a8c66c64b70..000c1e1d68c1 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -26,7 +26,11 @@ static bool tevent_initialized;
 
 int trace_event__init(struct trace_event *t)
 {
-	struct tep_handle *pevent = tep_alloc();
+	struct tep_handle *pevent;
+
+	trace_event__cleanup(t);
+
+	pevent = tep_alloc();
 
 	if (pevent) {
 		t->plugin_list = tep_load_plugins(pevent);
@@ -63,8 +67,13 @@ int trace_event__register_resolver(struct machine *machine,
 
 void trace_event__cleanup(struct trace_event *t)
 {
+	if (!t->pevent)
+		return;
+
 	tep_unload_plugins(t->plugin_list, t->pevent);
 	tep_free(t->pevent);
+	t->pevent = NULL;
+	t->plugin_list = NULL;
 }
 
 /*
-- 
2.47.3

Re: [PATCH v4 3/5] perf trace-event: Avoid double free and leak in trace_event__cleanup()/trace_event__init()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Fixing a memory leak in `trace_event__init()` introduces an exploitable Use-After-Free when processing malformed or crafted `perf.data` files with multiple `PERF_RECORD_HEADER_TRACING_DATA` headers.
--

--- Patch [3]: [PATCH v4 3/5] perf trace-event: Avoid double free and leak in trace_event__cleanup()/trace_event__init() ---
commit 47e0085adcff000a3a6472e037799a5ddd33c012
Author: Tanushree Shah <tshah@linux.ibm.com>

perf trace-event: Avoid double free and leak in trace_event__cleanup()/trace_event__init()

This commit addresses a double-free in trace_event__cleanup() and a memory
leak in trace_event__init() when a perf.data file contains multiple
PERF_RECORD_HEADER_TRACING_DATA headers. It fixes the leak by calling
trace_event__cleanup() before allocating a new tep handle.

> diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
> index 6a8c66c64b705..000c1e1d68c16 100644
> --- a/tools/perf/util/trace-event.c
> +++ b/tools/perf/util/trace-event.c
> @@ -26,7 +26,11 @@ static bool tevent_initialized;
>  
>  int trace_event__init(struct trace_event *t)
>  {
> -	struct tep_handle *pevent = tep_alloc();
> +	struct tep_handle *pevent;
> +
> +	trace_event__cleanup(t);
> +

[Severity: Critical]
Could this cleanup introduce a use-after-free when parsing a perf.data
file with multiple PERF_RECORD_HEADER_TRACING_DATA headers?

When trace_event__cleanup(t) frees the existing tep_event objects, any 
evsel->tp_format pointers that were set during the first header's parsing 
are left dangling.

Specifically, in tools/perf/util/header.c:evsel__prepare_tracepoint_event(),
it short-circuits if the pointer is already set:

	/* already prepared */
	if (evsel->tp_format)
		return 0;

This short-circuit prevents evsel->tp_format from being updated to point to
the newly allocated memory. Later, tools/perf/util/evsel.c:evsel__tp_format()
retrieves this dangling pointer:

	if (tp_format)
		return tp_format;

Does this leave the perf tool vulnerable to memory corruption when analyzing
a malformed or maliciously crafted perf.data file?

> +	pevent = tep_alloc();
>  
>  	if (pevent) {
>  		t->plugin_list = tep_load_plugins(pevent);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725184953.234759-1-tshah@linux.ibm.com?part=3

[PATCH v4 4/5] perf trace-event: Fix heap overflows in read_ftrace_printk()/read_saved_cmdline()

[Tanushree Shah]

Both functions read an attacker-controlled size directly from the
input file and pass size + 1 to malloc() before reading size bytes
into the result:

read_ftrace_printk(): size is an unsigned int from read4(). When
size == UINT_MAX, size + 1 overflows to 0, so malloc(0) returns a
minimal allocation while size itself remains UINT_MAX.

read_saved_cmdline(): size is an unsigned long long from read8().
When size == ULLONG_MAX, size + 1 overflows to 0 the same way.

In both cases, do_read(buf, size) then attempts to read the full,
unwrapped size into the tiny allocated buffer, a heap buffer
overflow.

This was previously masked by do_read()'s size parameter being
'int': passing these values truncated them, which the read()
syscall's own boundary checks rejected before any data was read.
Fixing that truncation (widening do_read() to size_t) is correct
on its own, but it removes this accidental protection and exposes
the pre-existing missing bounds check in both functions.

Reject the one value that causes the overflow before it's used, in
each function.

Signed-off-by: Tanushree Shah <tshah@linux.ibm.com>
---
 tools/perf/util/trace-event-read.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
index 52ed496d92c3..53f1920c3fcb 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -15,6 +15,7 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <errno.h>
+#include <limits.h>
 
 #include "trace-event.h"
 #include "debug.h"
@@ -180,6 +181,11 @@ static int read_ftrace_printk(struct tep_handle *pevent)
 	if (!size)
 		return 0;
 
+	if (size == UINT_MAX) {
+		pr_debug("invalid ftrace printk size\n");
+		return -1;
+	}
+
 	buf = malloc(size + 1);
 	if (buf == NULL)
 		return -1;
@@ -357,6 +363,11 @@ static int read_saved_cmdline(struct tep_handle *pevent)
 	if (!size)
 		return 0;
 
+	if (size == ULLONG_MAX) {
+		pr_debug("invalid saved cmdline size");
+		return -1;
+	}
+
 	buf = malloc(size + 1);
 	if (buf == NULL) {
 		pr_debug("memory allocation failure\n");
-- 
2.47.3

Re: [PATCH v4 4/5] perf trace-event: Fix heap overflows in read_ftrace_printk()/read_saved_cmdline()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Incomplete fix for a heap buffer overflow in read_saved_cmdline() on 32-bit architectures.
- [High] Data corruption and pipe failures in read_proc_kallsyms() due to the use of lseek().
--

--- Patch [4]: [PATCH v4 4/5] perf trace-event: Fix heap overflows in read_ftrace_printk()/read_saved_cmdline() ---
commit 8469cf15f119b26dcccff6c290c31c101e46c1ab
Author: Tanushree Shah <tshah@linux.ibm.com>

perf trace-event: Fix heap overflows in read_ftrace_printk()/read_saved_cmdline()

This commit attempts to fix heap buffer overflows in read_ftrace_printk()
and read_saved_cmdline() by rejecting maximum integer sizes. This prevents
an integer overflow where size + 1 wraps to zero, avoiding a minimal memory
allocation before calling do_read().

> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
> index 52ed496d92c3b..53f1920c3fcbe 100644
> --- a/tools/perf/util/trace-event-read.c
> +++ b/tools/perf/util/trace-event-read.c

[ ... ]

> @@ -357,6 +363,11 @@ static int read_saved_cmdline(struct tep_handle *pevent)
>  	if (!size)
>  		return 0;
>  
> +	if (size == ULLONG_MAX) {
> +		pr_debug("invalid saved cmdline size");
> +		return -1;
> +	}
> +
>  	buf = malloc(size + 1);

[Severity: Critical]
This isn't a bug introduced by this patch, but on 32-bit architectures,
does this check prevent size + 1 from wrapping to zero if size is
0xFFFFFFFF?

Since size is an unsigned long long and malloc takes a size_t, a 32-bit
SIZE_MAX value will bypass this ULLONG_MAX check. When passed to
malloc(size + 1), it would truncate to 32 bits and wrap to zero, causing
do_read() to overflow the allocation. Could we check against SIZE_MAX here
instead?

[Severity: High]
This is a pre-existing issue, but looking at read_proc_kallsyms() in the
same file:

read_proc_kallsyms() {
    ...
    lseek(input_fd, size, SEEK_CUR);
    trace_data_size += size;
    return 0;
}

Does using lseek() instead of the skip() helper silently fail on pipes?

If input_fd is a pipe, lseek fails and returns -1, which is ignored. This
prevents the file pointer from advancing and can cause the next parsing
function to interpret the payload bytes as its own trace section.

Also, when repipe is enabled, the size header is repiped but the skipped
payload bytes are not, which corrupts the resulting perf.data file. Could
this be updated to use skip() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725184953.234759-1-tshah@linux.ibm.com?part=4

[PATCH v4 5/5] perf trace-event: Fix infinite loop in skip()

[Tanushree Shah]

skip() ignores do_read()'s return value and unconditionally
subtracts the requested chunk size from 'size' on every iteration.
This was previously bounded by size being 'int': a maliciously
large 64-bit value was truncated on assignment, capping the loop
early by accident.

Now that size is size_t, a crafted file supplying a very large
size causes skip() to keep requesting BUFSIZ-sized reads and
subtracting BUFSIZ from size regardless of whether do_read()
actually succeeds, spinning indefinitely even after EOF or a read
error.

Check do_read()'s return value and break out of the loop on
failure or EOF, so forward progress is only counted when a read
actually succeeds.

Signed-off-by: Tanushree Shah <tshah@linux.ibm.com>
---
 tools/perf/util/trace-event-read.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
index 53f1920c3fcb..db1622fc99c2 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -72,12 +72,16 @@ static ssize_t do_read(void *data, size_t size)
 static void skip(size_t size)
 {
 	char buf[BUFSIZ];
-	size_t r;
+	ssize_t ret;
 
 	while (size) {
-		r = size > BUFSIZ ? BUFSIZ : size;
-		do_read(buf, r);
-		size -= r;
+		size_t len = size > BUFSIZ ? BUFSIZ : size;
+
+		ret = do_read(buf, len);
+		if (ret <= 0)
+			break;
+
+		size -= ret;
 	}
 }
 
-- 
2.47.3

Re: [PATCH v4 5/5] perf trace-event: Fix infinite loop in skip()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Incomplete fix for heap buffer overflow in read_saved_cmdline() on 32-bit systems.
- [High] Integer truncation on 32-bit systems leads to Out-Of-Bounds reads during file parsing.
- [High] Unhandled lseek() failure and bypassed repipe mechanism in read_proc_kallsyms().
- [Medium] Incorrect PTR_ERR usage extracts error from NULL pointer in evsel__tp_format().
--

--- Patch [5]: [PATCH v4 5/5] perf trace-event: Fix infinite loop in skip() ---
commit 3b2c19f9da89212b3fdd3fac1e4250b2f316409f
Author: Tanushree Shah <tshah@linux.ibm.com>

perf trace-event: Fix infinite loop in skip()

The patch updates skip() to check the return value of do_read() and break out
of the loop on failure or end-of-file. This prevents an infinite loop when
reading crafted files with very large sizes or when hitting read errors.

> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
> index 53f1920c3fcbe..db1622fc99c23 100644
> --- a/tools/perf/util/trace-event-read.c
> +++ b/tools/perf/util/trace-event-read.c
> @@ -72,12 +72,16 @@ static ssize_t do_read(void *data, size_t size)
>  static void skip(size_t size)
>  {
>  	char buf[BUFSIZ];
> -	size_t r;
> +	ssize_t ret;
>  
>  	while (size) {
> -		r = size > BUFSIZ ? BUFSIZ : size;
> -		do_read(buf, r);
> -		size -= r;
> +		size_t len = size > BUFSIZ ? BUFSIZ : size;
> +
> +		ret = do_read(buf, len);
> +		if (ret <= 0)
> +			break;
> +
> +		size -= ret;
>  	}
>  }

[Severity: Critical]
This is a pre-existing issue, but could this malloc allocation and subsequent
do_read() in read_saved_cmdline() cause a heap buffer overflow on 32-bit
systems?

tools/perf/util/trace-event-read.c:read_saved_cmdline() {
    ...
	if (size == ULLONG_MAX) {
		pr_debug("invalid saved cmdline size");
		return -1;
	}

	buf = malloc(size + 1);
    ...
	ret = do_read(buf, size);
    ...
}

Since size is an unsigned long long, if an attacker provides 0xFFFFFFFF
(UINT_MAX) from a crafted perf.data file, the size == ULLONG_MAX check
will not catch it.

The malloc(size + 1) calculation would then wrap around to 0 on 32-bit
systems, resulting in a tiny allocation. The subsequent do_read(buf, size)
call would use the original 0xFFFFFFFF value, causing a massive out-of-bounds
write.

[Severity: High]
This isn't a regression introduced by this patch, but does 32-bit truncation
of size in read_header_files() lead to an out-of-bounds read?

tools/perf/util/trace-event-read.c:read_header_files() {
    ...
	size = read8(pevent);

	header_page = malloc(size);
    ...
	if (do_read(header_page, size) < 0) {
    ...
	if (!tep_parse_header_page(pevent, header_page, size,
    ...
}

size is a 64-bit unsigned long long. On 32-bit systems, passing size to
malloc() and do_read() implicitly truncates it to 32 bits.

If an attacker supplies 0x100000001ULL, a 1-byte buffer is allocated and
read. However, tep_parse_header_page() receives the full 64-bit size,
causing it to potentially read past the 1-byte heap allocation.

The same integer truncation pattern also appears in read_ftrace_file() and
read_event_file().

[Severity: High]
This is a pre-existing issue, but does this lseek() failure break stream
parsing when running in pipe mode?

tools/perf/util/trace-event-read.c:read_proc_kallsyms() {
    ...
	/*
	 * Just skip it, now that we configure libtraceevent to use the
	 * tools/perf/ symbol resolver.
    ...
	lseek(input_fd, size, SEEK_CUR);
	trace_data_size += size;
	return 0;
}

If input_fd is a pipe, lseek() returns -1 (ESPIPE). Because the error is
ignored, the file pointer remains unchanged, and subsequent reads might
incorrectly interpret the kallsyms payload as trace headers or events.

Additionally, using lseek() bypasses the repipe logic in __do_read(), meaning
these skipped payload bytes aren't written to standard output, breaking
repiped streams. Should this use skip(size) instead?

[Severity: Medium]
This isn't a regression introduced by this patch, but does evsel__tp_format()
extract the error code from a NULL pointer?

tools/perf/util/evsel.c:evsel__tp_format() {
    ...
	if (!evsel->tp_sys)
		tp_format = trace_event__tp_format_id(evsel->core.attr.config);
	else
		tp_format = trace_event__tp_format(evsel->tp_sys, evsel->tp_name);

	if (IS_ERR(tp_format)) {
		int err = -PTR_ERR(evsel->tp_format);

		errno = err;
		pr_err("Error getting tracepoint format '%s': %m\n",
    ...
}

If the trace format fails, it returns an ERR_PTR into the local variable
tp_format. But the code tries to extract the error using -PTR_ERR() on
evsel->tp_format, which is still NULL.

This would result in errno being set to 0, causing pr_err() to log "Success"
instead of the actual error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725184953.234759-1-tshah@linux.ibm.com?part=5