Re: [PATCH 5/5] perf arm-spe: Reject zero nr_cpu in metadata to prevent division by zero

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] `arm_spe_print_info` uses unvalidated, attacker-controlled fields to index into static format string arrays, leading to arbitrary memory dereferences and format string vulnerabilities.
- [High] Missing bounds check on user-controlled `hdr_sz` allows `ptr` to advance out of bounds and causes integer underflow in the `per_cpu_sz` calculation, leading to arbitrary heap out-of-bounds reads.
- [High] Missing minimum size validation on `per_cpu_sz` allows crafted inputs to cause heap out-of-bounds reads in downstream functions.
- [High] Missing payload size validation before accessing V2 metadata fields leads to an out-of-bounds read.
- [High] Bypassing the metadata allocation error check via a crafted V1 version flag causes an unhandled NULL pointer dereference in `arm_spe__is_homogeneous`.
--

--- Patch [5]: [PATCH 5/5] perf arm-spe: Reject zero nr_cpu in metadata to prevent division by zero ---
commit cdd9c13905fbd08524e5237d0ba280b139dcb3bc
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Subject: perf arm-spe: Reject zero nr_cpu in metadata to prevent division by zero

This patch adds a check to reject nr_cpu <= 0 in arm_spe__alloc_metadata()
before performing a division by nr_cpu. This prevents a division by zero
when a crafted perf.data file provides a zero value for the CPU count.

> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> index 552f063f126e6..401aab529309c 100644
> --- a/tools/perf/util/arm-spe.c
> +++ b/tools/perf/util/arm-spe.c
> @@ -1604,8 +1604,12 @@ static u64 **arm_spe__alloc_metadata(struct perf_record_auxtrace_info *info,
>  	*ver = ptr[ARM_SPE_HEADER_VERSION];
>  	hdr_sz = ptr[ARM_SPE_HEADER_SIZE];
>  	*nr_cpu = ptr[ARM_SPE_CPUS_NUM];

[Severity: High]
This isn't a bug introduced by this patch, but does this read out of bounds
if the payload is exactly 24 bytes?

In arm_spe_process_auxtrace_info(), the payload is only validated against a
minimum size of 16 bytes for V1 metadata. If a crafted perf.data provides a
payload size of exactly 24 bytes (and is not V1), accessing ptr at index 3
(ARM_SPE_CPUS_NUM) attempts to read 32 bytes into the payload, which goes
beyond the verified payload bounds.

>  
> +	/* nr_cpu is used as a divisor below */
> +	if (*nr_cpu <= 0)
> +		return NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does returning NULL here
trigger a NULL pointer dereference in arm_spe__is_homogeneous() if the
attacker sets the version to 1?

If a crafted file sets ARM_SPE_HEADER_VERSION to 1, and the
arm_spe__alloc_metadata() function returns NULL due to an allocation failure
or a negative nr_cpu, the error check in arm_spe_process_auxtrace_info() is
bypassed:

tools/perf/util/arm-spe.c:arm_spe_process_auxtrace_info() {
    ...
	metadata = arm_spe__alloc_metadata(auxtrace_info, &metadata_ver,
					   &nr_cpu);
	if (!metadata && metadata_ver != 1) {
		pr_err("Failed to parse Arm SPE metadata.\n");
		return -EINVAL;
	}
    ...
	spe->metadata_nr_cpu = nr_cpu;
	spe->is_homogeneous = arm_spe__is_homogeneous(metadata, nr_cpu);
    ...
}

Because metadata_ver == 1, execution proceeds and passes the NULL metadata
pointer to arm_spe__is_homogeneous(), where it dereferences metadata[i]
without checking if metadata itself is NULL.

>  
>  	metadata = calloc(*nr_cpu, sizeof(*metadata));
>  	if (!metadata)
>  		return NULL;
> 
>  	/* Locate the start address of per CPU metadata */
>  	ptr += hdr_sz;
>  	per_cpu_sz = (metadata_size - (hdr_sz * sizeof(u64))) / (*nr_cpu);

[Severity: High]
This is a pre-existing issue, but does this allow ptr to advance out of
bounds if hdr_sz is larger than metadata_size?

Since hdr_sz is controlled by the input and lacks a bounds check against
metadata_size, a large hdr_sz could advance ptr past the payload bounds and
cause an integer underflow in the per_cpu_sz calculation.

[Severity: High]
This is a pre-existing issue, but is there a missing minimum size validation
on per_cpu_sz?

If per_cpu_sz is too small (e.g. 8 bytes), arm_spe__alloc_per_cpu_metadata()
allocates an undersized buffer. Later, arm_spe__is_homogeneous()
unconditionally accesses fields at fixed indices like ARM_SPE_CPU_MIDR,
which would result in out-of-bounds heap reads:

tools/perf/util/arm-spe.c:arm_spe__is_homogeneous() {
    ...
		if (i == 0) {
			midr = metadata[i][ARM_SPE_CPU_MIDR];
			continue;
		}

		if (midr != metadata[i][ARM_SPE_CPU_MIDR])
			return false;
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does arm_spe_print_info() use the
unvalidated hdr_size to index into static format string arrays?

If a crafted file provides a large hdr_size or cpu_size, these unchecked
values are used as loop bounds, leading to out-of-bounds reads of format
string pointers which are then passed to fprintf():

tools/perf/util/arm-spe.c:arm_spe_print_info() {
    ...
	for (i = 0; i < hdr_size; i++)
		fprintf(stdout, hdr_fmts[i], arr[i]);
    ...
		cpu_size = (ARM_SPE_CPU_NR_PARAMS + 1) + arr[ARM_SPE_CPU_NR_PARAMS];
		for (i = 0; i < cpu_size; i++)
			fprintf(stdout, metadata_per_cpu_fmts[i], arr[i]);
    ...
}

Could this lead to an arbitrary memory read or format string vulnerability
when running perf report with dump_trace?

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