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

[sashiko-bot@kernel.org]

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