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

[Lukas Beckmann <lbckmnn@mailbox.org>]

On Fri, Nov 13, 2026 at 04:09:23PM -0700, Shuah Khan wrote:
> 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.
>

Thanks for the review,

I will try to come up with a better changelog for V2 (including your
feedback below).

> > 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.
>
The memory-leak is of course not noticeable by "normal usage".

However, tools like ASAN still report it, so the main benefit of fixing
this would be, that someone who is using e.g. ASAN to find a different
issue, is not side-tracked by this. I will drop this in V2.

> > ---
> >   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
> 

Okay, I will do that in V2.

> > +	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

thanks,
Lukas