Re: [PATCH] perf header: Fix HEADER_CPU_DOMAIN_INFO feature

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch adds a new field `nr_cpus` to the `HEADER_CPU_DOMAIN_INFO` section in `write_cpu_domain_info()`, but completely fails to update the corresponding read logic in `process_cpu_domain_info()`. As a result, the parser mistakenly consumes `nr_cpus` as the first `cpu` ID, permanently desynchronizing all subsequent reads of variable-length arrays and strings. This fatally breaks the perf.data format reading for this feature.
- [Low] The updated documentation in `perf.data-file-format.txt` erroneously contains literal `+` signs (diff artifacts) and misrepresents the structural layout of the binary format by appending the new fields instead of showing them at the beginning of the header section, contradicting both the serialization code and the commit message.

Pre-existing issues:
- [Critical] Heap out-of-bounds write in `build_cpu_domain_map()` when parsing `/proc/schedstat`. The `cpu` index is extracted and used directly as an array index into the dynamically allocated `cd_map` array without any bounds checking against its allocated size `nr`.
- [Critical] Stack buffer overflow in `build_cpu_domain_map()` when parsing unbounded strings from `/proc/schedstat`. The unbounded `%s` format specifiers write directly into fixed-size stack buffers (`dname` of size 16, `cpumask` of size 4096) without width limits.
--

commit b3021ce087d716ca61ad4fc99fbab6158ead0501
Author: Thomas Richter <tmricht@linux.ibm.com>

perf header: Fix HEADER_CPU_DOMAIN_INFO feature

This commit updates the `HEADER_CPU_DOMAIN_INFO` feature serialization to
include a new `nr_cpus` field and aims to align the documentation with the
binary layout.

> diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt
> index b90cba9168f84..e37f7484487bc 100644
> --- a/tools/perf/Documentation/perf.data-file-format.txt
> +++ b/tools/perf/Documentation/perf.data-file-format.txt

[ ... ]

> @@ -461,7 +461,14 @@ struct domain_info {
>  struct cpu_domain_info {
>  	int cpu;
>  	int nr_domains;
> -	struct domain_info domains[];
> ++	int schedstat_version;
> ++	int max_sched_domains;
> ++	int nr_cpus;
> ++	struct {
> ++	       int cpu;
> ++	       int nr_domains;
> ++	       struct domain_info domains[nr_domains];
> ++	} [nr_cpus]
>  };

[Severity: Low]
Does this documentation update mistakenly contain literal '+' signs from diff
artifacts?

Additionally, does appending the new fields at the end misrepresent the actual
structural layout of the binary format, which writes these values before `cpu`
and `nr_domains`?

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 7db7da090a1e0..770fd21eb1a55 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1836,7 +1836,7 @@ struct cpu_domain_map **build_cpu_domain_map(u32 *schedstat_version, u32 *max_sc

[Severity: Critical]
This is a pre-existing issue, but does `build_cpu_domain_map()` have an
out-of-bounds write when parsing the cpu index from /proc/schedstat?

tools/perf/util/header.c:build_cpu_domain_map() {
...
		} else if (strncmp(line, "cpu", 3) == 0) {
			retval = sscanf(line, "cpu%u %*s", &cpu);
			if (retval == 1) {
				cd_map[cpu] = zalloc(sizeof(*cd_map[cpu]));
...
}

The `cpu` index is extracted and used directly as an array index into the
dynamically allocated `cd_map` array without a bounds check against its
allocated size `nr`. Could this lead to a heap out-of-bounds write?

[Severity: Critical]
This is also a pre-existing issue, but does `build_cpu_domain_map()` overflow
its stack buffers when parsing unbounded strings?

tools/perf/util/header.c:build_cpu_domain_map() {
...
	char dname[DNAME_LEN], cpumask[MAX_NR_CPUS];
...
			if (*schedstat_version >= 17) {
				retval = sscanf(line, "domain%u %s %s %*s", &domain, dname,
						cpumask);
...
}

The `%s` format specifiers in `sscanf` write directly into the fixed-size
`dname` and `cpumask` stack arrays without length limits. Can this lead to a
stack buffer overflow?

>  static int write_cpu_domain_info(struct feat_fd *ff,
>  				 struct evlist *evlist __maybe_unused)
>  {
> -	u32 max_sched_domains = 0, schedstat_version = 0;
> +	u32 max_sched_domains = 0, schedstat_version = 0, nr_cpus = 0;
>  	struct cpu_domain_map **cd_map;

[ ... ]

> @@ -1855,6 +1855,15 @@ static int write_cpu_domain_info(struct feat_fd *ff,
>  	if (ret < 0)
>  		goto out;
>  
> +	for (i = 0; i < nr; i++) {
> +		if (!cd_map[i])
> +			continue;
> +		++nr_cpus;
> +	}
> +	ret = do_write(ff, &nr_cpus, sizeof(u32));

[Severity: Critical]
By adding `nr_cpus` to the payload here, do we need a corresponding update in
`process_cpu_domain_info()`?

Without a matching read, it appears `process_cpu_domain_info()` will consume
`nr_cpus` instead of the first `cpu` ID:

tools/perf/util/header.c:process_cpu_domain_info() {
...
	ret = do_read_u32(ff, &max_sched_domains);
	if (ret)
		return ret;
...
	env->max_sched_domains = max_sched_domains;

	for (i = 0; i < nr; i++) {
		if (do_read_u32(ff, &cpu))
			return -1;
...
}

Will this permanently desynchronize all subsequent reads from the perf.data
file?

> +	if (ret < 0)
> +		goto out;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911094129.2032815-1-tmricht@linux.ibm.com?part=1