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

From: Julia Lawall <redacted>

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

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

On Sat, Oct 29, 2011 at 01:58:15AM +0200, Julia Lawall wrote:

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

On 10/29/2011 08:24 AM, Dan Carpenter wrote:

On Sat, Oct 29, 2011 at 01:58:15AM +0200, Julia Lawall wrote:

quoted

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.

quoted

+		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

On Sat, Oct 29, 2011 at 11:36 AM, Arend van Spriel [off-list ref] wrote:

On 10/29/2011 08:24 AM, Dan Carpenter wrote:

quoted

On Sat, Oct 29, 2011 at 01:58:15AM +0200, Julia Lawall wrote:

quoted

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.

quoted

quoted

+            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

From: Julia Lawall <redacted>

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

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

On Sat, 29 Oct 2011, Julia Lawall wrote:

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

On Sat, 29 Oct 2011, Jiri Kosina wrote:

On Sat, 29 Oct 2011, Julia Lawall wrote:

quoted

From: Julia Lawall <redacted>

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

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

From nobody Sat Oct 29 01:38:55 CEST 2011

From: Julia Lawall <redacted>
To: Jiri Kosina <redacted>
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 <redacted>

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

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

On 10/29/2011 12:53 PM, David Herrmann wrote:

On Sat, Oct 29, 2011 at 11:36 AM, Arend van Spriel [off-list ref] wrote:

quoted

On 10/29/2011 08:24 AM, Dan Carpenter wrote:

quoted

On Sat, Oct 29, 2011 at 01:58:15AM +0200, Julia Lawall wrote:

quoted

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

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

Applied, thank you.

-- 
Jiri Kosina
SUSE Labs