Re: [PATCH] cpufreq: brcmstb-avs-cpufreq: avoid a stuck risk and UAF issue in brcm_avs_cpufreq_get()

[chenqiwu <qiwuchen55@gmail.com>]

On Thu, Jan 02, 2020 at 04:08:53PM -0800, Florian Fainelli wrote:
> On 12/28/19 4:39 AM, qiwuchen55@gmail.com wrote:
> > From: chenqiwu <chenqiwu@xiaomi.com>
> > 
> > brcm_avs_cpufreq_get() calls cpufreq_cpu_get() to get cpufreq policy,
> > meanwhile, it also increments the kobject reference count of policy to
> > mark it busy. However, a corresponding call to cpufreq_cpu_put() is
> > ignored to decrement the kobject reference count back, which may lead
> > to a potential stuck risk that cpuhp thread deadly wait for dropping
> > of refcount when cpufreq policy free.
> > 
> > The call trace of stuck risk could be:
> > cpufreq_online()  //If cpufreq initialization failed, goto out_free_policy.
> >     ->cpufreq_policy_free()	//Do cpufreq_policy free.
> >         ->cpufreq_policy_put_kobj()
> >             ->kobject_put()       //Skip if policy kfref count is not 1.
> >                 ->cpufreq_sysfs_release()
> >                     ->complete()  //Complete policy->kobj_unregister.
> >                 ->wait_for_completion() //Wait for policy->kobj_unregister.
> > 
> > A simple way to avoid this stuck risk is use cpufreq_cpu_get_raw()
> > instead of cpufreq_cpu_get(), since brcmstb-avs driver just wants
> > to get cpufreq policy.
> > 
> > What's more, there is a potential UAF issue in cpufreq_notify_transition()
> > that the cpufreq policy of current cpu has been released before using it.
> > So we should make a judgement to avoid it.
> > 
> > Thanks!
> > Qiwu
> > 
> > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> 
> This can be easily exercised by attempting to force an unbind of the
> CPUfreq driver without your patch, we will indeed be stuck in the code
> sequence you indicated, whereas with your patch, we can successfully unbind.
> 
> You might want to make some changes though, since you return NULL from a
> function whose signature for the return type is unsigned int. If nothing
> else returning 0 would make sure you hit that code path:
> 
>         if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
>                 policy->cur = cpufreq_driver->get(policy->cpu);
>                 if (!policy->cur) {
>                         pr_err("%s: ->get() failed\n", __func__);
>                         goto out_exit_policy;
>                 }
> 
> something like this on top of your patch:
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index f4f0d6b4e77c..be559fc4e7c6 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -488,11 +488,11 @@ static unsigned int brcm_avs_cpufreq_get(unsigned
> int cpu)
>         struct private_data *priv;
> 
>         if (!policy)
> -               return NULL;
> +               return 0;
> 
>         priv = policy->driver_data;
>         if (!priv || !priv->base)
> -               return NULL;
> +               return 0;
> 
>         return brcm_avs_get_frequency(priv->base);
>  }
> 
> With that, you can add:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Tested-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Thank you!
> -- 
> Florian
Hi Florian,
I agree your idea, since NULL equals to ((void *)0), return 0 matches
with unsigned int.

Thanks!
Qiwu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel