[PATCH] perf header: Fix HEADER_CPU_DOMAIN_INFO feature

[PATCH] perf header: Fix HEADER_CPU_DOMAIN_INFO feature

[Thomas Richter]

commit d40c68a49f69 ("perf header: Support CPU DOMAIN relation info")
introduced the HEADER_CPU_DOMAIN_INFO feature and the necessary data
into the perf.data file.

The documentation differs from the data actually written by
write_cpu_domain_info(). The first two values written to the header
section are schedstat_version and max_sched_domains. They are missing
in the documentation. Add them to the documentation.

Also structure cpu_domain_info actually contains an array of
struct domain_info. It is very important for the reader to know
how many array elements have been written to that file section,
especially when the member domain_info::dname is optional and
only present when schedstat_version >= 17.
Add the number of CPUs which have been saved into that file section
so the reader knows in advance how many array elements to read.
This is also in sync with other perf.data file sections which always
have the number of elements before the array data.

Fixes: d40c68a49f69 ("perf header: Support CPU DOMAIN relation info")
Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
 # Cc: Swapnil Sapkal <swapnil.sapkal@amd.com>
---
 tools/perf/Documentation/perf.data-file-format.txt | 11 +++++++++--
 tools/perf/util/header.c                           | 11 ++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt
index b90cba9168f8..e37f7484487b 100644
--- a/tools/perf/Documentation/perf.data-file-format.txt
+++ b/tools/perf/Documentation/perf.data-file-format.txt
@@ -453,7 +453,7 @@ List of cpu-domain relation info. The format of the data is as below.
 
 struct domain_info {
 	int domain;
-	char dname[];
+	char dname[];	if schedstat_version >= 17
 	char cpumask[];
 	char cpulist[];
 };
@@ -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]
 };
 
 	HEADER_E_MACHINE = 33,
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 7db7da090a1e..770fd21eb1a5 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
 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;
 	u32 i, j, nr, ret;
 
@@ -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));
+	if (ret < 0)
+		goto out;
+
 	for (i = 0; i < nr; i++) {
 		if (!cd_map[i])
 			continue;
-- 
2.55.0

Re: [PATCH] perf header: Fix HEADER_CPU_DOMAIN_INFO feature

[sashiko-bot]

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