* [PATCH v2] usb: misc: legousbtower: Fix buffers on stack [not found] <CAE+v5Of10hZOEC8zyhVgszWmBaFzcMjqCGdLSDJEKjWwt9F+sg@mail.gmail.com> @ 2017-04-22 16:24 ` Maksim Salau 2017-04-25 18:04 ` Greg Kroah-Hartman 0 siblings, 1 reply; 9+ messages in thread From: Maksim Salau @ 2017-04-22 16:24 UTC (permalink / raw) To: Juergen Stuber, Greg Kroah-Hartman, legousb-devel, linux-usb, Alfredo Rafael Vicente Boix Cc: Maksim Salau, stable Allocate buffers on HEAP instead of STACK for local structures that are to be received using usb_control_msg(). Signed-off-by: Maksim Salau <maksim.salau@gmail.com> Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com> Cc: stable@vger.kernel.org --- Changes in v2: * made checkpatch happy with the format string passed to dev_info in tower_probe() (merged two parts into a single string literal); * changed commit message to better reflect location of the module; was: USB: legousbtower: Fix buffers on stack; * added Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com> and Cc: stable@vger.kernel.org drivers/usb/misc/legousbtower.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c index b10e26c..ed1999e 100644 --- a/drivers/usb/misc/legousbtower.c +++ b/drivers/usb/misc/legousbtower.c @@ -317,9 +317,16 @@ static int tower_open (struct inode *inode, struct file *file) int subminor; int retval = 0; struct usb_interface *interface; - struct tower_reset_reply reset_reply; + struct tower_reset_reply *reset_reply = NULL; int result; + reset_reply = kmalloc(sizeof(*reset_reply), GFP_KERNEL); + + if (!reset_reply) { + retval = -ENOMEM; + goto exit; + } + nonseekable_open(inode, file); subminor = iminor(inode); @@ -364,8 +371,8 @@ static int tower_open (struct inode *inode, struct file *file) USB_TYPE_VENDOR | USB_DIR_IN | USB_RECIP_DEVICE, 0, 0, - &reset_reply, - sizeof(reset_reply), + reset_reply, + sizeof(*reset_reply), 1000); if (result < 0) { dev_err(&dev->udev->dev, @@ -406,6 +413,7 @@ static int tower_open (struct inode *inode, struct file *file) mutex_unlock(&dev->lock); exit: + kfree(reset_reply); return retval; } @@ -808,7 +816,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device struct lego_usb_tower *dev = NULL; struct usb_host_interface *iface_desc; struct usb_endpoint_descriptor* endpoint; - struct tower_get_version_reply get_version_reply; + struct tower_get_version_reply *get_version_reply = NULL; int i; int retval = -ENOMEM; int result; @@ -886,6 +894,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), @@ -893,18 +908,19 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device USB_TYPE_VENDOR | USB_DIR_IN | USB_RECIP_DEVICE, 0, 0, - &get_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(&interface->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(&interface->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); @@ -928,6 +944,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device return retval; error: + kfree(get_version_reply); tower_delete(dev); return retval; } -- 2.9.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] usb: misc: legousbtower: Fix buffers on stack 2017-04-22 16:24 ` [PATCH v2] usb: misc: legousbtower: Fix buffers on stack Maksim Salau @ 2017-04-25 18:04 ` Greg Kroah-Hartman 2017-04-25 19:28 ` Maksim Salau 0 siblings, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2017-04-25 18:04 UTC (permalink / raw) To: Maksim Salau Cc: Juergen Stuber, legousb-devel, linux-usb, Alfredo Rafael Vicente Boix, stable On Sat, Apr 22, 2017 at 07:24:37PM +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 <maksim.salau@gmail.com> > Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com> > Cc: stable@vger.kernel.org This patch does not apply to usb-next, what did you make it against? Can you rebase it and resend? thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] usb: misc: legousbtower: Fix buffers on stack 2017-04-25 18:04 ` Greg Kroah-Hartman @ 2017-04-25 19:28 ` Maksim Salau 2017-04-25 19:49 ` [PATCH v3] " Maksim Salau 2017-04-26 9:28 ` [PATCH v2] " Greg Kroah-Hartman 0 siblings, 2 replies; 9+ messages in thread From: Maksim Salau @ 2017-04-25 19:28 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Juergen Stuber, legousb-devel, linux-usb, Alfredo Rafael Vicente Boix, stable On Tue, 25 Apr 2017 20:04:24 +0200 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Sat, Apr 22, 2017 at 07:24:37PM +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 <maksim.salau@gmail.com> > > Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com> > > Cc: stable@vger.kernel.org > > This patch does not apply to usb-next, what did you make it against? > > Can you rebase it and resend? > > thanks, > > greg k-h Hi Greg, I've created the patch against linux-stable v4.10.12. I've checked history of the driver and it turns out that the patch conflicts with 9b181166f17534a82b4b628b13e524a893715dfc https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/usb/misc/legousbtower.c?h=usb-next&id=9b181166f17534a82b4b628b13e524a893715dfc There are no conflicting changes, but context has changed. I'll rebase and resend shortly. The same patch can't be used for stable and for usb-next anymore. Is there any chance to backport the fix for stable? Regards, Maksim. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] usb: misc: legousbtower: Fix buffers on stack 2017-04-25 19:28 ` Maksim Salau @ 2017-04-25 19:49 ` Maksim Salau 2017-04-26 9:29 ` Greg Kroah-Hartman 2017-05-04 12:36 ` Heikki Krogerus 2017-04-26 9:28 ` [PATCH v2] " Greg Kroah-Hartman 1 sibling, 2 replies; 9+ messages in thread From: Maksim Salau @ 2017-04-25 19:49 UTC (permalink / raw) To: Juergen Stuber, Greg Kroah-Hartman, legousb-devel, linux-usb, linux-kernel, Alfredo Rafael Vicente Boix Cc: Maksim Salau Allocate buffers on HEAP instead of STACK for local structures that are to be received using usb_control_msg(). Signed-off-by: Maksim Salau <maksim.salau@gmail.com> --- Changes in v3: * rebased against usb-next; * removed Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com>; * removed Cc: stable@vger.kernel.org since this patch doesn't apply against v4.10.12 Changes in v2: * made checkpatch happy with the format string passed to dev_info in tower_probe() (merged two parts into a single string literal); * changed commit message to better reflect location of the module; was: USB: legousbtower: Fix buffers on stack; * added Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com> and Cc: stable@vger.kernel.org drivers/usb/misc/legousbtower.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c index 201c9c3..aa3c280 100644 --- a/drivers/usb/misc/legousbtower.c +++ b/drivers/usb/misc/legousbtower.c @@ -317,9 +317,16 @@ static int tower_open (struct inode *inode, struct file *file) int subminor; int retval = 0; struct usb_interface *interface; - struct tower_reset_reply reset_reply; + struct tower_reset_reply *reset_reply; int result; + reset_reply = kmalloc(sizeof(*reset_reply), GFP_KERNEL); + + if (!reset_reply) { + retval = -ENOMEM; + goto exit; + } + nonseekable_open(inode, file); subminor = iminor(inode); @@ -364,8 +371,8 @@ static int tower_open (struct inode *inode, struct file *file) USB_TYPE_VENDOR | USB_DIR_IN | USB_RECIP_DEVICE, 0, 0, - &reset_reply, - sizeof(reset_reply), + reset_reply, + sizeof(*reset_reply), 1000); if (result < 0) { dev_err(&dev->udev->dev, @@ -406,6 +413,7 @@ static int tower_open (struct inode *inode, struct file *file) mutex_unlock(&dev->lock); exit: + kfree(reset_reply); return retval; } @@ -806,7 +814,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device struct device *idev = &interface->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, - &get_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(&interface->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(&interface->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 return retval; error: + kfree(get_version_reply); tower_delete(dev); return retval; } -- 2.9.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack 2017-04-25 19:49 ` [PATCH v3] " Maksim Salau @ 2017-04-26 9:29 ` Greg Kroah-Hartman 2017-04-27 6:30 ` Maksim Salau 2017-05-04 12:36 ` Heikki Krogerus 1 sibling, 1 reply; 9+ messages in thread From: Greg Kroah-Hartman @ 2017-04-26 9:29 UTC (permalink / raw) To: Maksim Salau Cc: Juergen Stuber, legousb-devel, linux-usb, linux-kernel, Alfredo Rafael Vicente Boix 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 <maksim.salau@gmail.com> > Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com>; > Cc: stable <stable@vger.kernel.org> > > --- > Changes in v3: > * rebased against usb-next; > * removed Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com>; I added this back, as it matters, and your change from the previous version was trivial. > * removed Cc: stable@vger.kernel.org > since this patch doesn't apply against v4.10.12 I added this back as well :) thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack 2017-04-26 9:29 ` Greg Kroah-Hartman @ 2017-04-27 6:30 ` Maksim Salau 0 siblings, 0 replies; 9+ messages in thread From: Maksim Salau @ 2017-04-27 6:30 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Juergen Stuber, legousb-devel, linux-usb, linux-kernel, Alfredo Rafael Vicente Boix > > * removed Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com>; > > I added this back, as it matters, and your change from the previous > version was trivial. > > > * removed Cc: stable@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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack 2017-04-25 19:49 ` [PATCH v3] " Maksim Salau 2017-04-26 9:29 ` Greg Kroah-Hartman @ 2017-05-04 12:36 ` Heikki Krogerus 2017-05-04 19:11 ` Maksim Salau 1 sibling, 1 reply; 9+ messages in thread From: Heikki Krogerus @ 2017-05-04 12:36 UTC (permalink / raw) To: Maksim Salau Cc: Juergen Stuber, Greg Kroah-Hartman, legousb-devel, linux-usb, linux-kernel, Alfredo Rafael Vicente Boix 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 = &interface->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, > - &get_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(&interface->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(&interface->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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] usb: misc: legousbtower: Fix buffers on stack 2017-05-04 12:36 ` Heikki Krogerus @ 2017-05-04 19:11 ` Maksim Salau 0 siblings, 0 replies; 9+ messages in thread From: Maksim Salau @ 2017-05-04 19:11 UTC (permalink / raw) To: Heikki Krogerus Cc: Juergen Stuber, Greg Kroah-Hartman, legousb-devel, linux-usb, linux-kernel, Alfredo Rafael Vicente Boix > > @@ -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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] usb: misc: legousbtower: Fix buffers on stack 2017-04-25 19:28 ` Maksim Salau 2017-04-25 19:49 ` [PATCH v3] " Maksim Salau @ 2017-04-26 9:28 ` Greg Kroah-Hartman 1 sibling, 0 replies; 9+ messages in thread From: Greg Kroah-Hartman @ 2017-04-26 9:28 UTC (permalink / raw) To: Maksim Salau Cc: Juergen Stuber, legousb-devel, linux-usb, Alfredo Rafael Vicente Boix, stable On Tue, Apr 25, 2017 at 10:28:42PM +0300, Maksim Salau wrote: > On Tue, 25 Apr 2017 20:04:24 +0200 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > On Sat, Apr 22, 2017 at 07:24:37PM +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 <maksim.salau@gmail.com> > > > Tested-by: Alfredo Rafael Vicente Boix <alviboi@gmail.com> > > > Cc: stable@vger.kernel.org > > > > This patch does not apply to usb-next, what did you make it against? > > > > Can you rebase it and resend? > > > > thanks, > > > > greg k-h > > Hi Greg, > > I've created the patch against linux-stable v4.10.12. I've checked history > of the driver and it turns out that the patch conflicts with > 9b181166f17534a82b4b628b13e524a893715dfc > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/drivers/usb/misc/legousbtower.c?h=usb-next&id=9b181166f17534a82b4b628b13e524a893715dfc > > There are no conflicting changes, but context has changed. > I'll rebase and resend shortly. > > The same patch can't be used for stable and for usb-next anymore. Is there > any chance to backport the fix for stable? Yes, we can backport it, I'll mark it for stable and do the backport when it hits Linus's tree after 4.12-rc1 is out. thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-05-04 19:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CAE+v5Of10hZOEC8zyhVgszWmBaFzcMjqCGdLSDJEKjWwt9F+sg@mail.gmail.com> 2017-04-22 16:24 ` [PATCH v2] usb: misc: legousbtower: Fix buffers on stack Maksim Salau 2017-04-25 18:04 ` Greg Kroah-Hartman 2017-04-25 19:28 ` Maksim Salau 2017-04-25 19:49 ` [PATCH v3] " Maksim Salau 2017-04-26 9:29 ` Greg Kroah-Hartman 2017-04-27 6:30 ` Maksim Salau 2017-05-04 12:36 ` Heikki Krogerus 2017-05-04 19:11 ` Maksim Salau 2017-04-26 9:28 ` [PATCH v2] " Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.