All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/5] perf trace-event: Fix security bugs in trace-event-read.c and trace-event.c
@ 2026-07-24 14:13 Tanushree Shah
  2026-07-24 14:13 ` [PATCH v3 1/5] perf trace-event: Fix buffer overflow in read_string() Tanushree Shah
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Tanushree Shah @ 2026-07-24 14:13 UTC (permalink / raw)
  To: acme, jolsa, adrian.hunter, vmolnaro, mpetlan, tmricht, maddy,
	irogers, namhyung
  Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, Tejas.Manhas1,
	Tanushree.Shah, Shivani.Nittor, 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.
4. Heap buffer overflow in read_ftrace_printk(): 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 in trace_event__cleanup()
  perf trace-event: Fix heap buffer overflow in read_ftrace_printk()
  perf trace-event: Fix infinite loop in skip()

 tools/perf/util/trace-event-read.c | 48 +++++++++++++++++++-----------
 tools/perf/util/trace-event.c      |  4 +++
 2 files changed, 35 insertions(+), 17 deletions(-)

---
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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/5] perf trace-event: Fix buffer overflow in read_string()
  2026-07-24 14:13 [PATCH v3 0/5] perf trace-event: Fix security bugs in trace-event-read.c and trace-event.c Tanushree Shah
@ 2026-07-24 14:13 ` Tanushree Shah
  2026-07-24 14:47   ` sashiko-bot
  2026-07-24 14:13 ` [PATCH v3 2/5] perf trace-event: Fix integer truncation in do_read() and skip() Tanushree Shah
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Tanushree Shah @ 2026-07-24 14:13 UTC (permalink / raw)
  To: acme, jolsa, adrian.hunter, vmolnaro, mpetlan, tmricht, maddy,
	irogers, namhyung
  Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, Tejas.Manhas1,
	Tanushree.Shah, Shivani.Nittor, 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/5] perf trace-event: Fix integer truncation in do_read() and skip()
  2026-07-24 14:13 [PATCH v3 0/5] perf trace-event: Fix security bugs in trace-event-read.c and trace-event.c Tanushree Shah
  2026-07-24 14:13 ` [PATCH v3 1/5] perf trace-event: Fix buffer overflow in read_string() Tanushree Shah
@ 2026-07-24 14:13 ` Tanushree Shah
  2026-07-24 14:47   ` sashiko-bot
  2026-07-24 14:13 ` [PATCH v3 3/5] perf trace-event: Avoid double free in trace_event__cleanup() Tanushree Shah
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Tanushree Shah @ 2026-07-24 14:13 UTC (permalink / raw)
  To: acme, jolsa, adrian.hunter, vmolnaro, mpetlan, tmricht, maddy,
	irogers, namhyung
  Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, Tejas.Manhas1,
	Tanushree.Shah, Shivani.Nittor, 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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 3/5] perf trace-event: Avoid double free in trace_event__cleanup()
  2026-07-24 14:13 [PATCH v3 0/5] perf trace-event: Fix security bugs in trace-event-read.c and trace-event.c Tanushree Shah
  2026-07-24 14:13 ` [PATCH v3 1/5] perf trace-event: Fix buffer overflow in read_string() Tanushree Shah
  2026-07-24 14:13 ` [PATCH v3 2/5] perf trace-event: Fix integer truncation in do_read() and skip() Tanushree Shah
@ 2026-07-24 14:13 ` Tanushree Shah
  2026-07-24 14:55   ` sashiko-bot
  2026-07-24 14:13 ` [PATCH v3 4/5] perf trace-event: Fix heap buffer overflow in read_ftrace_printk() Tanushree Shah
  2026-07-24 14:13 ` [PATCH v3 5/5] perf trace-event: Fix infinite loop in skip() Tanushree Shah
  4 siblings, 1 reply; 11+ messages in thread
From: Tanushree Shah @ 2026-07-24 14:13 UTC (permalink / raw)
  To: acme, jolsa, adrian.hunter, vmolnaro, mpetlan, tmricht, maddy,
	irogers, namhyung
  Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, Tejas.Manhas1,
	Tanushree.Shah, Shivani.Nittor, 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.

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.

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

diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index 6a8c66c64b70..cf40e98d1617 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -63,8 +63,12 @@ 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;
 }
 
 /*
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 4/5] perf trace-event: Fix heap buffer overflow in read_ftrace_printk()
  2026-07-24 14:13 [PATCH v3 0/5] perf trace-event: Fix security bugs in trace-event-read.c and trace-event.c Tanushree Shah
                   ` (2 preceding siblings ...)
  2026-07-24 14:13 ` [PATCH v3 3/5] perf trace-event: Avoid double free in trace_event__cleanup() Tanushree Shah
@ 2026-07-24 14:13 ` Tanushree Shah
  2026-07-24 14:54   ` sashiko-bot
  2026-07-24 14:13 ` [PATCH v3 5/5] perf trace-event: Fix infinite loop in skip() Tanushree Shah
  4 siblings, 1 reply; 11+ messages in thread
From: Tanushree Shah @ 2026-07-24 14:13 UTC (permalink / raw)
  To: acme, jolsa, adrian.hunter, vmolnaro, mpetlan, tmricht, maddy,
	irogers, namhyung
  Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, Tejas.Manhas1,
	Tanushree.Shah, Shivani.Nittor, Tanushree Shah

size is read directly from the input file as an unsigned int.
When size == UINT_MAX, malloc(size + 1) overflows to malloc(0),
so malloc(0) returns a minimal allocation while size itself
remains UINT_MAX. do_read(buf, size) then attempts to read
~4 GiB into that tiny buffer, causing a heap buffer overflow.

This was previously masked by do_read()'s size parameter being
'int': passing UINT_MAX truncated it to -1, which the read()
syscall rejected before any data was read. Widening do_read() to
size_t is correct on its own, but it removes this accidental guard
and exposes the pre-existing missing bounds check here.

Fix it by rejecting size == UINT_MAX before the allocation.

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 52ed496d92c3..147a3b95ce06 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -180,6 +180,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;
-- 
2.47.3



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 5/5] perf trace-event: Fix infinite loop in skip()
  2026-07-24 14:13 [PATCH v3 0/5] perf trace-event: Fix security bugs in trace-event-read.c and trace-event.c Tanushree Shah
                   ` (3 preceding siblings ...)
  2026-07-24 14:13 ` [PATCH v3 4/5] perf trace-event: Fix heap buffer overflow in read_ftrace_printk() Tanushree Shah
@ 2026-07-24 14:13 ` Tanushree Shah
  2026-07-24 14:57   ` sashiko-bot
  4 siblings, 1 reply; 11+ messages in thread
From: Tanushree Shah @ 2026-07-24 14:13 UTC (permalink / raw)
  To: acme, jolsa, adrian.hunter, vmolnaro, mpetlan, tmricht, maddy,
	irogers, namhyung
  Cc: linux-perf-users, linuxppc-dev, atrajeev, hbathini, Tejas.Manhas1,
	Tanushree.Shah, Shivani.Nittor, 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 147a3b95ce06..529a56c3730f 100644
--- a/tools/perf/util/trace-event-read.c
+++ b/tools/perf/util/trace-event-read.c
@@ -71,12 +71,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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/5] perf trace-event: Fix buffer overflow in read_string()
  2026-07-24 14:13 ` [PATCH v3 1/5] perf trace-event: Fix buffer overflow in read_string() Tanushree Shah
@ 2026-07-24 14:47   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-24 14:47 UTC (permalink / raw)
  To: Tanushree Shah; +Cc: linux-perf-users

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

Pre-existing issues:
- [High] Integer truncation when passing 64-bit sizes to do_read() and skip() leads to parser stream corruption and potential out-of-bounds memory reads.
- [High] read_proc_kallsyms() attempts to skip the kallsyms payload using lseek() but ignores the return value, breaking parsing when perf.data is read from an unseekable stream like a pipe.
--

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

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

This commit adds a bounds check to read_string() to prevent a stack buffer
overflow when reading strings longer than BUFSIZ from the input. It ensures
the function returns NULL on overflow, 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
[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential integer truncation
when passing 64-bit sizes to do_read() and skip().

In tools/perf/util/trace-event-read.c, do_read() and skip() take an int:

static int do_read(void *data, int size)
{ ... }

static void skip(int size)
{ ... }

But in read_header_files(), a 64-bit size is passed directly to them:

static int read_header_files(struct tep_handle *pevent)
{
	unsigned long long size;
	...
	size = read8(pevent);
	...
	if (do_read(header_page, size) < 0) {
	...
	size = read8(pevent);
	skip(size);
}

Could this lead to silent truncation and parser stream corruption if a
malicious perf.data file provides a large 64-bit size (like 0x100000005)?

On 32-bit systems, malloc() would allocate a small buffer, while the parser
uses the full 64-bit size, causing an immediate out-of-bounds read. On 64-bit
systems, skip() truncates the size, skipping only a few bytes, which might
desync the parser and allow arbitrary data into subsequent parsing stages.


[Severity: High]
This is also a pre-existing issue, but does read_proc_kallsyms() properly
handle unseekable streams like pipes?

In tools/perf/util/trace-event-read.c:read_proc_kallsyms():

	lseek(input_fd, size, SEEK_CUR);

This lseek() is called without checking its return value or providing a
read-based fallback.

If a user pipes a perf.data file into the tool (e.g., perf report -i -),
the stream pointer is not advanced. Could subsequent parser functions
(like read_ftrace_printk()) interpret ASCII text from the kallsyms payload
as a 4-byte size header, resulting in massive arbitrary memory allocations
and tool crashes?

This appears to violate the subsystem rule which states that pipe streams
do not support seek and must be properly handled.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/5] perf trace-event: Fix integer truncation in do_read() and skip()
  2026-07-24 14:13 ` [PATCH v3 2/5] perf trace-event: Fix integer truncation in do_read() and skip() Tanushree Shah
@ 2026-07-24 14:47   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-24 14:47 UTC (permalink / raw)
  To: Tanushree Shah; +Cc: linux-perf-users

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

Pre-existing issues:
- [Critical] Integer overflow in read_saved_cmdline() leads to massive heap buffer overflow
--

--- Patch [2]: [PATCH v3 2/5] perf trace-event: Fix integer truncation in do_read() and skip() ---
commit 907c8b1b7591be61f25cc72141dbcdba5822dfa4
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 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.

> 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

[ ... ]

> @@ -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 is a pre-existing issue, but can this lead to a massive heap buffer
overflow?

In read_saved_cmdline(), size is an 8-byte value read directly from the
untrusted file:

	size = read8(pevent);
	if (!size)
		return 0;

	buf = malloc(size + 1);
	if (buf == NULL)
		return -1;

	ret = do_read(buf, size);

If the size read from the file is ULLONG_MAX (or UINT_MAX on 32-bit systems),
the size + 1 expression will wrap to 0.

The malloc(0) call will then succeed and return a small heap chunk, but the
original unwrapped massive size is passed to do_read(), which will loop and
read an enormous amount of data directly into the tiny buffer.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 4/5] perf trace-event: Fix heap buffer overflow in read_ftrace_printk()
  2026-07-24 14:13 ` [PATCH v3 4/5] perf trace-event: Fix heap buffer overflow in read_ftrace_printk() Tanushree Shah
@ 2026-07-24 14:54   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-24 14:54 UTC (permalink / raw)
  To: Tanushree Shah; +Cc: linux-perf-users

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

New issues:
- [Low] The patch uses `UINT_MAX` without explicitly including `<limits.h>`, violating musl compatibility guidelines.

Pre-existing issues:
- [Critical] The commit patches a heap buffer overflow in `read_ftrace_printk()` by preventing `malloc(size + 1)` from wrapping to 0, but misses an identical vulnerability in `read_saved_cmdline()`.
--

--- Patch [4]: [PATCH v3 4/5] perf trace-event: Fix heap buffer overflow in read_ftrace_printk() ---
commit e35d114b06414be20beac51dfd3a3e158eba2833
Author: Tanushree Shah <tshah@linux.ibm.com>
Subject: perf trace-event: Fix heap buffer overflow in read_ftrace_printk()

Fixes a heap buffer overflow in read_ftrace_printk() by preventing
malloc(size + 1) from wrapping to 0 when size is UINT_MAX.

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

[ ... ]

> @@ -180,6 +180,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;
> +	}
> +

[Severity: Low]
Does this code need to include <limits.h>?

The perf tools guidelines for musl compatibility state that we shouldn't rely
on implicit header inclusions.

Compiling against musl could fail since UINT_MAX is used without explicitly
including <limits.h>.

>  	buf = malloc(size + 1);
>  	if (buf == NULL)
>  		return -1;

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but doesn't the exact same wrapping
issue exist in read_saved_cmdline() in the same file?

tools/perf/util/trace-event-read.c:read_saved_cmdline() {
	unsigned long long size;
	char *buf;
	ssize_t ret;

	/* it can have 0 size */
	size = read8(pevent);
	if (!size)
		return 0;

	buf = malloc(size + 1);
	if (buf == NULL) {
		pr_debug("memory allocation failure\n");
		return -1;
	}

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

