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

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

[qiwuchen55]

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

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);
 }
-- 
1.9.1


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

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

[Florian Fainelli]

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

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

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

[chenqiwu]

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

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

[chenqiwu]

On Sat, Dec 28, 2019 at 06:20:25PM +0800, kbuild test robot wrote:
> Hi,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on pm/linux-next]
> [also build test ERROR on v5.5-rc3 next-20191220]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> 
> url:    https://github.com/0day-ci/linux/commits/qiwuchen55-gmail-com/cpufreq-brcmstb-avs-cpufreq-avoid-a-stuck-risk-and-UAF-issue-in-brcm_avs_cpufreq_get/20191228-141943
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> config: arm-allmodconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.5.0 make.cross ARCH=arm 
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All error/warnings (new ones prefixed by >>):
> 
>    In file included from include/linux/pm_qos.h:10:0,
>                     from include/linux/cpufreq.h:16,
>                     from drivers/cpufreq/brcmstb-avs-cpufreq.c:44:
>    drivers/cpufreq/brcmstb-avs-cpufreq.c: In function 'brcm_avs_cpufreq_get':
> >> drivers/cpufreq/brcmstb-avs-cpufreq.c:459:12: error: 'dev' undeclared (first use in this function); did you mean 'sev'?
>       dev_warn(dev, "cpu %d: CPUFreq policy not found\n", cpu);
>                ^
>    include/linux/device.h:1776:12: note: in definition of macro 'dev_warn'
>      _dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__)
>                ^~~
>    drivers/cpufreq/brcmstb-avs-cpufreq.c:459:12: note: each undeclared identifier is reported only once for each function it appears in
>       dev_warn(dev, "cpu %d: CPUFreq policy not found\n", cpu);
>                ^
>    include/linux/device.h:1776:12: note: in definition of macro 'dev_warn'
>      _dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__)
>                ^~~
>    In file included from include/uapi/linux/posix_types.h:5:0,
>                     from include/uapi/linux/types.h:14,
>                     from include/linux/compiler.h:180,
>                     from include/linux/err.h:5,
>                     from include/linux/clk.h:12,
>                     from include/linux/cpufreq.h:11,
>                     from drivers/cpufreq/brcmstb-avs-cpufreq.c:44:
> >> include/linux/stddef.h:8:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
>     #define NULL ((void *)0)
>                  ^
> >> drivers/cpufreq/brcmstb-avs-cpufreq.c:460:10: note: in expansion of macro 'NULL'
>       return NULL;
>              ^~~~
> >> include/linux/stddef.h:8:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
>     #define NULL ((void *)0)
>                  ^
>    drivers/cpufreq/brcmstb-avs-cpufreq.c:465:10: note: in expansion of macro 'NULL'
>       return NULL;
>              ^~~~
> --
>    In file included from include/linux/pm_qos.h:10:0,
>                     from include/linux/cpufreq.h:16,
>                     from drivers//cpufreq/brcmstb-avs-cpufreq.c:44:
>    drivers//cpufreq/brcmstb-avs-cpufreq.c: In function 'brcm_avs_cpufreq_get':
>    drivers//cpufreq/brcmstb-avs-cpufreq.c:459:12: error: 'dev' undeclared (first use in this function); did you mean 'sev'?
>       dev_warn(dev, "cpu %d: CPUFreq policy not found\n", cpu);
>                ^
>    include/linux/device.h:1776:12: note: in definition of macro 'dev_warn'
>      _dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__)
>                ^~~
>    drivers//cpufreq/brcmstb-avs-cpufreq.c:459:12: note: each undeclared identifier is reported only once for each function it appears in
>       dev_warn(dev, "cpu %d: CPUFreq policy not found\n", cpu);
>                ^
>    include/linux/device.h:1776:12: note: in definition of macro 'dev_warn'
>      _dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__)
>                ^~~
>    In file included from include/uapi/linux/posix_types.h:5:0,
>                     from include/uapi/linux/types.h:14,
>                     from include/linux/compiler.h:180,
>                     from include/linux/err.h:5,
>                     from include/linux/clk.h:12,
>                     from include/linux/cpufreq.h:11,
>                     from drivers//cpufreq/brcmstb-avs-cpufreq.c:44:
> >> include/linux/stddef.h:8:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
>     #define NULL ((void *)0)
>                  ^
>    drivers//cpufreq/brcmstb-avs-cpufreq.c:460:10: note: in expansion of macro 'NULL'
>       return NULL;
>              ^~~~
> >> include/linux/stddef.h:8:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
>     #define NULL ((void *)0)
>                  ^
>    drivers//cpufreq/brcmstb-avs-cpufreq.c:465:10: note: in expansion of macro 'NULL'
>       return NULL;
>              ^~~~
> 
> vim +459 drivers/cpufreq/brcmstb-avs-cpufreq.c
> 
>    452	
>    453	static unsigned int brcm_avs_cpufreq_get(unsigned int cpu)
>    454	{
>    455		struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
>    456		struct private_data *priv;
>    457	
>    458		if (!policy) {
>  > 459			dev_warn(dev, "cpu %d: CPUFreq policy not found\n", cpu);
>  > 460			return NULL;
>    461		}
>    462	
>    463		priv = policy->driver_data;
>    464		if (!priv || !priv->base)
>    465			return NULL;
>    466	
>    467		return brcm_avs_get_frequency(priv->base);
>    468	}
>    469	
> 
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

So sorry, I sent the wrong patch file, and resend correct patch which compile ok.

Thanks!
Qiwu

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

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

[qiwuchen55]

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()	//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 want to get cpufreq
policy in cpufreq_notify_transition().

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

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index 77b0e5d..31aa76f 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -452,8 +452,17 @@ 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) {
+		dev_warn(dev, "cpu %d: CPUFreq policy not found\n", cpu);
+		return NULL;
+	}
+
+	priv = policy->driver_data;
+	if (!priv || !priv->base)
+		return NULL;
 
 	return brcm_avs_get_frequency(priv->base);
 }
-- 
1.9.1


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