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

[sashiko-bot@kernel.org]

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