Re: [PATCH 07/13] perf header: Sanity check HEADER_PMU_MAPPINGS

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] State corruption on error paths leads to a guaranteed NULL pointer dereference when parsing malformed files.
- [High] Missing null-termination check on strings read from the file leads to an out-of-bounds read in `strbuf_addf`.
--

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

perf header: Sanity check HEADER_PMU_MAPPINGS

This commit adds an upper bound check on pmu_num and a section size check
in process_pmu_mappings() to harden against malformed 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
> @@ -3069,6 +3070,18 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
>  		return 0;
>  	}
>  
> +	if (pmu_num > MAX_PMU_MAPPINGS) {
> +		pr_err("Invalid HEADER_PMU_MAPPINGS: pmu_num (%u) > %u\n",
> +		       pmu_num, MAX_PMU_MAPPINGS);
> +		return -1;
> +	}
> +
> +	if (ff->size < sizeof(u32) + pmu_num * 2 * sizeof(u32)) {
> +		pr_err("Invalid HEADER_PMU_MAPPINGS: section too small (%zu) for %u PMUs\n",
> +		       ff->size, pmu_num);
> +		return -1;
> +	}
> +
>  	env->nr_pmu_mappings = pmu_num;
>  	if (strbuf_init(&sb, 128) < 0)
>  		return -1;

This isn't a bug introduced by this patch, but if strbuf_init() fails or if
an error occurs in the while loop, this function returns -1 but leaves
env->nr_pmu_mappings set to pmu_num. 

Because the caller perf_session__read_header() silently ignores the -1
return value from section processing, the session initialization succeeds.

Later, functions like perf_env__has_pmu_mapping() will iterate
env->nr_pmu_mappings times over the NULL env->pmu_mappings pointer:

bool perf_env__has_pmu_mapping(struct perf_env *env, const char *pmu_name)
{
	char *pmu_mapping = env->pmu_mappings, *colon;

	for (int i = 0; i < env->nr_pmu_mappings; ++i) {
		if (strtoul(pmu_mapping, &colon, 0) == ULONG_MAX || *colon != ':')
			goto out_error;

Could this lead to a NULL pointer dereference when passing pmu_mapping to
strtoul()? Should the assignment to env->nr_pmu_mappings be deferred
until env->pmu_mappings is successfully constructed?

>  	while (pmu_num) {
>  		if (do_read_u32(ff, &type))
>  			goto error;
>  
>  		name = do_read_string(ff);
>  		if (!name)
>  			goto error;
>  
>  		if (strbuf_addf(&sb, "%u:%s", type, name) < 0)

This isn't a bug introduced by this patch, but does do_read_string() ensure
null-termination of the read string?

If a malformed perf.data file provides a string exactly equal to len without
a trailing null byte, passing the buffer to strbuf_addf() could cause
vsnprintf to read past the end of the allocated buffer.

Could this lead to a heap out-of-bounds read?

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