Re: [PATCH 04/13] perf header: Sanity check HEADER_CPU_TOPOLOGY

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Regression: Freeing `env->cpu` on error paths creates a state inconsistency leading to a guaranteed NULL pointer dereference.
- [High] Pre-existing: Heap out-of-bounds read and information leak due to missing null-termination guarantees in `do_read_string()`.
--

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

perf header: Sanity check HEADER_CPU_TOPOLOGY

This commit adds validation to process_cpu_topology() to harden against
malformed perf.data files. It bounds checks sibling counts and changes bare
returns to goto statements that free the env->cpu structure.

> 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

[ ... ]

> @@ -2887,7 +2898,13 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
>  	env->sibling_cores = strbuf_detach(&sb, NULL);
>  
>  	if (do_read_u32(ff, &nr))
> -		return -1;
> +		goto free_cpu;
> +
> +	if (nr > (u32)cpu_nr) {
> +		pr_err("Invalid HEADER_CPU_TOPOLOGY: nr_sibling_threads (%u) > nr_cpus_avail (%d)\n",
> +		       nr, cpu_nr);
> +		goto free_cpu;
> +	}

Does freeing env->cpu on error paths here create a state inconsistency?

If perf_session__read_header() ignores the return value of
perf_header__process_sections() and proceeds, env->nr_cpus_avail will still
be greater than 0, but env->cpu will be NULL because it was freed.

Later consumers like perf_env__get_socket_aggr_by_cpu() might blindly
dereference env->cpu based on nr_cpus_avail being greater than zero:

perf_env__get_socket_aggr_by_cpu()
    env->cpu[cpu.cpu].socket_id

Before this change, the partially initialized array was left intact and
safely zero-initialized, which avoided a null pointer dereference. 

The memory leak this attempts to address appears to be safely handled already,
as perf_env__exit() properly frees env->cpu during session cleanup. Could
these new error paths just return directly to avoid the crash?

A check of subsequent commits in this series indicates this resulting state
inconsistency remains unaddressed.

[ ... ]

> @@ -2936,7 +2953,13 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
>  		return 0;
>  
>  	if (do_read_u32(ff, &nr))
> -		return -1;
> +		goto free_cpu;
> +
> +	if (nr > (u32)cpu_nr) {
> +		pr_err("Invalid HEADER_CPU_TOPOLOGY: nr_sibling_dies (%u) > nr_cpus_avail (%d)\n",
> +		       nr, cpu_nr);
> +		goto free_cpu;
> +	}

This isn't a regression from this patch, but looking at the surrounding code
that processes these sibling strings:

str = do_read_string(ff);
if (!str)
    goto error;

if (strbuf_add(&sb, str, strlen(str) + 1) < 0)

Is it possible for do_read_string() to return a string without a null
terminator? 

If a malformed file provides a string without a null byte, could strlen()
read past the end of the heap allocation until it finds one, potentially
leaking heap data when the topology information is dumped?

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