Re: [PATCH v2] cpufreq: brcmstb-avs-cpufreq: avoid potential stuck and UAF risk
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: 2020-01-06 05:56:45
Also in:
linux-pm
On 05-01-20, 20:50, qiwuchen55@gmail.com wrote:
quoted hunk ↗ jump to hunk
From: chenqiwu <redacted> 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 <redacted> --- 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
-- viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel