Re: [PATCH v3 4/5] perf trace-event: Fix heap buffer overflow in read_ftrace_printk()

[sashiko-bot@kernel.org]

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