Re: [PATCH] cpupower: Avoid uninitialized reads in topology sorting

Re: [PATCH] cpupower: Avoid uninitialized reads in topology sorting

[Shuah]

On 7/29/26 20:07, Ali Ahmet Memis wrote:
> 
> get_cpu_topology() allocates core_info with malloc(). If a topology
> attribute disappears while CPUs are being hotplugged, an error path can
> leave core_cpu_list uninitialized. __compare_core_cpu_list() then passes
> the field to strcmp() while sorting the array.
> 
> Use calloc() for the array and ignore entries without complete topology
> data when counting cores. Besides avoiding the invalid read, this keeps an
> incomplete entry from being counted as a physical core.
> 
> Fixes: f89cb9cba7a2 ("cpupower: Implement CPU physical core querying")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> Tested with a two-CPU topology mock that makes the second CPU's sysfs
> reads fail. Valgrind reports uninitialized reads before this patch and
> no errors after it.
> 
> Build-tested with:
>    make -C tools/power/cpupower NLS=false CPUFREQ_BENCH=false

Sorry I am not taking this patch.

Please note that build test just verifies that the code build which
fall way short of testing the changed code.

thanks,
-- Shuah

Re: [PATCH] cpupower: Avoid uninitialized reads in topology sorting

[Ali Ahmet Memis]

On Mon, Aug 03, 2026 at 10:29:15AM -0600, Shuah wrote:
> Sorry I am not taking this patch.
>
> Please note that build test just verifies that the code build which
> fall way short of testing the changed code.

First, sorry about the duplicates. The mail provider I was using at the
time rewrote the Message-ID and delivered the same patch several times, so
it hit the list as a handful of separate threads. That was not me resending
it. I have moved off that provider and everything since August 1 goes out
with a stable Message-ID from git send-email.

The valgrind run was in the notes under the scissors, not just a build: a
two-CPU mock where CPU1's topology reads fail, uninitialized reads in
__compare_core_cpu_list() before the patch and none after. If there is
testing you would want beyond that, tell me what would satisfy you and I
will do it. Otherwise I am fine dropping this.

Re: [PATCH] cpupower: Avoid uninitialized reads in topology sorting

[Shuah]

On 8/3/26 10:36, Ali Ahmet Memis wrote:
> On Mon, Aug 03, 2026 at 10:29:15AM -0600, Shuah wrote:
>> Sorry I am not taking this patch.
>>
>> Please note that build test just verifies that the code build which
>> fall way short of testing the changed code.
> 
> First, sorry about the duplicates. The mail provider I was using at the
> time rewrote the Message-ID and delivered the same patch several times, so
> it hit the list as a handful of separate threads. That was not me resending
> it. I have moved off that provider and everything since August 1 goes out
> with a stable Message-ID from git send-email.
> 

I noticed the duplicates. I am glad you switched to git send-email.

> The valgrind run was in the notes under the scissors, not just a build: a
> two-CPU mock where CPU1's topology reads fail, uninitialized reads in
> __compare_core_cpu_list() before the patch and none after. If there is
> testing you would want beyond that, tell me what would satisfy you and I
> will do it. Otherwise I am fine dropping this.

I missed that. There is no need to say the patch was build-tested since
you did mock testing.

This patch is doing more than replacing malloc() with calloc(). Please
split the changes into separate patches.

thanks,
-- SHuah

[PATCH] cpupower: Avoid uninitialized reads in topology sorting

[Ali Ahmet Memis]

get_cpu_topology() allocates core_info with malloc(). If a topology
attribute disappears while CPUs are being hotplugged, an error path can
leave core_cpu_list uninitialized. __compare_core_cpu_list() then passes
the field to strcmp() while sorting the array.

Use calloc() for the array and ignore entries without complete topology
data when counting cores. Besides avoiding the invalid read, this keeps an
incomplete entry from being counted as a physical core.

Fixes: f89cb9cba7a2 ("cpupower: Implement CPU physical core querying")
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
Tested with a two-CPU topology mock that makes the second CPU's sysfs
reads fail. Valgrind reports uninitialized reads before this patch and
no errors after it.

Build-tested with:
  make -C tools/power/cpupower NLS=false CPUFREQ_BENCH=false

 tools/power/cpupower/lib/cpupower.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/power/cpupower/lib/cpupower.c b/tools/power/cpupower/lib/cpupower.c
index d7f7ec6f1..a8ee304bd 100644
--- a/tools/power/cpupower/lib/cpupower.c
+++ b/tools/power/cpupower/lib/cpupower.c
@@ -171,7 +171,7 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 	char path[SYSFS_PATH_MAX];
 	char *last_cpu_list;
 
-	cpu_top->core_info = malloc(sizeof(struct cpuid_core_info) * cpus);
+	cpu_top->core_info = calloc(cpus, sizeof(struct cpuid_core_info));
 	if (cpu_top->core_info == NULL)
 		return -ENOMEM;
 	cpu_top->pkgs = cpu_top->cores = 0;
@@ -214,11 +214,17 @@ int get_cpu_topology(struct cpupower_topology *cpu_top)
 	qsort(cpu_top->core_info, cpus, sizeof(struct cpuid_core_info),
 	      __compare_core_cpu_list);
 
-	last_cpu_list = cpu_top->core_info[0].core_cpu_list;
-	cpu_top->cores = 1;
-	for (cpu = 1; cpu < cpus; cpu++) {
-		if (strcmp(cpu_top->core_info[cpu].core_cpu_list, last_cpu_list) != 0 &&
-		    cpu_top->core_info[cpu].pkg != -1) {
+	last_cpu_list = NULL;
+	cpu_top->cores = 0;
+	for (cpu = 0; cpu < cpus; cpu++) {
+		if (cpu_top->core_info[cpu].pkg == -1 ||
+		    cpu_top->core_info[cpu].core == -1 ||
+		    cpu_top->core_info[cpu].core_cpu_list[0] == '\0')
+			continue;
+
+		if (!last_cpu_list ||
+		    strcmp(cpu_top->core_info[cpu].core_cpu_list,
+			   last_cpu_list) != 0) {
 			last_cpu_list = cpu_top->core_info[cpu].core_cpu_list;
 			cpu_top->cores++;
 		}
-- 
2.54.0