* Re: [PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
@ 2011-10-29 10:53 ` David Herrmann
0 siblings, 0 replies; 21+ messages in thread
From: David Herrmann @ 2011-10-29 10:53 UTC (permalink / raw)
To: Arend van Spriel
Cc: Dan Carpenter, Julia Lawall, Jiri Kosina,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On Sat, Oct 29, 2011 at 11:36 AM, Arend van Spriel <arend@broadcom.com> wrote:
> On 10/29/2011 08:24 AM, Dan Carpenter wrote:
>> On Sat, Oct 29, 2011 at 01:58:15AM +0200, Julia Lawall wrote:
>>> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
>>> index 2596321..36a28b8 100644
>>> --- a/drivers/hid/hid-roccat.c
>>> +++ b/drivers/hid/hid-roccat.c
>>> @@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
>>>
>>> device = devices[minor];
>>>
>>> - mutex_lock(&device->readers_lock);
>>> -
>>> if (!device) {
>>> pr_emerg("roccat device with minor %d doesn't exist\n", minor);
>>> - error = -ENODEV;
>>> - goto exit_err;
>>> + kfree(reader);
>>> + mutex_lock(&devices_lock);
>>
>> Typo. mutex_unlock() instead of mutex_lock().
>
> This is no typo, but simply wrong. Remove the mutex_lock() as we are
> leaving the function here in error flow.
No, this one is definitely needed. Its devices_lock, not
device->readers_lock! And devices_lock is locked before so we need to
unlock it if we return in this branch.
>>> + return -ENODEV;
>>> }
>>>
>>> + mutex_lock(&device->readers_lock);
>>> +
>>> if (!device->open++) {
>>> /* power on device on adding first reader */
>>> error = hid_hw_power(device->hid, PM_HINT_FULLON);
>>>
>>
>> regards,
>> dan carpenter
>
> Gr. AvS
Regards
David
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
@ 2011-10-29 10:53 ` David Herrmann
0 siblings, 0 replies; 21+ messages in thread
From: David Herrmann @ 2011-10-29 10:53 UTC (permalink / raw)
To: Arend van Spriel
Cc: Dan Carpenter, Julia Lawall, Jiri Kosina,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On Sat, Oct 29, 2011 at 11:36 AM, Arend van Spriel <arend@broadcom.com> wrote:
> On 10/29/2011 08:24 AM, Dan Carpenter wrote:
>> On Sat, Oct 29, 2011 at 01:58:15AM +0200, Julia Lawall wrote:
>>> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
>>> index 2596321..36a28b8 100644
>>> --- a/drivers/hid/hid-roccat.c
>>> +++ b/drivers/hid/hid-roccat.c
>>> @@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
>>>
>>> device = devices[minor];
>>>
>>> - mutex_lock(&device->readers_lock);
>>> -
>>> if (!device) {
>>> pr_emerg("roccat device with minor %d doesn't exist\n", minor);
>>> - error = -ENODEV;
>>> - goto exit_err;
>>> + kfree(reader);
>>> + mutex_lock(&devices_lock);
>>
>> Typo. mutex_unlock() instead of mutex_lock().
>
> This is no typo, but simply wrong. Remove the mutex_lock() as we are
> leaving the function here in error flow.
No, this one is definitely needed. Its devices_lock, not
device->readers_lock! And devices_lock is locked before so we need to
unlock it if we return in this branch.
>>> + return -ENODEV;
>>> }
>>>
>>> + mutex_lock(&device->readers_lock);
>>> +
>>> if (!device->open++) {
>>> /* power on device on adding first reader */
>>> error = hid_hw_power(device->hid, PM_HINT_FULLON);
>>>
>>
>> regards,
>> dan carpenter
>
> Gr. AvS
Regards
David
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer
2011-10-29 10:53 ` David Herrmann
@ 2011-10-29 12:11 ` Julia Lawall
-1 siblings, 0 replies; 21+ messages in thread
From: Julia Lawall @ 2011-10-29 12:11 UTC (permalink / raw)
To: David Herrmann
Cc: Arend van Spriel, Dan Carpenter, Jiri Kosina,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
From: Julia Lawall <julia@diku.dk>
It is not possible to take the lock in device if device is NULL.
The mutex_lock is thus moved after the NULL test, and the relevant part of
the shared error handling code is moved up.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r@
expression E, E1;
identifier f;
statement S1,S2,S3;
@@
if (E = NULL)
{
... when != if (E = NULL || ...) S1 else S2
when != E = E1
*E->f
... when any
return ...;
}
else S3
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
mutex_lock changed to mutex_unlock in error handling code
drivers/hid/hid-roccat.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 2596321..36a28b8 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
device = devices[minor];
- mutex_lock(&device->readers_lock);
-
if (!device) {
pr_emerg("roccat device with minor %d doesn't exist\n", minor);
- error = -ENODEV;
- goto exit_err;
+ kfree(reader);
+ mutex_unlock(&devices_lock);
+ return -ENODEV;
}
+ mutex_lock(&device->readers_lock);
+
if (!device->open++) {
/* power on device on adding first reader */
error = hid_hw_power(device->hid, PM_HINT_FULLON);
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
@ 2011-10-29 12:11 ` Julia Lawall
0 siblings, 0 replies; 21+ messages in thread
From: Julia Lawall @ 2011-10-29 12:11 UTC (permalink / raw)
To: David Herrmann
Cc: Arend van Spriel, Dan Carpenter, Jiri Kosina,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
From: Julia Lawall <julia@diku.dk>
It is not possible to take the lock in device if device is NULL.
The mutex_lock is thus moved after the NULL test, and the relevant part of
the shared error handling code is moved up.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r@
expression E, E1;
identifier f;
statement S1,S2,S3;
@@
if (E == NULL)
{
... when != if (E == NULL || ...) S1 else S2
when != E = E1
*E->f
... when any
return ...;
}
else S3
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
mutex_lock changed to mutex_unlock in error handling code
drivers/hid/hid-roccat.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 2596321..36a28b8 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
device = devices[minor];
- mutex_lock(&device->readers_lock);
-
if (!device) {
pr_emerg("roccat device with minor %d doesn't exist\n", minor);
- error = -ENODEV;
- goto exit_err;
+ kfree(reader);
+ mutex_unlock(&devices_lock);
+ return -ENODEV;
}
+ mutex_lock(&device->readers_lock);
+
if (!device->open++) {
/* power on device on adding first reader */
error = hid_hw_power(device->hid, PM_HINT_FULLON);
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer
2011-10-29 12:11 ` [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference Julia Lawall
@ 2011-10-29 17:00 ` Jiri Kosina
-1 siblings, 0 replies; 21+ messages in thread
From: Jiri Kosina @ 2011-10-29 17:00 UTC (permalink / raw)
To: Julia Lawall
Cc: David Herrmann, Arend van Spriel, Dan Carpenter,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On Sat, 29 Oct 2011, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
>
> It is not possible to take the lock in device if device is NULL.
> The mutex_lock is thus moved after the NULL test, and the relevant part of
> the shared error handling code is moved up.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> expression E, E1;
> identifier f;
> statement S1,S2,S3;
> @@
>
> if (E = NULL)
> {
> ... when != if (E = NULL || ...) S1 else S2
> when != E = E1
> *E->f
> ... when any
> return ...;
> }
> else S3
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
> mutex_lock changed to mutex_unlock in error handling code
>
> drivers/hid/hid-roccat.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 2596321..36a28b8 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
> @@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
>
> device = devices[minor];
>
> - mutex_lock(&device->readers_lock);
> -
> if (!device) {
> pr_emerg("roccat device with minor %d doesn't exist\n", minor);
> - error = -ENODEV;
> - goto exit_err;
> + kfree(reader);
> + mutex_unlock(&devices_lock);
> + return -ENODEV;
> }
>
> + mutex_lock(&device->readers_lock);
> +
> if (!device->open++) {
> /* power on device on adding first reader */
> error = hid_hw_power(device->hid, PM_HINT_FULLON);
Julia,
thanks a lot for fixing this.
Could you please redo the patch in a way that it adds second
'exit_unlock1' label (and renames 'exit_unlock' to 'exit_unlock2') (or
any appropriate variation of the names) and preserve error path using goto
instead of mixture of returns and gotos that this patch would introduce?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
@ 2011-10-29 17:00 ` Jiri Kosina
0 siblings, 0 replies; 21+ messages in thread
From: Jiri Kosina @ 2011-10-29 17:00 UTC (permalink / raw)
To: Julia Lawall
Cc: David Herrmann, Arend van Spriel, Dan Carpenter,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On Sat, 29 Oct 2011, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
>
> It is not possible to take the lock in device if device is NULL.
> The mutex_lock is thus moved after the NULL test, and the relevant part of
> the shared error handling code is moved up.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> expression E, E1;
> identifier f;
> statement S1,S2,S3;
> @@
>
> if (E == NULL)
> {
> ... when != if (E == NULL || ...) S1 else S2
> when != E = E1
> *E->f
> ... when any
> return ...;
> }
> else S3
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
> mutex_lock changed to mutex_unlock in error handling code
>
> drivers/hid/hid-roccat.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 2596321..36a28b8 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
> @@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
>
> device = devices[minor];
>
> - mutex_lock(&device->readers_lock);
> -
> if (!device) {
> pr_emerg("roccat device with minor %d doesn't exist\n", minor);
> - error = -ENODEV;
> - goto exit_err;
> + kfree(reader);
> + mutex_unlock(&devices_lock);
> + return -ENODEV;
> }
>
> + mutex_lock(&device->readers_lock);
> +
> if (!device->open++) {
> /* power on device on adding first reader */
> error = hid_hw_power(device->hid, PM_HINT_FULLON);
Julia,
thanks a lot for fixing this.
Could you please redo the patch in a way that it adds second
'exit_unlock1' label (and renames 'exit_unlock' to 'exit_unlock2') (or
any appropriate variation of the names) and preserve error path using goto
instead of mixture of returns and gotos that this patch would introduce?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer
2011-10-29 17:00 ` [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference Jiri Kosina
@ 2011-10-29 18:18 ` Julia Lawall
-1 siblings, 0 replies; 21+ messages in thread
From: Julia Lawall @ 2011-10-29 18:18 UTC (permalink / raw)
To: Jiri Kosina
Cc: David Herrmann, Arend van Spriel, Dan Carpenter,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On Sat, 29 Oct 2011, Jiri Kosina wrote:
> On Sat, 29 Oct 2011, Julia Lawall wrote:
>
> > From: Julia Lawall <julia@diku.dk>
> >
> > It is not possible to take the lock in device if device is NULL.
> > The mutex_lock is thus moved after the NULL test, and the relevant part of
> > the shared error handling code is moved up.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @r@
> > expression E, E1;
> > identifier f;
> > statement S1,S2,S3;
> > @@
> >
> > if (E = NULL)
> > {
> > ... when != if (E = NULL || ...) S1 else S2
> > when != E = E1
> > *E->f
> > ... when any
> > return ...;
> > }
> > else S3
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> >
> > ---
> > mutex_lock changed to mutex_unlock in error handling code
> >
> > drivers/hid/hid-roccat.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> > index 2596321..36a28b8 100644
> > --- a/drivers/hid/hid-roccat.c
> > +++ b/drivers/hid/hid-roccat.c
> > @@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
> >
> > device = devices[minor];
> >
> > - mutex_lock(&device->readers_lock);
> > -
> > if (!device) {
> > pr_emerg("roccat device with minor %d doesn't exist\n", minor);
> > - error = -ENODEV;
> > - goto exit_err;
> > + kfree(reader);
> > + mutex_unlock(&devices_lock);
> > + return -ENODEV;
> > }
> >
> > + mutex_lock(&device->readers_lock);
> > +
> > if (!device->open++) {
> > /* power on device on adding first reader */
> > error = hid_hw_power(device->hid, PM_HINT_FULLON);
>
> Julia,
>
> thanks a lot for fixing this.
>
> Could you please redo the patch in a way that it adds second
> 'exit_unlock1' label (and renames 'exit_unlock' to 'exit_unlock2') (or
> any appropriate variation of the names) and preserve error path using goto
> instead of mixture of returns and gotos that this patch would introduce?
OK. At first I couldn't see how to do this without duplicating the
unlocks in the success and failure cases, but perhaps there is a solution
by adding more gotos. I'll try for that.
julia
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
@ 2011-10-29 18:18 ` Julia Lawall
0 siblings, 0 replies; 21+ messages in thread
From: Julia Lawall @ 2011-10-29 18:18 UTC (permalink / raw)
To: Jiri Kosina
Cc: David Herrmann, Arend van Spriel, Dan Carpenter,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On Sat, 29 Oct 2011, Jiri Kosina wrote:
> On Sat, 29 Oct 2011, Julia Lawall wrote:
>
> > From: Julia Lawall <julia@diku.dk>
> >
> > It is not possible to take the lock in device if device is NULL.
> > The mutex_lock is thus moved after the NULL test, and the relevant part of
> > the shared error handling code is moved up.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @r@
> > expression E, E1;
> > identifier f;
> > statement S1,S2,S3;
> > @@
> >
> > if (E == NULL)
> > {
> > ... when != if (E == NULL || ...) S1 else S2
> > when != E = E1
> > *E->f
> > ... when any
> > return ...;
> > }
> > else S3
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> >
> > ---
> > mutex_lock changed to mutex_unlock in error handling code
> >
> > drivers/hid/hid-roccat.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> > index 2596321..36a28b8 100644
> > --- a/drivers/hid/hid-roccat.c
> > +++ b/drivers/hid/hid-roccat.c
> > @@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
> >
> > device = devices[minor];
> >
> > - mutex_lock(&device->readers_lock);
> > -
> > if (!device) {
> > pr_emerg("roccat device with minor %d doesn't exist\n", minor);
> > - error = -ENODEV;
> > - goto exit_err;
> > + kfree(reader);
> > + mutex_unlock(&devices_lock);
> > + return -ENODEV;
> > }
> >
> > + mutex_lock(&device->readers_lock);
> > +
> > if (!device->open++) {
> > /* power on device on adding first reader */
> > error = hid_hw_power(device->hid, PM_HINT_FULLON);
>
> Julia,
>
> thanks a lot for fixing this.
>
> Could you please redo the patch in a way that it adds second
> 'exit_unlock1' label (and renames 'exit_unlock' to 'exit_unlock2') (or
> any appropriate variation of the names) and preserve error path using goto
> instead of mixture of returns and gotos that this patch would introduce?
OK. At first I couldn't see how to do this without duplicating the
unlocks in the success and failure cases, but perhaps there is a solution
by adding more gotos. I'll try for that.
julia
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/5 v3] drivers/hid/hid-roccat.c: eliminate a null pointer
2011-10-29 17:00 ` [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference Jiri Kosina
@ 2011-10-29 18:45 ` Julia Lawall
-1 siblings, 0 replies; 21+ messages in thread
From: Julia Lawall @ 2011-10-29 18:45 UTC (permalink / raw)
To: Jiri Kosina
Cc: David Herrmann, Arend van Spriel, Dan Carpenter,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
From nobody Sat Oct 29 01:38:55 CEST 2011
From: Julia Lawall <julia@diku.dk>
To: Jiri Kosina <jkosina@suse.cz>
Cc: linux-input@vger.kernel.org,linux-kernel@vger.kernel.org
Subject: [PATCH 3/5 v3] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
From: Julia Lawall <julia@diku.dk>
It is not possible to take the lock in device if device is NULL.
The mutex_lock is thus moved after the NULL test. New error handling
labels are added at the end to differentiate between the cases where
different sets of locks should be unlocks, and between whether or not
reader should be freed (only on error).
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r@
expression E, E1;
identifier f;
statement S1,S2,S3;
@@
if (E = NULL)
{
... when != if (E = NULL || ...) S1 else S2
when != E = E1
*E->f
... when any
return ...;
}
else S3
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
Moved the error handling code back to the end.
drivers/hid/hid-roccat.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 2596321..b685b04 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -163,27 +163,27 @@ static int roccat_open(struct inode *inode, struct file *file)
device = devices[minor];
- mutex_lock(&device->readers_lock);
-
if (!device) {
pr_emerg("roccat device with minor %d doesn't exist\n", minor);
error = -ENODEV;
- goto exit_err;
+ goto exit_err_devices;
}
+ mutex_lock(&device->readers_lock);
+
if (!device->open++) {
/* power on device on adding first reader */
error = hid_hw_power(device->hid, PM_HINT_FULLON);
if (error < 0) {
--device->open;
- goto exit_err;
+ goto exit_err_readers;
}
error = hid_hw_open(device->hid);
if (error < 0) {
hid_hw_power(device->hid, PM_HINT_NORMAL);
--device->open;
- goto exit_err;
+ goto exit_err_readers;
}
}
@@ -194,13 +194,13 @@ static int roccat_open(struct inode *inode, struct file *file)
list_add_tail(&reader->node, &device->readers);
file->private_data = reader;
-exit_unlock:
+exit_err_readers:
mutex_unlock(&device->readers_lock);
+exit_err_devices:
mutex_unlock(&devices_lock);
+ if (error)
+ kfree(reader);
return error;
-exit_err:
- kfree(reader);
- goto exit_unlock;
}
static int roccat_release(struct inode *inode, struct file *file)
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 3/5 v3] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
@ 2011-10-29 18:45 ` Julia Lawall
0 siblings, 0 replies; 21+ messages in thread
From: Julia Lawall @ 2011-10-29 18:45 UTC (permalink / raw)
To: Jiri Kosina
Cc: David Herrmann, Arend van Spriel, Dan Carpenter,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
>From nobody Sat Oct 29 01:38:55 CEST 2011
From: Julia Lawall <julia@diku.dk>
To: Jiri Kosina <jkosina@suse.cz>
Cc: linux-input@vger.kernel.org,linux-kernel@vger.kernel.org
Subject: [PATCH 3/5 v3] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
From: Julia Lawall <julia@diku.dk>
It is not possible to take the lock in device if device is NULL.
The mutex_lock is thus moved after the NULL test. New error handling
labels are added at the end to differentiate between the cases where
different sets of locks should be unlocks, and between whether or not
reader should be freed (only on error).
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r@
expression E, E1;
identifier f;
statement S1,S2,S3;
@@
if (E == NULL)
{
... when != if (E == NULL || ...) S1 else S2
when != E = E1
*E->f
... when any
return ...;
}
else S3
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
Moved the error handling code back to the end.
drivers/hid/hid-roccat.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 2596321..b685b04 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -163,27 +163,27 @@ static int roccat_open(struct inode *inode, struct file *file)
device = devices[minor];
- mutex_lock(&device->readers_lock);
-
if (!device) {
pr_emerg("roccat device with minor %d doesn't exist\n", minor);
error = -ENODEV;
- goto exit_err;
+ goto exit_err_devices;
}
+ mutex_lock(&device->readers_lock);
+
if (!device->open++) {
/* power on device on adding first reader */
error = hid_hw_power(device->hid, PM_HINT_FULLON);
if (error < 0) {
--device->open;
- goto exit_err;
+ goto exit_err_readers;
}
error = hid_hw_open(device->hid);
if (error < 0) {
hid_hw_power(device->hid, PM_HINT_NORMAL);
--device->open;
- goto exit_err;
+ goto exit_err_readers;
}
}
@@ -194,13 +194,13 @@ static int roccat_open(struct inode *inode, struct file *file)
list_add_tail(&reader->node, &device->readers);
file->private_data = reader;
-exit_unlock:
+exit_err_readers:
mutex_unlock(&device->readers_lock);
+exit_err_devices:
mutex_unlock(&devices_lock);
+ if (error)
+ kfree(reader);
return error;
-exit_err:
- kfree(reader);
- goto exit_unlock;
}
static int roccat_release(struct inode *inode, struct file *file)
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 3/5 v3] drivers/hid/hid-roccat.c: eliminate a null pointer
2011-10-29 18:45 ` [PATCH 3/5 v3] drivers/hid/hid-roccat.c: eliminate a null pointer dereference Julia Lawall
@ 2011-11-01 14:14 ` Jiri Kosina
-1 siblings, 0 replies; 21+ messages in thread
From: Jiri Kosina @ 2011-11-01 14:14 UTC (permalink / raw)
To: Julia Lawall
Cc: David Herrmann, Arend van Spriel, Dan Carpenter,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On Sat, 29 Oct 2011, Julia Lawall wrote:
> It is not possible to take the lock in device if device is NULL.
> The mutex_lock is thus moved after the NULL test. New error handling
> labels are added at the end to differentiate between the cases where
> different sets of locks should be unlocks, and between whether or not
> reader should be freed (only on error).
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> expression E, E1;
> identifier f;
> statement S1,S2,S3;
> @@
>
> if (E = NULL)
> {
> ... when != if (E = NULL || ...) S1 else S2
> when != E = E1
> *E->f
> ... when any
> return ...;
> }
> else S3
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
Applied, thank you.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/5 v3] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
@ 2011-11-01 14:14 ` Jiri Kosina
0 siblings, 0 replies; 21+ messages in thread
From: Jiri Kosina @ 2011-11-01 14:14 UTC (permalink / raw)
To: Julia Lawall
Cc: David Herrmann, Arend van Spriel, Dan Carpenter,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On Sat, 29 Oct 2011, Julia Lawall wrote:
> It is not possible to take the lock in device if device is NULL.
> The mutex_lock is thus moved after the NULL test. New error handling
> labels are added at the end to differentiate between the cases where
> different sets of locks should be unlocks, and between whether or not
> reader should be freed (only on error).
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> expression E, E1;
> identifier f;
> statement S1,S2,S3;
> @@
>
> if (E == NULL)
> {
> ... when != if (E == NULL || ...) S1 else S2
> when != E = E1
> *E->f
> ... when any
> return ...;
> }
> else S3
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
Applied, thank you.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null
2011-10-29 10:53 ` David Herrmann
@ 2011-10-31 9:22 ` Arend van Spriel
-1 siblings, 0 replies; 21+ messages in thread
From: Arend van Spriel @ 2011-10-31 9:22 UTC (permalink / raw)
To: David Herrmann
Cc: Dan Carpenter, Julia Lawall, Jiri Kosina,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On 10/29/2011 12:53 PM, David Herrmann wrote:
> On Sat, Oct 29, 2011 at 11:36 AM, Arend van Spriel <arend@broadcom.com> wrote:
>> On 10/29/2011 08:24 AM, Dan Carpenter wrote:
>>> On Sat, Oct 29, 2011 at 01:58:15AM +0200, Julia Lawall wrote:
>>>> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
>>>> index 2596321..36a28b8 100644
>>>> --- a/drivers/hid/hid-roccat.c
>>>> +++ b/drivers/hid/hid-roccat.c
>>>> @@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
>>>>
>>>> device = devices[minor];
>>>>
>>>> - mutex_lock(&device->readers_lock);
>>>> -
>>>> if (!device) {
>>>> pr_emerg("roccat device with minor %d doesn't exist\n", minor);
>>>> - error = -ENODEV;
>>>> - goto exit_err;
>>>> + kfree(reader);
>>>> + mutex_lock(&devices_lock);
>>>
>>> Typo. mutex_unlock() instead of mutex_lock().
>>
>> This is no typo, but simply wrong. Remove the mutex_lock() as we are
>> leaving the function here in error flow.
>
> No, this one is definitely needed. Its devices_lock, not
> device->readers_lock! And devices_lock is locked before so we need to
> unlock it if we return in this branch.
You are right. I missed that. As usual Dan's eye is sharper ;-)
Gr. AvS
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null pointer dereference
@ 2011-10-31 9:22 ` Arend van Spriel
0 siblings, 0 replies; 21+ messages in thread
From: Arend van Spriel @ 2011-10-31 9:22 UTC (permalink / raw)
To: David Herrmann
Cc: Dan Carpenter, Julia Lawall, Jiri Kosina,
kernel-janitors@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
On 10/29/2011 12:53 PM, David Herrmann wrote:
> On Sat, Oct 29, 2011 at 11:36 AM, Arend van Spriel <arend@broadcom.com> wrote:
>> On 10/29/2011 08:24 AM, Dan Carpenter wrote:
>>> On Sat, Oct 29, 2011 at 01:58:15AM +0200, Julia Lawall wrote:
>>>> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
>>>> index 2596321..36a28b8 100644
>>>> --- a/drivers/hid/hid-roccat.c
>>>> +++ b/drivers/hid/hid-roccat.c
>>>> @@ -163,14 +163,15 @@ static int roccat_open(struct inode *inode, struct file *file)
>>>>
>>>> device = devices[minor];
>>>>
>>>> - mutex_lock(&device->readers_lock);
>>>> -
>>>> if (!device) {
>>>> pr_emerg("roccat device with minor %d doesn't exist\n", minor);
>>>> - error = -ENODEV;
>>>> - goto exit_err;
>>>> + kfree(reader);
>>>> + mutex_lock(&devices_lock);
>>>
>>> Typo. mutex_unlock() instead of mutex_lock().
>>
>> This is no typo, but simply wrong. Remove the mutex_lock() as we are
>> leaving the function here in error flow.
>
> No, this one is definitely needed. Its devices_lock, not
> device->readers_lock! And devices_lock is locked before so we need to
> unlock it if we return in this branch.
You are right. I missed that. As usual Dan's eye is sharper ;-)
Gr. AvS
^ permalink raw reply [flat|nested] 21+ messages in thread