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

Thanks for the review.

On 24/07/26 20:24, [email protected] wrote:
> 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 <[email protected]>
> 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>.

Fair point on musl compatibility. Although it compiles fine without it 
via transitive includes, I will add it explicitly. Will include in v4.
> 
>>   	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?
> 
Confirmed, same as noted earlier in this thread ( patch 2 review ). Will 
fix it in v4.