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

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Incomplete Hardening / Brittle State Mutation leading to NULL Pointer Dereference
- [High] Information Leak / Out-of-Bounds Read in `do_read_string()`
--

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

perf header: Sanity check HEADER_PMU_MAPPINGS

Add upper bound check on pmu_num in process_pmu_mappings() to harden
against malformed perf.data files (max 4096).

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 45f533fbb8fdc..1d7ca467acf32 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -3067,6 +3067,19 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
>  		return 0;
>  	}
>  
> +#define MAX_PMU_MAPPINGS 4096
> +	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 regression introduced by this patch, but since the goal is to
harden process_pmu_mappings() against malformed files, does this leave a crash
condition if parsing fails later?

If an error occurs after env->nr_pmu_mappings is set:

	while (pmu_num) {
		if (do_read_u32(ff, &type))
			goto error;

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

the function jumps to the error label and returns -1.

If the caller perf_session__read_header() ignores the error, we are left with
env->nr_pmu_mappings > 0 but env->pmu_mappings as NULL.

Could this lead to a NULL pointer dereference in functions like
perf_env__has_pmu_mapping() that iterate up to env->nr_pmu_mappings and pass
env->pmu_mappings directly to strtoul()? Should env->nr_pmu_mappings only be
updated on the success path?

This is also not a regression introduced by this patch, but does
do_read_string() ensure the returned string is null-terminated?

		if (strbuf_addf(&sb, "%u:%s", type, name) < 0)
			goto error;

If a malformed perf.data file provides a string without a null terminator,
and do_read_string() returns a buffer that isn't null-terminated, could the
%s format specifier in strbuf_addf() read past the end of the allocation
until it hits a null byte in adjacent heap memory?

Could this lead to an out-of-bounds heap read, potentially crashing perf or
leaking heap data into the PMU mappings output?

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