Re: [PATCH] cpuidle: Avoid possible NULL pointer dereference in __cpuidle_register_device()

From: Daniel Lezcano
Date: Tue Apr 03 2012 - 10:17:10 EST

On 04/03/2012 04:04 PM, Srivatsa S. Bhat wrote:
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@xxxxxxxxxxxxxxxxxx>
---

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@xxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxx>

Acked-by: Daniel Lezcano<daniel.lezcano@xxxxxxxxxx>


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.

Ok. I use the thread message-id when git-send-email'ing the patch in a thread context, so the patch appears in the thread but separated. I assume that facilitates the maintainer to use git-am.

-- Daniel


---

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))) {







--
<http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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