Re: [PATCH 08/13] perf header: Sanity check HEADER_GROUP_DESC

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] Zero-length string allocations and missing null-termination lead to an out-of-bounds read in `strcmp`.
--

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

perf header: Sanity check HEADER_GROUP_DESC

This commit adds an upper bound check on nr_groups in process_group_desc()
to harden against malformed perf.data files. It also moves the environment
assignment after validation.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 77035d9b138cb..993e20debd5ca 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c

[ ... ]

> @@ -3132,12 +3133,25 @@ static int process_group_desc(struct feat_fd *ff, void *data __maybe_unused)
>  	if (do_read_u32(ff, &nr_groups))
>  		return -1;
>  
> -	env->nr_groups = nr_groups;
>  	if (!nr_groups) {
>  		pr_debug("group desc not available\n");
>  		return 0;
>  	}
>  
> +	if (nr_groups > MAX_GROUP_DESC) {
> +		pr_err("Invalid HEADER_GROUP_DESC: nr_groups (%u) > %u\n",
> +		       nr_groups, MAX_GROUP_DESC);
> +		return -1;
> +	}
> +
> +	if (ff->size < sizeof(u32) + nr_groups * 3 * sizeof(u32)) {
> +		pr_err("Invalid HEADER_GROUP_DESC: section too small (%zu) for %u groups\n",
> +		       ff->size, nr_groups);
> +		return -1;
> +	}
> +
> +	env->nr_groups = nr_groups;
> +
>  	desc = calloc(nr_groups, sizeof(*desc));
>  	if (!desc)
>  		return -1;

Since this patch hardens the parser against malformed files, should there
also be validation on the strings returned by do_read_string(ff) later in
this function?

If a malformed perf.data file provides a string length of 0,
do_read_string() can call malloc(0), which on systems like glibc
returns a valid 0-byte allocation. Because 0 bytes are requested,
the underlying read succeeds but provides no null terminator. It
also does not appear to enforce null-termination for lengths > 0.

When the code subsequently evaluates the string:

    if (strcmp(desc[i].name, "{anon_group}")) {
        evsel->group_name = desc[i].name;

Can this lead to an out-of-bounds read and crash the perf utility if
desc[i].name lacks a null terminator?

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