Re: [PATCH] cpupower: fix use-after-free and stale limits in frequency-set

[Shuah Khan <skhan@linuxfoundation.org>]

On 11/12/26 12:49, Lukas Beckmann wrote:
> cmd_freq_set() currently has two issues:
> 
> 1. use-after-free
> 
> When cmd_freq_set() is invoked with --max and --min, it eventually calls
> do_new_policy() for every selected CPU with the same cpufreq_policy
> struct. do_new_policy() first retrieves the current policy and copies
> the governor pointer to new_pol.governor. After that the policy is
> applied and cur_pol (including the string to which new_pol.governor is
> pointing) is freed. For the next CPU, cpufreq_set_policy() reads that
> already freed memory leading to an error message (because verify_gov()
> fails).
> 

> 2. max/min is not retrieved for each individual CPU:
> 
> If cmd_freq_set() is invoked with e.g. --min and --governor, max is set
> by do_new_policy() on the first selected CPU by retrieving it from the
> current policy. Since the same struct is passed to the next selected
> CPU, it is not retrieved again, resulting in writing the first CPU's
> max to all selected CPUs.
> 
> Fix both issues by using a local copy in do_new_policy().

Good find. The change log is rather long and confusing to follow.

> 
> Fixes: 7fe2f6399a84 ("cpupowerutils - cpufrequtils extended with quite some features")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lukas Beckmann <lbckmnn@mailbox.org>
> ---
> Testing:
> 
> Issue 1:
> ASAN run of the (unpatched) cpupower example:
> 
> ./cpupower frequency-set -d 2500MHz -u 2500MHz
> Setting cpu: 0
> Setting cpu: 1
> =================================================================
> ==386949==ERROR: AddressSanitizer: heap-use-after-free on address 0x7c17340e0350 at pc 0x7ff73559241c bp 0x7fff5d8114a0 sp 0x7fff5d810c58
> READ of size 2 at 0x7c17340e0350 thread T0
>      #0 0x7ff73559241b  (/usr/lib/libasan.so.8+0x8841b) (BuildId: b8a4241051a1621937fdc46e867ba7ecb56d96ea)
>      #1 0x7ff7353cb475 in verify_gov lib/cpufreq.c:585
>      #2 0x7ff7353cc0dd in cpufreq_set_policy lib/cpufreq.c:631
>      #3 0x55f590fb79ab in do_new_policy utils/cpufreq-set.c:163
>      #4 0x55f590fb79ab in do_one_cpu utils/cpufreq-set.c:192
>      #5 0x55f590fb79ab in cmd_freq_set utils/cpufreq-set.c:327
>      #6 0x55f590fb3ee6 in main utils/cpupower.c:235
>      ...
> 
> 0x7c17340e0350 is located 0 bytes inside of 11-byte region [0x7c17340e0350,0x7c17340e035b)
> freed by thread T0 here:
>      #0 0x7ff735634e91  (/usr/lib/libasan.so.8+0x12ae91) (BuildId: b8a4241051a1621937fdc46e867ba7ecb56d96ea)
>      #1 0x7ff7353cb8ef in cpufreq_put_policy lib/cpufreq.c:283
>      #2 0x55f590fb79b6 in do_new_policy utils/cpufreq-set.c:165
>      #3 0x55f590fb79b6 in do_one_cpu utils/cpufreq-set.c:192
>      #4 0x55f590fb79b6 in cmd_freq_set utils/cpufreq-set.c:327
> 
> previously allocated by thread T0 here:
>      #0 0x7ff73562efce in strdup (/usr/lib/libasan.so.8+0x124fce) (BuildId: b8a4241051a1621937fdc46e867ba7ecb56d96ea)
>      #1 0x7ff7353cb1c8 in sysfs_cpufreq_get_one_string lib/cpufreq.c:156
>      #2 0x7ff7353cb845 in cpufreq_get_policy lib/cpufreq.c:262
>      #3 0x55f590fb7a4f in do_new_policy utils/cpufreq-set.c:146
>      #4 0x55f590fb7a4f in do_one_cpu utils/cpufreq-set.c:192
>      #5 0x55f590fb7a4f in cmd_freq_set utils/cpufreq-set.c:327
> 
> Running ASAN with the patch shows only an unrelated memory leak which
> was already reported in [1]. I can fix that in another patch if needed.
> 
> Issue 2:
> Minimal example without the patch:
> 
>      echo 2000000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>      echo 3000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
>      cat /sys/devices/system/cpu/cpu[0,1]/cpufreq/scaling_max_freq
>      2000000
>      3000000
> 
>      ./cpupower -c 0,1 frequency-set --min 1GHz -g performance
>      Setting cpu: 0
>      Setting cpu: 1
> 
>      cat /sys/devices/system/cpu/cpu[0,1]/cpufreq/scaling_max_freq
>      2000000
>      2000000
> 
> The maximum of CPU1 is set to the one of CPU0.
> 

