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

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

[Tanushree Shah]

Thank you for the review.

On 23/07/26 15:40, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Heap buffer overflow in `read_ftrace_printk` unmasked by `size_t` conversion.
> - [High] Infinite loop in `skip()` leading to Denial of Service.
> --
> 
> --- Patch [2]: [PATCH v2 2/2] perf trace-event: Fix integer truncation in do_read() and skip() ---
> commit b30cf605f4d2ba7957c47c5106a821726355dd7e
> Author: Tanushree Shah <tshah@linux.ibm.com>
> 
> perf trace-event: Fix integer truncation in do_read() and skip()
> 
> This commit modifies do_read(), __do_read(), and skip() to use size_t and
> ssize_t instead of int for sizes and return values. This addresses integer
> truncation of 64-bit sizes, fixing uninitialized memory dumps and out-of-bounds
> reads in the perf tools.
> 
>> 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
>> @@ -51,13 +51,13 @@ static int __do_read(int fd, void *buf, int size)
>>   	return rsize;
>>   }
>>   
>> -static int do_read(void *data, int size)
>> +static ssize_t do_read(void *data, size_t size)
> 
> [Severity: Critical]
> Does changing the size parameter to size_t unmask a heap buffer overflow in
> read_ftrace_printk()?
> 
> If a crafted file supplies 0xFFFFFFFF, it triggers an overflow in
> read_ftrace_printk():
> 
> read_ftrace_printk() {
>          size = read4(pevent);
>          ...
>          buf = malloc(size + 1);
>          if (buf == NULL)
>                  return -1;
> 
>          if (do_read(buf, size) < 0) {
>          ...
> }
> 
> Because size + 1 overflows to 0, malloc(0) returns a minimal heap chunk.
> 
> Previously, passing 0xFFFFFFFF to do_read() truncated the size parameter
> to -1. When this -1 was passed to the read() syscall on 64-bit systems,
> it became ULLONG_MAX. This failed boundary checks against TASK_SIZE_MAX,
> causing the kernel to immediately return -EFAULT and preventing the read.
> 
> With size now being size_t, 0xFFFFFFFF is passed directly to the read()
> syscall as 4294967295ULL. Because this is below the 64-bit TASK_SIZE_MAX
> limit, the kernel will read up to MAX_RW_COUNT of file data into the tiny
> allocated buffer.
> 
> Could this be prevented by adding a bounds check before the allocation?

Thanks for catching this. yes, this is a real heap overflow.

size is attacker-controlled, and size + 1 can overflow to 0,
causing a tiny buffer to be allocated while a huge read is still
performed into it.

Fix: reject, size == UINT_MAX before it's used, so the overflow
can't happen. Will include this in v3.
> 
>>   {
>> -	int r;
>> +	ssize_t r;
>>   
>>   	r = __do_read(input_fd, data, size);
> 
> [ ... ]
> 
>>   /* If it fails, the next read will report it */
>> -static void skip(int size)
>> +static void skip(size_t size)
>>   {
>>   	char buf[BUFSIZ];
>> -	int r;
>> +	size_t r;
>>   
>>   	while (size) {
>>   		r = size > BUFSIZ ? BUFSIZ : size;
>>   		do_read(buf, r);
>>   		size -= r;
>>   	}
>>   }
> 
> [Severity: High]
> Does this change introduce an infinite loop when a maliciously crafted
> perf.data file provides a massive 64-bit size?
> 
> Before the patch, passing ULLONG_MAX was implicitly truncated to -1.
> In the first loop iteration, r = size > BUFSIZ ? BUFSIZ : size evaluated
> -1 > 8192 as false, setting r = -1. Then size -= r set size to 0,
> terminating the loop immediately.
> 
> Now that size is widened to size_t, skip() will continuously subtract
> BUFSIZ from ULLONG_MAX. Because skip() ignores the return value of
> do_read(), does this cause the perf process to spin endlessly even
> after reaching EOF?
> 
Yes, its right that widening size to size_t removes the accidental 
truncation that was previously capping the loop early on bogus input, 
and the missing check on do_read()'s return value turns that into a real 
infinite loop  on a malicious size

Fix: Check do_read()'s return value and break out on failure/EOF instead 
of blindly subtracting the requested length. Will include this in v3.


Thanks
Tanushree Shah

Re: [PATCH v2 1/2] perf trace-event: Fix buffer overflow in read_string()

[Tanushree Shah]

On 23/07/26 15:28, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Error path in trace_report() leads to a double free of tevent->pevent when session initialization fails.
> - [High] Integer truncation in do_read() allows a malicious trace file to cause the parser to process uninitialized heap memory.
> --
> 
> --- Patch [1]: [PATCH v2 1/2] perf trace-event: Fix buffer overflow in read_string() ---
> commit fca4f0cc3071265252215647838a7b30c9c0baaf
> Author: Tanushree Shah <tshah@linux.ibm.com>
> 
> perf trace-event: Fix buffer overflow in read_string()
> 
> This patch fixes a potential stack buffer overflow in read_string() by
> adding a bounds check before writing each byte into the buffer. If the
> string exceeds the buffer size, it logs a debug message and returns NULL,
> matching other error handling paths.
> 
>> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
>> index ecbbb93f01853..afd458cf1387d 100644
>> --- a/tools/perf/util/trace-event-read.c
>> +++ b/tools/perf/util/trace-event-read.c
>> @@ -127,6 +127,11 @@ static char *read_string(void)
>>   			}
>>   		}
>>   
>> +		if (size >= (int)sizeof(buf) - 1) {
>> +			pr_debug("string too long (max %zu bytes)", sizeof(buf) - 1);
>> +			goto out;
> 
> [Severity: High]
> This is a pre-existing issue, but could this error path trigger a double free
> of tevent->pevent?
> 
> If read_string() returns NULL due to the bounds check, the error propagates
> and trace_report() jumps to its error path:
> 
> tools/perf/util/trace-event-read.c:trace_report() {
>      ...
> out:
> 	if (pevent)
> 		trace_event__cleanup(tevent);
>      ...
> }
> 
> trace_event__cleanup() frees t->pevent but leaves the pointer dangling:
> 
> tools/perf/util/trace-event.c:trace_event__cleanup() {
>      ...
> 	tep_free(t->pevent);
>      ...
> }
> 
> Later, during session teardown, perf_session__delete() unconditionally calls
> trace_event__cleanup(&session->tevent), which will attempt to free the same
> pointer again.

Thanks for catching this. trace_report()'s error path and 
perf_session__delete() can both call trace_event__cleanup() on the same 
tevent, with nothing preventing a double-free.

Fix: clear t->pevent to NULL after cleanup, and guard the function to 
no-op if it is already NULL, so a second call is safe.
> 
> [Severity: High]
> This is also a pre-existing issue, but does do_read() suffer from an integer
> truncation issue?
> 
> Functions like read_event_file() allocate memory based on a 64-bit size:
> 
> tools/perf/util/trace-event-read.c:read_event_file() {
>      ...
> 	ret = do_read(buf, size);
> 	if (ret < 0)
> 		goto out;
> 
> 	ret = parse_event_file(pevent, buf, size, sys);
>      ...
> }
> 
> But do_read() takes size as an int:
> 
> tools/perf/util/trace-event-read.c:do_read() {
> static int do_read(void *data, int size)
> ...
> }
> 
> If a maliciously crafted perf.data provides a size larger than 32 bits, the
> size is truncated when passed to do_read(), causing it to read fewer bytes.
> Since read_event_file() only checks if ret < 0 rather than verifying
> ret == size, it will pass the large, mostly uninitialized buffer to
> parse_event_file().
> 
>> +		}
>> +
>>   		buf[size++] = c;
>>   
>>   		if (!c)
> 

