Re: [PATCH v4 2/5] 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:

Pre-existing issues:
- [Critical] Integer overflow in read_saved_cmdline() leads to zero-sized allocation and massive out-of-bounds heap write.
- [High] The skip() function still truncates 64-bit sizes on 32-bit architectures, leaving the file pointer misaligned.
--

--- Patch [2]: [PATCH v4 2/5] perf trace-event: Fix integer truncation in do_read() and skip() ---
commit ceab52608d4a89e2c3da800798280df9bb428d4e
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 patch changes do_read(),
__do_read(), and skip() to use size_t for size parameters and ssize_t
for 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
[ ... ]
> @@ -68,10 +68,10 @@ static int do_read(void *data, int size)
>  }
>  
>  /* If it fails, the next read will report it */
> -static void skip(int size)
> +static void skip(size_t size)

[Severity: High]
This isn't a bug introduced by this patch, but does skip() still truncate
64-bit sizes on 32-bit architectures?

Since size_t is 32-bit on 32-bit platforms, passing an unsigned long long
from read8() into skip() can truncate values larger than 4GB.

Could a crafted perf.data file providing a massive header event size cause
skip() to truncate the value, leaving the file pointer misaligned?

>  {
>  	char buf[BUFSIZ];
> -	int r;
> +	size_t r;
[ ... ]
> @@ -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 isn't a bug introduced by this patch, but does this code overflow
size + 1 during memory allocation?

If a crafted perf.data file sets the saved cmdline size to ULLONG_MAX
(or ULONG_MAX on 32-bit architectures):

tools/perf/util/trace-event-read.c:read_saved_cmdline() {
    ...
    size = read8(pevent);
    ...
    buf = malloc(size + 1);
    if (buf == NULL) {
        pr_debug("memory allocation failure\n");
        return -1;
    }

    ret = do_read(buf, size);
    ...
}

The size + 1 calculation overflows to 0, causing malloc(0) to return a
small valid heap chunk.

When the original massive size is then passed to do_read(), could the read()
system call overwrite the heap with out-of-bounds file data?

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