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

Thanks for the review

On 24/07/26 20:27, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but there is a heap buffer overflow in `read_saved_cmdline()` due to an integer overflow when allocating memory for a buffer based on an untrusted `size`.
> --
> 
> --- Patch [5]: [PATCH v3 5/5] perf trace-event: Fix infinite loop in skip() ---
> commit 31b13f3b8b045b8ed6e1e9aaefa3637fac096e0b
> Author: Tanushree Shah <[email protected]>
> 
> perf trace-event: Fix infinite loop in skip()
> 
> This patch fixes an infinite loop in skip() where do_read()'s return value
> was ignored. By checking the return value, the loop now breaks correctly on
> failure or EOF.
> 
>> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
>> index 147a3b95ce06b..529a56c3730fa 100644
>> --- a/tools/perf/util/trace-event-read.c
>> +++ b/tools/perf/util/trace-event-read.c
>> @@ -71,12 +71,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: High]
> This isn't a bug introduced by this patch, but while reviewing this file I
> noticed a potential heap buffer overflow in read_saved_cmdline() due to an
> integer overflow when allocating memory:
> 
> tools/perf/util/trace-event-read.c:read_saved_cmdline() {
>      ...
> 	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 large enough, does size + 1 overflow to 0?
> 
> Could malloc(0) succeed and return a small valid pointer, causing do_read()
> to overflow the buffer with size bytes?
> 
Confirmed, same as noted earlier in this thread ( patch 2 review ).
Will fix it in v4.