linux-kernel - Re: [PATCH] PM / devfreq: Fix out of bounds access of transition table array

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] [day] [month] [year] [list]

Message-ID: <530E8A99.50402@codeaurora.org>
Date:	Wed, 26 Feb 2014 16:45:13 -0800
From:	Saravana Kannan <skannan@...eaurora.org>
To:	myungjoo.ham@...sung.com
CC:	박경민 <kyungmin.park@...sung.com>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] PM / devfreq: Fix out of bounds access of transition
 table array

On 02/25/2014 09:12 PM, 함명주 wrote:
>> On 02/23/2014 11:15 PM, Saravana Kannan wrote:
>>> The previous_freq value for a device could be an invalid frequency that
>>> results in a error value being returned from devfreq_get_freq_level().
>>> Check for an error value before using that to index into the transition
>>> table.
>>>
>>> Not doing this check will result in memory corruption when previous_freq is
>>> not a valid frequency.
>>>
>>> Signed-off-by: Saravana Kannan <skannan@...eaurora.org>
>>
>> MyungJoo/Kyungmin,
>>
>> Would either of you have some time to respond to this?
>>
>> Thanks,
>> Saravana
> 
> Dear Saravana,
> 
>>> +	prev_lev = devfreq_get_freq_level(devfreq, devfreq->previous_freq);
>>> +	if (prev_lev < 0)
>>> +		return 0;
> 
> If devfreq_get_freq_level returned error, please return that error
> to the caller. You are retuning 0 in that case.
> 
> Plus, do you think we are going to change profile->freq_table in run-time?
> (by accidently? or intentionally?)

Hi MyungJoo,

The reason for this fix is that some devices can't reliably detect their
current frequency. So, we set the initial freq to 0 so that the first
request from the default governor actually goes through.

If we randomly pick one of the valid frequencies, then the initial
request could get short circuited by devfreq, cause wrong stats, the
device itself might short circuit it after comparing it to prev freq, etc.

The reason I intentionally ignored the error was to prevent printing an
error at boot every time for such devices. I could do better with more
complicated code, but I couldn't imagine any other case where
previous_freq could actually end up being an invalid value.

Hold on, I just realized that the stats code is completely wrong! Isn't
the current code updating the time_in_state[] of the wrong level? It's
updating the new level's time in state instead of the previous one?

I'll send out a patch that fixes all this with in a couple of days.

Thanks,
Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
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/