lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 18 Feb 2016 17:20:52 +0100 From: "Rafael J. Wysocki" <rafael@...nel.org> To: Viresh Kumar <viresh.kumar@...aro.org> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM list <linux-pm@...r.kernel.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org> Subject: Re: [PATCH 1/12] cpufreq: governor: Close dbs_data update race condition On Thu, Feb 18, 2016 at 6:24 AM, Viresh Kumar <viresh.kumar@...aro.org> wrote: > On 18-02-16, 02:19, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com> >> >> It is possible for a dbs_data object to be updated after its >> usage counter has become 0. That may happen if governor_store() >> runs (via a govenor tunable sysfs attribute write) in parallel >> with cpufreq_governor_exit() called for the last cpufreq policy >> associated with the dbs_data in question. In that case, if >> governor_store() acquires dbs_data->mutex right after >> cpufreq_governor_exit() has released it, the ->store() callback >> invoked by it may operate on dbs_data with no users. Although >> sysfs will cause the kobject_put() in cpufreq_governor_exit() to >> block until governor_store() has returned, that situation may >> lead to some unexpected results, depending on the implementation >> of the ->store callback, and therefore it should be avoided. >> >> To that end, modify governor_store() to check the dbs_data's >> usage count before invoking the ->store() callback and return >> an error if it is 0 at that point. >> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com> >> --- >> drivers/cpufreq/cpufreq_governor.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c >> =================================================================== >> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c >> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c >> @@ -112,7 +112,7 @@ static ssize_t governor_store(struct kob >> >> mutex_lock(&dbs_data->mutex); >> >> - if (gattr->store) >> + if (dbs_data->usage_count && gattr->store) > > That's not gonna be enough. The above lock doesn't guarantee > protection with any such races. I'm not really sure what you're talking about to be honest, so please be more specific. You can say "For example, function X decrements the usage count without locking" or similar. Such vague comments are quite difficult to address, especially if they don't hold any water. :-) > And so usage_count can become zero > just after this check. But how? The only place it is decremented is cpufreq_governor_exit() and there it is done under dbs_data->mutex (at my direct request, BTW). So we are guaranteed that it won't go down to zero while we're holding dbs_data->mutex, aren't we? > Btw, we should also kill the gattr->store checks here as well, as we > did it in cpufreq-core. > >> ret = gattr->store(dbs_data, buf, count); >> >> mutex_unlock(&dbs_data->mutex); Yeah, they are quite useless. But not in this patch. Thanks, Rafael
Powered by blists - more mailing lists