If size is ULLONG_MAX, size + 1 wraps to 0.

This causes malloc(0) to return a minimal allocation while the original
ULLONG_MAX size is passed to do_read(), leading to a heap buffer overflow.

Should a similar size check be added here to prevent malicious perf.data files
from triggering this?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/5] perf trace-event: Avoid double free in trace_event__cleanup()
  2026-07-24 14:13 ` [PATCH v3 3/5] perf trace-event: Avoid double free in trace_event__cleanup() Tanushree Shah
@ 2026-07-24 14:55   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-24 14:55 UTC (permalink / raw)
  To: Tanushree Shah; +Cc: linux-perf-users

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

Pre-existing issues:
- [High] Calling trace_event__init() repeatedly on the same struct trace_event leaks the previously allocated tep_handle and plugins.
--

--- Patch [3]: [PATCH v3 3/5] perf trace-event: Avoid double free in trace_event__cleanup() ---
commit 1eb2775370e0095c8df022b4a83dcd46f4b90b94
Author: Tanushree Shah <tshah@linux.ibm.com>

perf trace-event: Avoid double free in trace_event__cleanup()

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.

> diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
> index 6a8c66c64b705..cf40e98d16177 100644
> --- a/tools/perf/util/trace-event.c
> +++ b/tools/perf/util/trace-event.c
> @@ -63,8 +63,12 @@ int trace_event__register_resolver(struct machine *machine,
>  
>  void trace_event__cleanup(struct trace_event *t)
>  {
> +	if (!t->pevent)
> +		return;
> +

[Severity: High]
This isn't a bug introduced by this patch, but while we are making it safe
to call trace_event__cleanup() multiple times, should we also call it from
trace_event__init() to prevent memory leaks?

If a maliciously crafted perf.data file contains multiple
PERF_RECORD_HEADER_TRACING_DATA headers, trace_report() can be called
multiple times on the same session, invoking trace_event__init() repeatedly:

tools/perf/util/trace-event-read.c:trace_report() {
    ...
	if (trace_event__init(tevent)) {
    ...
}

This overwrites the previously allocated tep_handle and plugins without
freeing them, leaking the memory:

tools/perf/util/trace-event.c:trace_event__init() {
    ...
		t->plugin_list = tep_load_plugins(pevent);
		t->pevent  = pevent;
    ...
}

>  	tep_unload_plugins(t->plugin_list, t->pevent);
>  	tep_free(t->pevent);
> +	t->pevent = NULL;
>  }
>  
>  /*

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 5/5] perf trace-event: Fix infinite loop in skip()
  2026-07-24 14:13 ` [PATCH v3 5/5] perf trace-event: Fix infinite loop in skip() Tanushree Shah
