Re: [PATCH v3 2/5] perf trace-event: Fix integer truncation in do_read() and skip()

[Tanushree Shah <tshah@linux.ibm.com>]

Thanks for the review

On 24/07/26 20:17, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Integer overflow in read_saved_cmdline() leads to massive heap buffer overflow
> --
> 
> --- Patch [2]: [PATCH v3 2/5] perf trace-event: Fix integer truncation in do_read() and skip() ---
> commit 907c8b1b7591be61f25cc72141dbcdba5822dfa4
> Author: Tanushree Shah <tshah@linux.ibm.com>
> 
> perf trace-event: Fix integer truncation in do_read() and skip()
> 
> The do_read() and skip() functions use 'int' for size parameters,
> truncating 64-bit sizes from callers. This causes two issues:
> 
> 1. Uninitialized memory dump: do_read() reads fewer bytes than
>     allocated, leaving uninitialized heap memory that gets written
>     to output files.
> 
> 2. Out-of-bounds read: Parsing functions process the full 64-bit
>     size while only partial data was read into the buffer.
> 
> Change do_read(), __do_read(), and skip() to use size_t for size
> parameters and ssize_t for return values (where applicable), matching
> read()/write() system calls.
> Update callers to use ssize_t for storing return values.
> 
>> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
>> index afd458cf1387d..52ed496d92c3b 100644
>> --- a/tools/perf/util/trace-event-read.c
>> +++ b/tools/perf/util/trace-event-read.c
> 
> [ ... ]
> 
>> @@ -350,7 +350,7 @@ static int read_saved_cmdline(struct tep_handle *pevent)
>>   {
>>   	unsigned long long size;
>>   	char *buf;
>> -	int ret;
>> +	ssize_t ret;
>>   
>>   	/* it can have 0 size */
>>   	size = read8(pevent);
> 
> [Severity: Critical]
> This is a pre-existing issue, but can this lead to a massive heap buffer
> overflow?
> 
> In read_saved_cmdline(), size is an 8-byte value read directly from the
> untrusted file:
> 
> 	size = read8(pevent);
> 	if (!size)
> 		return 0;
> 
> 	buf = malloc(size + 1);
> 	if (buf == NULL)
> 		return -1;
> 
> 	ret = do_read(buf, size);
> 
> If the size read from the file is ULLONG_MAX (or UINT_MAX on 32-bit systems),
> the size + 1 expression will wrap to 0.
> 
> The malloc(0) call will then succeed and return a small heap chunk, but the
> original unwrapped massive size is passed to do_read(), which will loop and
> read an enormous amount of data directly into the tiny buffer.
> 
Confirmed. will send a v4 with a fix for this.