* [PATCH] cpufreq: brcmstb-avs-cpufreq: avoid a stuck risk and UAF issue in brcm_avs_cpufreq_get()
@ 2019-12-28 6:15 qiwuchen55
[not found] ` <201912281807.gbP6xHJ1%lkp@intel.com>
0 siblings, 1 reply; 5+ messages in thread
From: qiwuchen55 @ 2019-12-28 6:15 UTC (permalink / raw)
To: mmayer, rjw, viresh.kumar, f.fainelli
Cc: chenqiwu, bcm-kernel-feedback-list, linux-kernel,
linux-arm-kernel, linux-pm
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] cpufreq: brcmstb-avs-cpufreq: avoid a stuck risk and UAF issue in brcm_avs_cpufreq_get()
@ 2019-12-28 12:39 qiwuchen55
2020-01-03 0:08 ` Florian Fainelli
0 siblings, 1 reply; 5+ messages in thread
From: qiwuchen55 @ 2019-12-28 12:39 UTC (permalink / raw)
To: mmayer, rjw, viresh.kumar, f.fainelli
Cc: chenqiwu, bcm-kernel-feedback-list, linux-arm-kernel, linux-pm
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] cpufreq: brcmstb-avs-cpufreq: avoid a stuck risk and UAF issue in brcm_avs_cpufreq_get()
2019-12-28 12:39 qiwuchen55
@ 2020-01-03 0:08 ` Florian Fainelli
2020-01-03 2:35 ` chenqiwu
0 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2020-01-03 0:08 UTC (permalink / raw)
To: qiwuchen55, mmayer, rjw, viresh.kumar, f.fainelli
Cc: chenqiwu, bcm-kernel-feedback-list, linux-arm-kernel, linux-pm
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] cpufreq: brcmstb-avs-cpufreq: avoid a stuck risk and UAF issue in brcm_avs_cpufreq_get()
2020-01-03 0:08 ` Florian Fainelli
@ 2020-01-03 2:35 ` chenqiwu
0 siblings, 0 replies; 5+ messages in thread
From: chenqiwu @ 2020-01-03 2:35 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-pm, viresh.kumar, bcm-kernel-feedback-list, mmayer,
chenqiwu, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-03 2:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-12-28 6:15 [PATCH] cpufreq: brcmstb-avs-cpufreq: avoid a stuck risk and UAF issue in brcm_avs_cpufreq_get() qiwuchen55
[not found] ` <201912281807.gbP6xHJ1%lkp@intel.com>
2019-12-28 12:47 ` chenqiwu
-- strict thread matches above, loose matches on Subject: below --
2019-12-28 12:39 qiwuchen55
2020-01-03 0:08 ` Florian Fainelli
2020-01-03 2:35 ` chenqiwu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox