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

[sashiko-bot@kernel.org]

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?

>  {
> -	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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723093958.416650-3-tshah@linux.ibm.com?part=2