| Message ID | 1578228650-17157-1-git-send-email-qiwuchen55@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] cpufreq: brcmstb-avs-cpufreq: avoid potential stuck and UAF risk | expand |
On 05-01-20, 20:50, 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 of cpufreq_cpu_put() is > ignored to decrement the kobject reference count back, which may lead > to a potential stuck risk that percpu cpuhp thread deadly waits for > dropping of kobject refcount when percpu cpufreq policy free. > > The call trace of stuck risk could be: > cpufreq_online() //If cpufreq online 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 this can be easily exercised by > attempting to force an unbind of the CPUfreq driver. > > 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..6d2bf5c 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 0; > + Since we always reach here after the cpufreq driver is registered, we may not need to check the policy pointer at all. > + priv = policy->driver_data; > + if (!priv || !priv->base) > + return 0; Can there be a case where priv or priv->base be set to NULL for this driver ? I don't think so and so this may not be required. > > return brcm_avs_get_frequency(priv->base); > } > -- > 1.9.1
On Mon, Jan 06, 2020 at 11:26:37AM +0530, Viresh Kumar wrote: > On 05-01-20, 20:50, 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 of cpufreq_cpu_put() is > > ignored to decrement the kobject reference count back, which may lead > > to a potential stuck risk that percpu cpuhp thread deadly waits for > > dropping of kobject refcount when percpu cpufreq policy free. > > > > The call trace of stuck risk could be: > > cpufreq_online() //If cpufreq online 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 this can be easily exercised by > > attempting to force an unbind of the CPUfreq driver. > > > > 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..6d2bf5c 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 0; > > + > > Since we always reach here after the cpufreq driver is registered, we > may not need to check the policy pointer at all. > > > + priv = policy->driver_data; > > + if (!priv || !priv->base) > > + return 0; > > Can there be a case where priv or priv->base be set to NULL for this > driver ? I don't think so and so this may not be required. > Hi viresh, There could be a case as the description of this patch besides brcm_avs_driver unloads. Since cpufreq_policy_free() will free the mm of cpufreq_policy at the last moment. Thanks! Qiwu > > > > return brcm_avs_get_frequency(priv->base); > > } > > -- > > 1.9.1 > > -- > viresh
On 06-01-20, 15:09, chenqiwu wrote: > There could be a case as the description of this patch besides > brcm_avs_driver unloads. Since cpufreq_policy_free() will free > the mm of cpufreq_policy at the last moment. Ahh, right. Please fix the other "policy" thing I reported and resend the patch then.
On Mon, Jan 06, 2020 at 01:01:09PM +0530, Viresh Kumar wrote: > On 06-01-20, 15:09, chenqiwu wrote: > > There could be a case as the description of this patch besides > > brcm_avs_driver unloads. Since cpufreq_policy_free() will free > > the mm of cpufreq_policy at the last moment. > > Ahh, right. Please fix the other "policy" thing I reported and resend > the patch then. > > -- > viresh Hi, Any progress about this patch? Qiwu
On 1/18/2020 6:25 PM, chenqiwu wrote: > On Mon, Jan 06, 2020 at 01:01:09PM +0530, Viresh Kumar wrote: >> On 06-01-20, 15:09, chenqiwu wrote: >>> There could be a case as the description of this patch besides >>> brcm_avs_driver unloads. Since cpufreq_policy_free() will free >>> the mm of cpufreq_policy at the last moment. >> >> Ahh, right. Please fix the other "policy" thing I reported and resend >> the patch then. >> >> -- >> viresh > Hi, > Any progress about this patch? Viresh gave you some feedback to address, so my understanding is that we should see a v3.
diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c index 77b0e5d..6d2bf5c 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 0; + + priv = policy->driver_data; + if (!priv || !priv->base) + return 0; return brcm_avs_get_frequency(priv->base); }