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

Commit Message

chenqiwu Dec. 28, 2019, 12:39 p.m. UTC

From: chenqiwu <[e-mail]>

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 <[e-mail]>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Florian Fainelli Jan. 3, 2020, 12:08 a.m. UTC | #1

On 12/28/19 4:39 AM, [e-mail] wrote:
> From: chenqiwu <[e-mail]>
> 
> 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 <[e-mail]>

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 <[e-mail]>
Tested-by: Florian Fainelli <[e-mail]>

Thank you!

chenqiwu Jan. 3, 2020, 2:35 a.m. UTC | #2

On Thu, Jan 02, 2020 at 04:08:53PM -0800, Florian Fainelli wrote:
> On 12/28/19 4:39 AM, [e-mail] wrote:
> > From: chenqiwu <[e-mail]>
> > 
> > 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 <[e-mail]>
> 
> 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 <[e-mail]>
> Tested-by: Florian Fainelli <[e-mail]>
> 
> Thank you!
> -- 
> Florian
Hi Florian,
I agree your idea, since NULL equals to ((void *)0), return 0 matches
with unsigned int.

Thanks!
Qiwu

Patch

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index 77b0e5d..7c4d60a 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -452,8 +452,15 @@  static bool brcm_avs_is_firmware_loaded(struct private_data *priv)
 
 static unsigned int brcm_avs_cpufreq_get(unsigned int cpu)
 {
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
-	struct private_data *priv = policy->driver_data;
+	struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
+	struct private_data *priv;
+
+	if (!policy)
+		return NULL;
+
+	priv = policy->driver_data;
+	if (!priv || !priv->base)
+		return NULL;
 
 	return brcm_avs_get_frequency(priv->base);
 }