* [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 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
* 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
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.