Thanks: this is exactly what patch 2 fixes. Widening do_read()'s
size to size_t removes the truncation, so it reads (or fails on)
the full 64-bit size.

do_read() either reads everything or returns -1, so callers only need to 
check ret < 0, no ret == size check needed.

Thanks
Tanushree Shah

Re: [PATCH v2 1/2] perf trace-event: Fix buffer overflow in read_string()

[Tanushree Shah]

On 23/07/26 15:28, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Error path in trace_report() leads to a double free of tevent->pevent when session initialization fails.
> - [High] Integer truncation in do_read() allows a malicious trace file to cause the parser to process uninitialized heap memory.
> --
> 
> --- Patch [1]: [PATCH v2 1/2] perf trace-event: Fix buffer overflow in read_string() ---
> commit fca4f0cc3071265252215647838a7b30c9c0baaf
> Author: Tanushree Shah <tshah@linux.ibm.com>
> 
> perf trace-event: Fix buffer overflow in read_string()
> 
> This patch fixes a potential stack buffer overflow in read_string() by
> adding a bounds check before writing each byte into the buffer. If the
> string exceeds the buffer size, it logs a debug message and returns NULL,
> matching other error handling paths.
> 
>> diff --git a/tools/perf/util/trace-event-read.c b/tools/perf/util/trace-event-read.c
>> index ecbbb93f01853..afd458cf1387d 100644
>> --- a/tools/perf/util/trace-event-read.c
>> +++ b/tools/perf/util/trace-event-read.c
>> @@ -127,6 +127,11 @@ static char *read_string(void)
>>   			}
>>   		}
>>   
>> +		if (size >= (int)sizeof(buf) - 1) {
>> +			pr_debug("string too long (max %zu bytes)", sizeof(buf) - 1);
>> +			goto out;
> 
> [Severity: High]
> This is a pre-existing issue, but could this error path trigger a double free
> of tevent->pevent?
> 
> If read_string() returns NULL due to the bounds check, the error propagates
> and trace_report() jumps to its error path:
> 
> tools/perf/util/trace-event-read.c:trace_report() {
>      ...
> out:
> 	if (pevent)
> 		trace_event__cleanup(tevent);
>      ...
> }
> 
> trace_event__cleanup() frees t->pevent but leaves the pointer dangling:
> 
> tools/perf/util/trace-event.c:trace_event__cleanup() {
>      ...
> 	tep_free(t->pevent);
>      ...
> }
> 
> Later, during session teardown, perf_session__delete() unconditionally calls
> trace_event__cleanup(&session->tevent), which will attempt to free the same
> pointer again.

Thanks for catching this. trace_report()'s error path and 
perf_session__delete() can both call trace_event__cleanup() on the same 
tevent, with nothing preventing a double-free.

Fix: clear t->pevent to NULL after cleanup, and guard the function to 
no-op if it is already NULL, so a second call is safe.
> 
> [Severity: High]
> This is also a pre-existing issue, but does do_read() suffer from an integer
> truncation issue?
> 
> Functions like read_event_file() allocate memory based on a 64-bit size:
> 
> tools/perf/util/trace-event-read.c:read_event_file() {
>      ...
> 	ret = do_read(buf, size);
> 	if (ret < 0)
> 		goto out;
> 
> 	ret = parse_event_file(pevent, buf, size, sys);
>      ...
> }
> 
> But do_read() takes size as an int:
> 
> tools/perf/util/trace-event-read.c:do_read() {
> static int do_read(void *data, int size)
> ...
> }
> 
> If a maliciously crafted perf.data provides a size larger than 32 bits, the
> size is truncated when passed to do_read(), causing it to read fewer bytes.
> Since read_event_file() only checks if ret < 0 rather than verifying
> ret == size, it will pass the large, mostly uninitialized buffer to
> parse_event_file().
> 
>> +		}
>> +
>>   		buf[size++] = c;
>>   
>>   		if (!c)
> 

Thanks: this is exactly what patch 2 fixes. Widening do_read()'s
size to size_t removes the truncation, so it reads (or fails on)
the full 64-bit size.

do_read() either reads everything or returns -1, so callers only need to 
check ret < 0, no ret == size check needed.

Thanks
Tanushree Shah