Re: [PATCH 2/4] perf header: Validate string length before allocating in do_read_string()

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] `do_read_string()` fails to ensure null-termination, leading to out-of-bounds reads and heap buffer overflows.
- [Medium] The newly added size check is trivially bypassable because `ff->size` is unvalidated.
--

commit 1c3491f2bf9f038ae68f7007db4323e57b4d8132
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf header: Validate string length before allocating in do_read_string()

This commit attempts to prevent large memory allocations by verifying the
requested string length against the remaining section size before calling
malloc.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index ca0c9e80345b2..f8acd06ae9235 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -259,6 +259,9 @@ static char *do_read_string(struct feat_fd *ff)
>  	if (do_read_u32(ff, &len))
>  		return NULL;
>  
> +	if (len > ff->size - ff->offset)
> +		return NULL;

Is ff->size validated against the actual physical file size before this check?

If ff->size is initialized directly from the user-controlled section size in
the perf.data headers, couldn't a crafted file provide a maliciously large
section size?

This would allow the bounds check to pass, triggering the large allocation
this code was intended to prevent.

> +
>  	buf = malloc(len);
>  	if (!buf)
>  		return NULL;
>  
>  	if (!__do_read(ff, buf, len)) {
>  		/*
>  		 * strings are padded by zeroes
>  		 * thus the actual strlen of buf
>  		 * may be less than len
>  		 */
>  		return buf;
>  	}

Does this guarantee the returned string is null-terminated?

If a crafted perf.data provides a string without a null byte, or if len is 0,
wouldn't callers invoking strlen(buf) read out-of-bounds into adjacent heap
memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260414204847.293557-1-acme@kernel.org?part=2