linux-kernel - Re: [PATCH] cpufreq: Correct a freq check in cpufreq_set

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55B71050.7070707@intel.com>
Date:	Tue, 28 Jul 2015 13:17:04 +0800
From:	Pan Xinhui <xinhuix.pan@...el.com>
To:	Viresh Kumar <viresh.kumar@...aro.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"rjw@...ysocki.net" <rjw@...ysocki.net>,
	"mnipxh@....com" <mnipxh@....com>,
	"yanmin_zhang@...ux.intel.com" <yanmin_zhang@...ux.intel.com>
Subject: Re: [PATCH] cpufreq: Correct a freq check in cpufreq_set_policy

hi, Viresh
	thanks for your quick reply! :)
On 2015年07月28日 12:41, Viresh Kumar wrote:
> On 28-07-15, 11:34, Pan Xinhui wrote:
>> From: Pan Xinhui <xinhuix.pan@...el.com>
>>
>> This check was originally added by commit 9c9a43ed2734 ("[CPUFREQ]
>> return error when failing to set minfreq").It attempt to return an error
>> on obviously incorrect limits when we echo xxx >.../scaling_max,min_freq
>> Actually we just need check if new_policy->min > new_policy->max.
>> Because at least one of max/min is copied from cpufreq_get_policy().
>>
>> For example, when we echo xxx > .../scaling_min_freq, new_policy is
>> copied from policy in cpufreq_get_policy. new_policy->max is same with
>> policy->max. new_policy->min is set to a new value.
>>
>> Let me explain it in deduction method, first statment in if ():
>> new_policy->min > policy->max
>> policy->max == new_policy->max
>> ==> new_policy->min > new_policy->max
>>
>> second statment in if():
>> new_policy->max < policy->min
>> policy->max < policy->min
>> ==>new_policy->min > new_policy->max (induction method)
>>
>> So we have proved that we only need check if new_policy->min >
>> new_policy->max.
>>
>> After apply this patch, we can also modify ->min and ->max in same time
>> if new freq range is very much different from current freq range. For
>> example, if current freq range is 480000-960000, then we want to set
>> this range to 1120000-2240000, we would fail in the past because
>> new_policy->min > policy->max. As long as the cpufreq range is valid, we
>> has no reason to reject the user. So correct the check.
>>
>> Signed-off-by: Pan Xinhui <xinhuix.pan@...el.com>
> 
> Does this patch depend on the other patch you sent where you are
> trying to update both min/max in the same call to
> cpufreq_set_policy()? If so, they should have been part of the same
> series in proper order, as you have sent them as separate patches.
>

Thanks for pointing out my mistakes. I will send them in a same series with proper order.
Sorry for that.
 
> Now, if we don't consider your first patch at all, then this patch is
> obviously wrong. We need to take care of both the checks.
> 
Agree, we need take care of every checks. BUT, As We have proved, it's equal to check if (new_policy->min > new_policy->max). I don't why it's wrong.
with/without this patch, echo 0 > .../scaling_min_freq has no error. min freq is just set to the limit min freq. I prefer to treat it as a feature. :)
So I don't add new_policy->min < policy->cpuinfo.min_freq || new_policy->max > policy->cpuinfo.max_freq.

We have ->verify callback, no need to worry about that an out-of-limit cpufreq will harm kernel.
This check is just to tell userspace that *the cpufreq you are trying to set is wrong, pls double check.*

thanks
xinhui
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/