[patch]fix memleak in error case of hiddev

[patch]fix memleak in error case of hiddev

[Oliver Neukum]

Hi,

this bug is old, the patch should go into 2.6.28 and stable.

This fixes a memleak in hiddev's open in an error case.

Signed-off-by: Oliver Neukum <oneukum@suse.de>

	Regards
		Oliver

---

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index b218cbc..6834d1f 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -291,8 +291,13 @@ static int hiddev_open(struct inode *inode, struct file *file)
 		if (list->hiddev->exist) {
 			struct hid_device *hid = hiddev_table[i]->hid;
 			res = usbhid_get_power(hid);
-			if (res < 0)
+			if (res < 0) {
+				spin_lock_irqsave(&list->hiddev->list_lock, flags);
+				list_del(&list->node);
+				spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
+				kfree(list->hiddev);
 				return -EIO;
+			}
 			usbhid_open(hid);
 		}
 

Re: [patch]fix memleak in error case of hiddev

[Jiri Kosina]

On Fri, 31 Oct 2008, Oliver Neukum wrote:

> this bug is old, the patch should go into 2.6.28 and stable.
> This fixes a memleak in hiddev's open in an error case.
> ---
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index b218cbc..6834d1f 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -291,8 +291,13 @@ static int hiddev_open(struct inode *inode, struct file *file)
>  		if (list->hiddev->exist) {
>  			struct hid_device *hid = hiddev_table[i]->hid;
>  			res = usbhid_get_power(hid);
> -			if (res < 0)
> +			if (res < 0) {
> +				spin_lock_irqsave(&list->hiddev->list_lock, flags);
> +				list_del(&list->node);
> +				spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
> +				kfree(list->hiddev);
>  				return -EIO;
> +			}
>  			usbhid_open(hid);
>  		}

Hmm, this is probably based on not-yet-merged usbhid autosuspend patchset, 
right?

Now, you are right that hiddev_open() doesn't handle error condition from 
usbhid_open(), and that should be fixed. But that doesn't seem to be 
addressed by your patch at all ... ?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [patch]fix memleak in error case of hiddev

[Oliver Neukum]

> Hmm, this is probably based on not-yet-merged usbhid autosuspend patchset, 
> right?

Yes, that's right. I looked at the wrong tree.
 
> Now, you are right that hiddev_open() doesn't handle error condition from 
> usbhid_open(), and that should be fixed. But that doesn't seem to be 
> addressed by your patch at all ... ?

Yes I am looking at hiddev currently and have found several errors.
I'll make patches for the appropriate trees.

	Regards
		Oliver

Re: [patch]fix memleak in error case of hiddev

[Oliver Neukum]

Am Freitag, 31. Oktober 2008 15:08:07 schrieb Jiri Kosina:
> Now, you are right that hiddev_open() doesn't handle error condition from 
> usbhid_open(), and that should be fixed. But that doesn't seem to be 
> addressed by your patch at all ... ?

Very well, this is what I've found in hiddev (with the autosuspend patch).
I am porting this to the vanilla and the stable tree.

- failure to test for lower range of minors
- addition to list before open finishes
- failure to handle errors from usbhid_open()
- possibility to miss a wakeup in hiddev_read
- open() races with hiddev_connect()

