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

sashiko-bot
2026-04-08T18:09:50Z

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