Re: [PATCH 5/8] perf header: Use a max number of command line args

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Unvalidated `ff->size` and unenforced string lengths lead to heap buffer overflow.
- [High] Missing null-termination validation causes out-of-bounds reads.
--

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

perf header: Use a max number of command line args

This commit sets a reasonable maximum number of command line arguments to
avoid overflows when reading perf.data files.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
[ ... ]
> @@ -2804,6 +2807,9 @@ static int process_cmdline(struct feat_fd *ff, void *data __maybe_unused)
>  	if (do_read_u32(ff, &nr))
>  		return -1;
>  
> +	if (nr > MAX_CMDLINE_NR)
> +		return -1;
> +
>  	env->nr_cmdline = nr;
>  
>  	cmdline = zalloc(ff->size + nr + 1);

This problem wasn't introduced by this patch, but is there a potential integer
overflow here?

Because ff->size is attacker-controlled data from the perf.data file header
and is not validated, could an attacker set ff->size near the maximum value
to cause an integer overflow, resulting in zalloc allocating a tiny buffer?

Alternatively, even without an integer overflow, could an attacker set a small
ff->size but embed large strings in the file, bypassing validation if
do_read_string() does not enforce that the read bytes fit within the declared
ff->size?

This would cause the subsequent memcpy loop to write beyond the allocated
cmdline buffer, leading to a heap buffer overflow.

>  	if (!cmdline)
>  		return -1;
>  
>  	argv = zalloc(sizeof(char *) * (nr + 1));
>  	if (!argv)
>  		goto error;
>  
>  	for (i = 0; i < nr; i++) {
>  		str = do_read_string(ff);
>  		if (!str)
>  			goto error;
>  
>  		argv[i] = cmdline + len;
>  		memcpy(argv[i], str, strlen(str) + 1);

This issue was also not introduced by this patch, but does this code cause an
out-of-bounds read if the string is not null-terminated?

If do_read_string() allocates memory based on a length prefix and reads data
directly from the file without explicitly appending a null terminator, could a
malicious perf.data file provide a string length but fill the content entirely
with non-null bytes?

This would cause strlen(str) to scan past the end of the str allocation until
it encounters a null byte in adjacent heap memory.

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