Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack

[Maksim Salau]

> > @@ -913,6 +929,7 @@ static int tower_probe (struct usb_interface 
> > *interface, const struct usb_device  
> 
> Don't you need to free get_version_reply here?
> 
> > return retval;
> >  
> >  error:
> > +   kfree(get_version_reply);
> > tower_delete(dev);
> > return retval;
> >  }  

Thank you very much, Heikki!

I was so focused on failure cases, that missed memory leak in the case
 when all calls succeeded.

I'll prepare a patch shortly to fix this.

Regards,
Maksim.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack

[Heikki Krogerus]

Hi Maksim,

Sorry for commenting this so late but..

On Tue, Apr 25, 2017 at 10:49:21PM +0300, Maksim Salau wrote:
> @@ -806,7 +814,7 @@ static int tower_probe (struct usb_interface *interface, 
> const struct usb_device
>   struct device *idev = >dev;
>   struct usb_device *udev = interface_to_usbdev(interface);
>   struct lego_usb_tower *dev = NULL;
> - struct tower_get_version_reply get_version_reply;
> + struct tower_get_version_reply *get_version_reply = NULL;
>   int retval = -ENOMEM;
>   int result;
>  
> @@ -871,6 +879,13 @@ static int tower_probe (struct usb_interface *interface, 
> const struct usb_device
>   dev->interrupt_in_interval = interrupt_in_interval ? 
> interrupt_in_interval : dev->interrupt_in_endpoint->bInterval;
>   dev->interrupt_out_interval = interrupt_out_interval ? 
> interrupt_out_interval : dev->interrupt_out_endpoint->bInterval;
>  
> + get_version_reply = kmalloc(sizeof(*get_version_reply), GFP_KERNEL);
> +
> + if (!get_version_reply) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
>   /* get the firmware version and log it */
>   result = usb_control_msg (udev,
> usb_rcvctrlpipe(udev, 0),
> @@ -878,18 +893,19 @@ static int tower_probe (struct usb_interface 
> *interface, const struct usb_device
> USB_TYPE_VENDOR | USB_DIR_IN | 
> USB_RECIP_DEVICE,
> 0,
> 0,
> -   _version_reply,
> -   sizeof(get_version_reply),
> +   get_version_reply,
> +   sizeof(*get_version_reply),
> 1000);
>   if (result < 0) {
>   dev_err(idev, "LEGO USB Tower get version control request 
> failed\n");
>   retval = result;
>   goto error;
>   }
> - dev_info(>dev, "LEGO USB Tower firmware version is %d.%d "
> -  "build %d\n", get_version_reply.major,
> -  get_version_reply.minor,
> -  le16_to_cpu(get_version_reply.build_no));
> + dev_info(>dev,
> +  "LEGO USB Tower firmware version is %d.%d build %d\n",
> +  get_version_reply->major,
> +  get_version_reply->minor,
> +  le16_to_cpu(get_version_reply->build_no));
>  
>   /* we can register the device now, as it is ready */
>   usb_set_intfdata (interface, dev);
> @@ -913,6 +929,7 @@ static int tower_probe (struct usb_interface *interface, 
> const struct usb_device

Don't you need to free get_version_reply here?

>   return retval;
>  
>  error:
> + kfree(get_version_reply);
>   tower_delete(dev);
>   return retval;
>  }


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack

[Maksim Salau]

> >* removed Tested-by: Alfredo Rafael Vicente Boix ;
> 
> I added this back, as it matters, and your change from the previous
> version was trivial.
>   
> >* removed Cc: sta...@vger.kernel.org
> >  since this patch doesn't apply against v4.10.12
> 
> I added this back as well :)  

Thanks, Greg!

I was not sure about how strict are the rules about these tags.

Maksim.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack

[Greg Kroah-Hartman]

On Tue, Apr 25, 2017 at 10:49:21PM +0300, Maksim Salau wrote:
> Allocate buffers on HEAP instead of STACK for local structures
> that are to be received using usb_control_msg().
> 
> Signed-off-by: Maksim Salau 
> Tested-by: Alfredo Rafael Vicente Boix ;
> Cc: stable 
> 
> ---
>   Changes in v3:
>* rebased against usb-next;
>* removed Tested-by: Alfredo Rafael Vicente Boix ;

I added this back, as it matters, and your change from the previous
version was trivial.

>* removed Cc: sta...@vger.kernel.org
>  since this patch doesn't apply against v4.10.12

I added this back as well :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html