[2.4] Memleak in drivers/usb/hub.c::usb_reset_device

[2.4] Memleak in drivers/usb/hub.c::usb_reset_device

[Oleg Drokin]

Hello!

   There seems to be a memleak in drivers/usb/hub.c::usb_reset_device()
   on error exit path. See the patch.
   Found with help of smatch + enhanced unfree script.

Bye,
    Oleg
===== drivers/usb/hub.c 1.19 vs edited =====
--- 1.19/drivers/usb/hub.c	Sat Sep 21 00:12:53 2002
+++ edited/drivers/usb/hub.c	Wed Mar 12 22:38:43 2003
@@ -1057,8 +1057,10 @@
 	}
 	ret = usb_get_descriptor(dev, USB_DT_DEVICE, 0, descriptor,
 			sizeof(*descriptor));
-	if (ret < 0)
+	if (ret < 0) {
+		kfree(descriptor);
 		return ret;
+	}
 
 	le16_to_cpus(&descriptor->bcdUSB);
 	le16_to_cpus(&descriptor->idVendor);

Re: [2.4] Memleak in drivers/usb/hub.c::usb_reset_device

[David Brownell]

Oleg Drokin wrote:
> Hello!
> 
>    There seems to be a memleak in drivers/usb/hub.c::usb_reset_device()
>    on error exit path. See the patch.
>    Found with help of smatch + enhanced unfree script.

Hmm ... and 2.5 allocates it on the stack, which is actually
illegal (DMA-to-stack is nonportable).  This looks like a case
where it'd be good to make 2.5 look more like 2.4 (+ this patch).

- Dave


> Bye,
>     Oleg
> ===== drivers/usb/hub.c 1.19 vs edited =====
> --- 1.19/drivers/usb/hub.c	Sat Sep 21 00:12:53 2002
> +++ edited/drivers/usb/hub.c	Wed Mar 12 22:38:43 2003
> @@ -1057,8 +1057,10 @@
>  	}
>  	ret = usb_get_descriptor(dev, USB_DT_DEVICE, 0, descriptor,
>  			sizeof(*descriptor));
> -	if (ret < 0)
> +	if (ret < 0) {
> +		kfree(descriptor);
>  		return ret;
> +	}
>  
>  	le16_to_cpus(&descriptor->bcdUSB);
>  	le16_to_cpus(&descriptor->idVendor);
> 

Re: [2.4] Memleak in drivers/usb/hub.c::usb_reset_device

[Greg KH]

On Wed, Mar 12, 2003 at 10:41:33PM +0300, Oleg Drokin wrote:
> Hello!
> 
>    There seems to be a memleak in drivers/usb/hub.c::usb_reset_device()
>    on error exit path. See the patch.
>    Found with help of smatch + enhanced unfree script.

Applied to my tree, thanks.

And yes, as David said, there is another kind of error in this area for
2.5.  Patches to clean that up would be appreciated.

thanks,

greg k-h

Re: [2.4] Memleak in drivers/usb/hub.c::usb_reset_device

[Oleg Drokin]

Hello!

On Fri, Mar 14, 2003 at 11:37:19AM -0800, Greg KH wrote:
> >    There seems to be a memleak in drivers/usb/hub.c::usb_reset_device()
> >    on error exit path. See the patch.
> >    Found with help of smatch + enhanced unfree script.
> And yes, as David said, there is another kind of error in this area for
> 2.5.  Patches to clean that up would be appreciated.

Ok, I guess something like that should work:

Bye,
    Oleg
===== drivers/usb/core/hub.c 1.57 vs edited =====
--- 1.57/drivers/usb/core/hub.c	Wed Mar  5 18:24:34 2003
+++ edited/drivers/usb/core/hub.c	Fri Mar 14 22:59:45 2003
@@ -1175,7 +1175,7 @@
 int usb_reset_device(struct usb_device *dev)
 {
 	struct usb_device *parent = dev->parent;
-	struct usb_device_descriptor descriptor;
+	struct usb_device_descriptor *descriptor;
 	int i, ret, port = -1;
 
 	if (!parent) {
@@ -1224,17 +1224,24 @@
 	 * If nothing changed, we reprogram the configuration and then
 	 * the alternate settings.
 	 */
-	ret = usb_get_descriptor(dev, USB_DT_DEVICE, 0, &descriptor,
-			sizeof(descriptor));
-	if (ret < 0)
+	descriptor = kmalloc(sizeof *descriptor, GFP_NOIO);
+	if (!descriptor) {
+		return -ENOMEM;
+	}
+	ret = usb_get_descriptor(dev, USB_DT_DEVICE, 0, descriptor,
+			sizeof(*descriptor));
+	if (ret < 0) {
+		kfree(descriptor);
 		return ret;
+	}
 
-	le16_to_cpus(&descriptor.bcdUSB);
-	le16_to_cpus(&descriptor.idVendor);
-	le16_to_cpus(&descriptor.idProduct);
-	le16_to_cpus(&descriptor.bcdDevice);
+	le16_to_cpus(&descriptor->bcdUSB);
+	le16_to_cpus(&descriptor->idVendor);
+	le16_to_cpus(&descriptor->idProduct);
+	le16_to_cpus(&descriptor->bcdDevice);
 
-	if (memcmp(&dev->descriptor, &descriptor, sizeof(descriptor))) {
+	if (memcmp(&dev->descriptor, descriptor, sizeof(*descriptor))) {
+		kfree(descriptor);
 		usb_destroy_configuration(dev);
 
 		ret = usb_get_device_descriptor(dev);
@@ -1267,6 +1274,8 @@
 
 		return 1;
 	}
+
+	kfree(descriptor);
 
 	ret = usb_set_configuration(dev, dev->actconfig->desc.bConfigurationValue);
 	if (ret < 0) {

Re: [2.4] Memleak in drivers/usb/hub.c::usb_reset_device

[Greg KH]

On Fri, Mar 14, 2003 at 11:02:04PM +0300, Oleg Drokin wrote:
> Hello!
> 
> On Fri, Mar 14, 2003 at 11:37:19AM -0800, Greg KH wrote:
> > >    There seems to be a memleak in drivers/usb/hub.c::usb_reset_device()
> > >    on error exit path. See the patch.
> > >    Found with help of smatch + enhanced unfree script.
> > And yes, as David said, there is another kind of error in this area for
> > 2.5.  Patches to clean that up would be appreciated.
> 
> Ok, I guess something like that should work:

I've applied this to my trees, thanks.

greg k-h