[PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[Julia Lawall]

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>

---
 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_lock(&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);

Re: [PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[Dan Carpenter]

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().

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

Re: [PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[Arend van Spriel]

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.

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

Re: [PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[David Herrmann]

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

Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[Julia Lawall]

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

Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[Jiri Kosina]

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

Re: [PATCH 3/5 v2] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[Julia Lawall]

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

[PATCH 3/5 v3] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[Julia Lawall]

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

Re: [PATCH 3/5 v3] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[Jiri Kosina]

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

Re: [PATCH 3/5] drivers/hid/hid-roccat.c: eliminate a null pointer dereference

[Arend van Spriel]

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