Message-ID: <4F7B0387.6050804@linux.vnet.ibm.com>
Date:	Tue, 03 Apr 2012 19:34:55 +0530
From:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>
CC:	lenb@...nel.org, khilman@...com, deepthi@...ux.vnet.ibm.com,
	g.trinabh@...il.com, arjan@...radead.org,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	amit.kucheria@...aro.org
Subject: Re: [PATCH] cpuidle: Avoid possible NULL pointer dereference in __cpuidle_register_device()

On 04/03/2012 07:21 PM, Daniel Lezcano wrote:

> On 04/03/2012 03:15 PM, Srivatsa S. Bhat wrote:
>> On 04/03/2012 05:38 PM, Daniel Lezcano wrote:
>>
>>> On 04/03/2012 01:51 PM, Srivatsa S. Bhat wrote:
>>>> On 04/03/2012 01:01 AM, Daniel Lezcano wrote:
>>>>
>>>>> On 04/02/2012 04:44 PM, Srivatsa S. Bhat wrote:
>>>>>> In __cpuidle_register_device(), "dev->cpu" is used before checking if
>>>>>> dev is
>>>>>> non-NULL. Fix it.
>>>>>>
>>>>>> Signed-off-by: Srivatsa S. Bhat<srivatsa.bhat@...ux.vnet.ibm.com>
>>>>>> ---
>>>>>
>>>>> That should be fixed at the caller level. Usually, static function
>>>>> does
>>>>> not check the function parameters, it is up to the exported
>>>>> function to
>>>>> do that. It is supposed the static functions are called with valid
>>>>> parameters.
>>>>>
>>>>
>>>>
>>>> Ok, good point! I hadn't thought about that.. I just happened to notice
>>>> that in __cpuidle_register_device(), the dev == NULL check is performed
>>>> _after_ dereferencing it, which made the check useless. So I tried to
>>>> fix that within that function. But thanks for pointing out the
>>>> semantics..
>>>>
>>>>> There are two callers for __cpuidle_register_device:
>>>>>    * cpuidle_register_device
>>>>>    * cpuidle_enable_device
>>>>>
>>>>> Both of them do not check 'dev' is a valid parameter. They should as
>>>>> they are exported and could be used by an external module. IMHO,
>>>>> BUG_ON
>>>>> could be used here if dev == NULL.
>>>>>
>>>>
>>>>
>>>> BUG_ON? That would crash the system.. which might be unnecessary..
>>>
>>> Mmh, yes, I agree. never mind.
>>>
>>>> How about checking if dev == NULL in the 2 callers like you suggested
>>>> and returning -EINVAL if dev is indeed NULL?
>>>> (And of course no checks for dev == NULL in __cpuidle_register_device).
>>>
>>> Ok for me.
>>>
>>
>>
>> Great! Here is the updated patch:
>>
>> ---
>>
>> From: Srivatsa S. Bhat<srivatsa.bhat@...ux.vnet.ibm.com>
>> Subject: [PATCH v2] cpuidle: Add checks to avoid NULL pointer dereference
>>
>> The existing check for dev == NULL in __cpuidle_register_device() is
>> rendered
>> useless because dev is dereferenced before the check itself. Moreover,
>> correctly speaking, it is the job of the callers of this function, i.e.,
>> cpuidle_register_device()&  cpuidle_enable_device() (which also happen
>> to be
>> exported functions) to ensure that __cpuidle_register_device() is
>> called with
>> a non-NULL dev.
>>
>> So add the necessary dev == NULL checks in the two callers and remove the
>> (useless) check from __cpuidle_register_device().
>>
>> Signed-off-by: Srivatsa S. Bhat<srivatsa.bhat@...ux.vnet.ibm.com>
> 
> Acked-by: Daniel Lezcano <daniel.lezcano@...aro.org>


Thanks a lot!

> ps : shouldn't this patch be sent in a separate email ?
> 


A separate email (a separate thread rather) is preferred when the old thread
is dead or when the revised patch is fundamentally/drastically different
from the older version.

Otherwise, if the discussion around the patch is active in the thread, it is
best to mail the patch to that thread itself. It makes it easier for developers
to track what is going on in a single thread of discussion (such as who commented
on what, and how did that materialize as a patch and so on).

Another advantage of posting new versions of the patch to the same thread is that
many times we need not explicitly summarize the changes between the new and the
old patch, since the thread itself has enough discussion/history around it.
(However, for significantly complex patches, summary of revision/change in the
patch is always good to have).

Yet another advantage of mailing revisions of the patch to the same thread is
that it makes it easy for the maintainer to pick up the latest patch along with
all the reviewed-by's, acked-by's and tested-by's that came in that thread.

Of course, if the thread is too dense with too much of discussion and there is
a chance that the patch will get lost/missed out, then its a good idea to post
it in a new separate thread, with a link to the old thread so as to provide
some context to new readers.

Regards,
Srivatsa S. Bhat

>> ---
>>
>>   drivers/cpuidle/cpuidle.c |    8 ++++++--
>>   1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 87411ce..eae2f11 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -291,6 +291,9 @@ int cpuidle_enable_device(struct cpuidle_device *dev)
>>       int ret, i;
>>       struct cpuidle_driver *drv = cpuidle_get_driver();
>>
>> +    if (!dev)
>> +        return -EINVAL;
>> +
>>       if (dev->enabled)
>>           return 0;
>>       if (!drv || !cpuidle_curr_governor)
>> @@ -375,8 +378,6 @@ static int __cpuidle_register_device(struct
>> cpuidle_device *dev)
>>       struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu);
>>       struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
>>
>> -    if (!dev)
>> -        return -EINVAL;
>>       if (!try_module_get(cpuidle_driver->owner))
>>           return -EINVAL;
>>
>> @@ -401,6 +402,9 @@ int cpuidle_register_device(struct cpuidle_device
>> *dev)
>>   {
>>       int ret;
>>
>> +    if (!dev)
>> +        return -EINVAL;
>> +
>>       mutex_lock(&cpuidle_lock);
>>
>>       if ((ret = __cpuidle_register_device(dev))) {
>>
>>
> 
> 

--
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/

Powered by blists - more mailing lists