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

[PATCH] 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] perf trace-event: Fix buffer overflow in read_string()

[sashiko-bot]

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 integer truncation when passing a 64-bit size to a 32-bit `do_read` parameter leads to out-of-bounds reads or massive uninitialized memory parsing.
--

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

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

This patch adds a bounds check to read_string() before writing to the buffer.
It ensures that the size does not exceed the bounds of buf, preventing a
buffer overflow when processing trace events.

> 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 integer truncation when passing
a 64-bit size to a 32-bit do_read() parameter can lead to out-of-bounds reads.

In tools/perf/util/trace-event-read.c, do_read() expects a 32-bit integer:

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

However, read_ftrace_file() passes an unsigned long long size parameter
into it:

static int read_ftrace_file(struct tep_handle *pevent, unsigned long long size)
{
    ...
    buf = malloc(size);
    if (buf == NULL) {
        ...
    }

    ret = do_read(buf, size);
    if (ret < 0) {
        ...
    }

    ret = parse_ftrace_file(pevent, buf, size);
    ...
}

If a maliciously crafted trace file provides a size like 0x100000005,
malloc(size) on 32-bit architectures truncates and allocates only 5 bytes. The
size parameter in parse_ftrace_file() remains the full 64-bit value, which
would result in a massive out-of-bounds read.

On 64-bit architectures, do_read() truncates the size and reads only 5 bytes,
causing parse_ftrace_file() to process up to 4GB of mostly uninitialized
memory.

Does this code need to update do_read() to use size_t or ssize_t to properly
handle the size parameter?

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