Note that this is untested and should be tested.

	Regards
		Oliver

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 6834d1f..3b3e179 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -268,40 +268,42 @@ static int hiddev_release(struct inode * inode, struct file * file)
 static int hiddev_open(struct inode *inode, struct file *file)
 {
 	struct hiddev_list *list;
-	unsigned long flags;
 	int res;
 
 	int i = iminor(inode) - HIDDEV_MINOR_BASE;
 
-	if (i >= HIDDEV_MINORS || !hiddev_table[i])
+	if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
 		return -ENODEV;
 
 	if (!(list = kzalloc(sizeof(struct hiddev_list), GFP_KERNEL)))
 		return -ENOMEM;
 
 	list->hiddev = hiddev_table[i];
-
-	spin_lock_irqsave(&list->hiddev->list_lock, flags);
-	list_add_tail(&list->node, &hiddev_table[i]->list);
-	spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
-
 	file->private_data = list;
 
 	if (!list->hiddev->open++)
 		if (list->hiddev->exist) {
 			struct hid_device *hid = hiddev_table[i]->hid;
 			res = usbhid_get_power(hid);
-			if (res < 0) {
-				spin_lock_irqsave(&list->hiddev->list_lock, flags);
-				list_del(&list->node);
-				spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
-				kfree(list->hiddev);
-				return -EIO;
-			}
-			usbhid_open(hid);
+			if (res < 0)
+				goto bail;
+			res = usbhid_open(hid);
+			if (res < 0)
+				goto bail_power_put;
 		}
 
+	spin_lock_irq(&list->hiddev->list_lock);
+	list_add_tail(&list->node, &hiddev_table[i]->list);
+	spin_unlock_irq(&list->hiddev->list_lock);
+
 	return 0;
+
+bail_power_put:
+	usbhid_put_power(list->hiddev->hid);
+bail:
+	file->private_data = NULL;
+	kfree(list->hiddev);
+	return -EIO;
 }
 
 /*
@@ -330,8 +332,8 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 
 	while (retval == 0) {
 		if (list->head == list->tail) {
-			add_wait_queue(&list->hiddev->wait, &wait);
 			set_current_state(TASK_INTERRUPTIBLE);
+			add_wait_queue(&list->hiddev->wait, &wait);
 
 			while (list->head == list->tail) {
 				if (file->f_flags & O_NONBLOCK) {
@@ -348,7 +350,6 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 				}
 
 				schedule();
-				set_current_state(TASK_INTERRUPTIBLE);
 			}
 
 			set_current_state(TASK_RUNNING);
@@ -823,13 +824,6 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
 		return -1;
 
-	retval = usb_register_dev(usbhid->intf, &hiddev_class);
-	if (retval) {
-		err_hid("Not able to get a minor for this device.");
-		kfree(hiddev);
-		return -1;
-	}
-
 	init_waitqueue_head(&hiddev->wait);
 	INIT_LIST_HEAD(&hiddev->list);
 	spin_lock_init(&hiddev->list_lock);
@@ -841,6 +835,14 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 
 	hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
 
+	retval = usb_register_dev(usbhid->intf, &hiddev_class);
+	if (retval) {
+		err_hid("Not able to get a minor for this device.");
+		hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = NULL;
+		kfree(hiddev);
+		return -1;
+	}
+
 	return 0;
 }
 

Re: [patch]fix memleak in error case of hiddev

[Jiri Kosina]

On Fri, 31 Oct 2008, Oliver Neukum wrote:

> > Now, you are right that hiddev_open() doesn't handle error condition from 
> > usbhid_open(), and that should be fixed. But that doesn't seem to be 
> > addressed by your patch at all ... ?
> Very well, this is what I've found in hiddev (with the autosuspend patch).
> I am porting this to the vanilla and the stable tree.
> - failure to test for lower range of minors
> - addition to list before open finishes
> - failure to handle errors from usbhid_open()
> - possibility to miss a wakeup in hiddev_read
> - open() races with hiddev_connect()
> Note that this is untested and should be tested.

Does the patch below look OK to you? I have removed the dependency on 
autosuspend code for now.

Thanks.


 drivers/hid/usbhid/hiddev.c |   38 ++++++++++++++++++++++----------------
 1 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 3ac3207..99b6c65 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -270,7 +270,7 @@ static int hiddev_open(struct inode *inode, struct file *file)
 
 	int i = iminor(inode) - HIDDEV_MINOR_BASE;
 
-	if (i >= HIDDEV_MINORS || !hiddev_table[i])
+	if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
 		return -ENODEV;
 
 	if (!(list = kzalloc(sizeof(struct hiddev_list), GFP_KERNEL)))
@@ -278,16 +278,22 @@ static int hiddev_open(struct inode *inode, struct file *file)
 
 	list->hiddev = hiddev_table[i];
 
+	file->private_data = list;
+
+	if (!list->hiddev->open++) {
+		if (list->hiddev->exist) {
+			if (usbhid_open(hiddev_table[i]->hid) < 0) {
+				file->private_data = NULL;
+				kfree(list->hiddev);
+				return -EIO;
+			}
+		}
+	}
+
 	spin_lock_irqsave(&list->hiddev->list_lock, flags);
 	list_add_tail(&list->node, &hiddev_table[i]->list);
 	spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
 
-	file->private_data = list;
-
-	if (!list->hiddev->open++)
-		if (list->hiddev->exist)
-			usbhid_open(hiddev_table[i]->hid);
-
 	return 0;
 }
 
@@ -317,8 +323,8 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 
 	while (retval == 0) {
 		if (list->head == list->tail) {
-			add_wait_queue(&list->hiddev->wait, &wait);
 			set_current_state(TASK_INTERRUPTIBLE);
+			add_wait_queue(&list->hiddev->wait, &wait);
 
 			while (list->head == list->tail) {
 				if (file->f_flags & O_NONBLOCK) {
@@ -335,7 +341,6 @@ static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t coun
 				}
 
 				schedule();
-				set_current_state(TASK_INTERRUPTIBLE);
 			}
 
 			set_current_state(TASK_RUNNING);
@@ -810,13 +815,6 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
 		return -1;
 
-	retval = usb_register_dev(usbhid->intf, &hiddev_class);
-	if (retval) {
-		err_hid("Not able to get a minor for this device.");
-		kfree(hiddev);
-		return -1;
-	}
-
 	init_waitqueue_head(&hiddev->wait);
 	INIT_LIST_HEAD(&hiddev->list);
 	spin_lock_init(&hiddev->list_lock);
@@ -828,6 +826,14 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 
 	hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
 
+	retval = usb_register_dev(usbhid->intf, &hiddev_class);
+	if (retval) {
+		err_hid("Not able to get a minor for this device.");
+		hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = NULL;
+		kfree(hiddev);
+		return -1;
+	}
+
 	return 0;
 }
 

Re: [patch]fix memleak in error case of hiddev

[Oliver Neukum]

Am Mittwoch, 12. November 2008 15:48:38 schrieb Jiri Kosina:
> On Fri, 31 Oct 2008, Oliver Neukum wrote:
> 
> > > Now, you are right that hiddev_open() doesn't handle error condition from 
> > > usbhid_open(), and that should be fixed. But that doesn't seem to be 
> > > addressed by your patch at all ... ?
> > Very well, this is what I've found in hiddev (with the autosuspend patch).
> > I am porting this to the vanilla and the stable tree.
> > - failure to test for lower range of minors
> > - addition to list before open finishes
> > - failure to handle errors from usbhid_open()
> > - possibility to miss a wakeup in hiddev_read
> > - open() races with hiddev_connect()
> > Note that this is untested and should be tested.
> 
> Does the patch below look OK to you? I have removed the dependency on 
> autosuspend code for now.

No, it doesn't look good.

> +	if (!list->hiddev->open++) {
> +		if (list->hiddev->exist) {

The order of checks is inverted. If the device no longer exists
opening should fail, even if the device is already opened.
Didn't I post a patch that does this? My memory is a bit clouded.

	Regards
		Oliver

Re: [patch]fix memleak in error case of hiddev

[Jiri Kosina]

On Wed, 12 Nov 2008, Oliver Neukum wrote:

> > +	if (!list->hiddev->open++) {
> > +		if (list->hiddev->exist) {
> The order of checks is inverted. If the device no longer exists opening 
> should fail, even if the device is already opened. 

Right.

> Didn't I post a patch that does this? My memory is a bit clouded.

I don't seem to have it, but I have been buried in hundreds of pending 
e-mails after coming back from vacation. So if you have it handy with 
changelog and Signed-off-by, I'd appreciate if you resend it to me. 
Otherwise I'll dig for it a little bit harder.

Thanks,

-- 
Jiri Kosina

Re: [patch]fix memleak in error case of hiddev

[Jiri Slaby]

On 11/12/2008 03:55 PM, Oliver Neukum wrote:
> Am Mittwoch, 12. November 2008 15:48:38 schrieb Jiri Kosina:
>> +	if (!list->hiddev->open++) {
>> +		if (list->hiddev->exist) {
> 
> The order of checks is inverted. If the device no longer exists
> opening should fail, even if the device is already opened.
> Didn't I post a patch that does this? My memory is a bit clouded.

This is very old (and broken) version of the patch. I remember I already
commented some issues contained in this one and you did fix them later ;).