The real issue here is not that the maximum of CPU1 is set to
the one of CPU0.

The real problem is that stale new_pol is used for all cpus
from do_one_cpu() slow path when more than one policy value
changes.

Please re-write this in your own words in a simplifies way
describing what lead to the problem.


> Minimal example with the patch:
> 
>      echo 2000000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>      echo 3000000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
>      cat /sys/devices/system/cpu/cpu[0,1]/cpufreq/scaling_max_freq
>      2000000
>      3000000
> 
>      ./cpupower -c 0,1 frequency-set --min 1GHz -g performance
>      Setting cpu: 0
>      Setting cpu: 1
> 
>      cat /sys/devices/system/cpu/cpu[0,1]/cpufreq/scaling_max_freq
>      2000000
>      3000000
> 
> The maximum of CPU1 is no longer set to the maximum of CPU0.
> 
> [1] https://lore.kernel.org/all/20250827-power-memoryleak-v1-1-e4baf7b59a41@uniontech.com/

The above patch doesn't make sense. I need more information
to determine, if it is needed.

> ---
>   tools/power/cpupower/utils/cpufreq-set.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c
> index 06cd4b280132..d9f6ca522fe5 100644
> --- a/tools/power/cpupower/utils/cpufreq-set.c
> +++ b/tools/power/cpupower/utils/cpufreq-set.c
> @@ -141,9 +141,10 @@ static unsigned long string_to_frequency(const char *str)
>   	}
>   }
>   
> -static int do_new_policy(unsigned int cpu, struct cpufreq_policy *new_pol)
> +static int do_new_policy(unsigned int cpu, const struct cpufreq_policy *new_pol)
>   {
>   	struct cpufreq_policy *cur_pol = cpufreq_get_policy(cpu);
> +	struct cpufreq_policy pol = *new_pol;

Don't assign new_pol to pol.


>   	int ret;
>   
>   	if (!cur_pol) {
> @@ -151,16 +152,16 @@ static int do_new_policy(unsigned int cpu, struct cpufreq_policy *new_pol)
>   		return -EINVAL;
>   	}
>   
> -	if (!new_pol->min)
> -		new_pol->min = cur_pol->min;
		pol->min = cur_pol->min;

Use new_pol in the if logic using pol as temp - changing
pol to temp_pol

> +	if (!pol.min)
> +		pol.min = cur_pol->min;
>   
> -	if (!new_pol->max)
> -		new_pol->max = cur_pol->max;
> +	if (!pol.max)
> +		pol.max = cur_pol->max;
>   
> -	if (!new_pol->governor)
> -		new_pol->governor = cur_pol->governor;
> +	if (!pol.governor)
> +		pol.governor = cur_pol->governor;
>   
> -	ret = cpufreq_set_policy(cpu, new_pol);
> +	ret = cpufreq_set_policy(cpu, &pol);
>   
>   	cpufreq_put_policy(cur_pol);
>   


thanks,
-- Shuah