linux-kernel - Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

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: <575FE64C.9080107@linux.vnet.ibm.com>
Date:	Tue, 14 Jun 2016 16:41:08 +0530
From:	Shreyas B Prabhu <shreyas@...ux.vnet.ibm.com>
To:	benh@....ibm.com, mpe@...erman.id.au
CC:	ego@...ux.vnet.ibm.com, mikey@...ling.org,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	linux-pm@...r.kernel.org,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	linux-kernel@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
	maddy@...ux.vnet.ibm.com,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3
 idle states



On 06/14/2016 03:18 AM, Benjamin Herrenschmidt wrote:
> On Wed, 2016-06-08 at 11:54 -0500, Shreyas B. Prabhu wrote:
>>
>>  /*
>>   * States for dedicated partition case.
>>   */
>> @@ -167,6 +183,8 @@ static int powernv_add_idle_states(void)
>>  	int nr_idle_states = 1; /* Snooze */
>>  	int dt_idle_states;
>>  	u32 *latency_ns, *residency_ns, *flags;
>> +	u64 *psscr_val = NULL;
>> +	const char *names[CPUIDLE_STATE_MAX];
>>  	int i, rc;
>>  
>>  	/* Currently we have snooze statically defined */
>> @@ -199,12 +217,41 @@ static int powernv_add_idle_states(void)
>>  		goto out_free_latency;
>>  	}
>>  
>> +	rc = of_property_read_string_array(power_mgt,
>> +					   "ibm,cpu-idle-state-names", names,
>> +					   dt_idle_states);
> 
> Ok so from this I assume that dt_idle_states is the number of entries,
> which has been checked properly to be < CPUIDLE_STATE_MAX correct ?
> 
> Beause ...
>

While dt_idle_states should not be > CPUIDLE_STATE_MAX, if that were the
case we will end up corrupting memory while updating powernv_states[].
I'll add a WARN_ON for such a case and
handle adding idle states to powernv_states accordingly. Thanks for
pointing this out.

>> +	if (rc < 0) {
>> +		pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n");
>> +		goto out_free_latency;
>> +	}
>> +
>> +	/*
>> +	 * If the idle states use stop instruction, probe for psscr values
>> +	 * which are necessary to specify required stop level.
>> +	 */
>> +	if (flags[0] & (OPAL_PM_STOP_INST_FAST | OPAL_PM_STOP_INST_DEEP)) {
>> +		psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val),
>> +				    GFP_KERNEL);
>> +		rc = of_property_read_u64_array(power_mgt,
>> +						"ibm,cpu-idle-state-psscr",
>> +						psscr_val, dt_idle_states);
> 
> Here, psscr val is only one u64 ... shouldn't you kmalloc sizeof(..) *
> dt_idle_states ?

I'm using kcalloc here since checkpatch script suggested kcalloc over
kzalloc for allocating memory for arrays.
I'll also include a patch to use kcalloc throughout the file for
uniformity in next version. I was originally planning to post that
cleanup separately.

Thanks,
Shreyas