@ 2026-07-24 14:57   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-24 14:57 UTC (permalink / raw)
  To: Tanushree Shah; +Cc: linux-perf-users

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a heap buffer overflow in `read_saved_cmdline()` due to an integer overflow when allocating memory for a buffer based on an untrusted `size`.
--

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

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

This patch fixes an infinite loop in skip() where do_read()'s return value
was ignored. By checking the return value, the loop now breaks correctly on
failure or EOF.

> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
> index 147a3b95ce06b..529a56c3730fa 100644
> --- a/tools/perf/util/trace-event-read.c
> +++ b/tools/perf/util/trace-event-read.c
> @@ -71,12 +71,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: High]
This isn't a bug introduced by this patch, but while reviewing this file I
noticed a potential heap buffer overflow in read_saved_cmdline() due to an
integer overflow when allocating memory:

tools/perf/util/trace-event-read.c:read_saved_cmdline() {
    ...
	size = read8(pevent);
	if (!size)
		return 0;

	buf = malloc(size + 1);
	if (buf == NULL) {
		pr_debug("memory allocation failure\n");
		return -1;
	}

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

If size is large enough, does size + 1 overflow to 0?

Could malloc(0) succeed and return a small valid pointer, causing do_read()
to overflow the buffer with size bytes?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-07-24 14:57 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-24 14:13 [PATCH v3 0/5] perf trace-event: Fix security bugs in trace-event-read.c and trace-event.c Tanushree Shah
2026-07-24 14:13 ` [PATCH v3 1/5] perf trace-event: Fix buffer overflow in read_string() Tanushree Shah
2026-07-24 14:47   ` sashiko-bot
2026-07-24 14:13 ` [PATCH v3 2/5] perf trace-event: Fix integer truncation in do_read() and skip() Tanushree Shah
2026-07-24 14:47   ` sashiko-bot
2026-07-24 14:13 ` [PATCH v3 3/5] perf trace-event: Avoid double free in trace_event__cleanup() Tanushree Shah
2026-07-24 14:55   ` sashiko-bot
2026-07-24 14:13 ` [PATCH v3 4/5] perf trace-event: Fix heap buffer overflow in read_ftrace_printk() Tanushree Shah
2026-07-24 14:54   ` sashiko-bot
2026-07-24 14:13 ` [PATCH v3 5/5] perf trace-event: Fix infinite loop in skip() Tanushree Shah
2026-07-24 14:57   ` sashiko-bot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.