Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1629118 > unrolled thread

[PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack Florian Fainelli <f.fainelli@gmail.com> - 2017-04-24 03:40 +0200
    Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on  stack Clemens Ladisch <clemens@ladisch.de> - 2017-04-24 09:30 +0200
    Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on  stack Alan Stern <stern@rowland.harvard.edu> - 2017-04-24 15:30 +0200
    Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on  stack Maksim Salau <maksim.salau@gmail.com> - 2017-04-25 12:40 +0200
      Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on  stack Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-04-25 14:00 +0200
        Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack Felipe Balbi <felipe.balbi@linux.intel.com> - 2017-04-25 15:30 +0200

#1629118 — [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack

We see a large number of fixes to several drivers to remove the usage of
on-stack buffers feeding into USB transfer functions. Make it easier to spot
the offenders by adding a warning in usb_hcd_map_urb_for_dma() checking that
urb->transfer_buffer is not a stack object.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()

 drivers/usb/core/hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 49550790a3cb..ce9063ce906a 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/version.h>
 #include <linux/kernel.h>
+#include <linux/sched/task_stack.h>
 #include <linux/slab.h>
 #include <linux/completion.h>
 #include <linux/utsname.h>
@@ -1587,6 +1588,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 			} else if (is_vmalloc_addr(urb->transfer_buffer)) {
 				WARN_ONCE(1, "transfer buffer not dma capable\n");
 				ret = -EAGAIN;
+			} else if (object_is_on_stack(urb->transfer_buffer)) {
+				WARN_ONCE(1, "transfer buffer is on stack\n");
+				ret = -EAGAIN;
 			} else {
 				urb->transfer_dma = dma_map_single(
 						hcd->self.sysdev,
-- 
2.11.0

[toc] | [next] | [standalone]

#1629264 — Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack

Florian Fainelli wrote:
> We see a large number of fixes to several drivers to remove the usage of
> on-stack buffers feeding into USB transfer functions. Make it easier to spot
> the offenders by adding a warning in usb_hcd_map_urb_for_dma() checking that
> urb->transfer_buffer is not a stack object.

This description is incomplete.

> +			} else if (object_is_on_stack(urb->transfer_buffer)) {
> +				WARN_ONCE(1, "transfer buffer is on stack\n");
> +				ret = -EAGAIN;
>  			} else {
>  				urb->transfer_dma = dma_map_single(

Not only is there a warning, but the check also forces all those URBs
to abort with an error.

Well, that makes it even easier to spot the offenders ...


Regards,
Clemens

[toc] | [prev] | [next] | [standalone]

#1629554 — Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack

On Sun, 23 Apr 2017, Florian Fainelli wrote:

> We see a large number of fixes to several drivers to remove the usage of
> on-stack buffers feeding into USB transfer functions. Make it easier to spot
> the offenders by adding a warning in usb_hcd_map_urb_for_dma() checking that
> urb->transfer_buffer is not a stack object.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v2:
> 
> - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()

You probably should add a similar check to the pathway that maps 
urb->setup_packet, for the sake of consistency.

Alan Stern

>  drivers/usb/core/hcd.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 49550790a3cb..ce9063ce906a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -26,6 +26,7 @@
>  #include <linux/module.h>
>  #include <linux/version.h>
>  #include <linux/kernel.h>
> +#include <linux/sched/task_stack.h>
>  #include <linux/slab.h>
>  #include <linux/completion.h>
>  #include <linux/utsname.h>
> @@ -1587,6 +1588,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  			} else if (is_vmalloc_addr(urb->transfer_buffer)) {
>  				WARN_ONCE(1, "transfer buffer not dma capable\n");
>  				ret = -EAGAIN;
> +			} else if (object_is_on_stack(urb->transfer_buffer)) {
> +				WARN_ONCE(1, "transfer buffer is on stack\n");
> +				ret = -EAGAIN;
>  			} else {
>  				urb->transfer_dma = dma_map_single(
>  						hcd->self.sysdev,
> 

[toc] | [prev] | [next] | [standalone]

#1630390 — Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack

> +			} else if (object_is_on_stack(urb->transfer_buffer)) {
> +				WARN_ONCE(1, "transfer buffer is on stack\n");
> +				ret = -EAGAIN;
>  			} else {

Hi,

Has anyone considered a fail-safe mode? I.e.: if a buffer is on stack,
kmemdup it and continue with a warning. This will give us both: functional
drivers (with possibly decreased efficiency in speed and memory footprint)
and warnings for developers that a particular driver requires attention.

This mode will not affect drivers which obey the rules, but will make
offenders at least functional. My main concern is that not every user is able
to detect and report a problem, which prevents drivers from functioning.
Especially this is a problem for not wide spread devices.
Due to this users a seeing unusable equipment, but developers are not
aware of those, even if fixes are trivial.

Such mode has a also a negative effect: if a developer has a device
with an offending driver, he can miss the warning message, since the driver
just works.

Regards,
Maksim.

[toc] | [prev] | [next] | [standalone]

#1630433 — Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack

On Tue, Apr 25, 2017 at 01:35:33PM +0300, Maksim Salau wrote:
> > +			} else if (object_is_on_stack(urb->transfer_buffer)) {
> > +				WARN_ONCE(1, "transfer buffer is on stack\n");
> > +				ret = -EAGAIN;
> >  			} else {
> 
> Hi,
> 
> Has anyone considered a fail-safe mode? I.e.: if a buffer is on stack,
> kmemdup it and continue with a warning. This will give us both: functional
> drivers (with possibly decreased efficiency in speed and memory footprint)
> and warnings for developers that a particular driver requires attention.

No, I do not want that, let's fix the drivers.

> This mode will not affect drivers which obey the rules, but will make
> offenders at least functional. My main concern is that not every user is able
> to detect and report a problem, which prevents drivers from functioning.
> Especially this is a problem for not wide spread devices.
> Due to this users a seeing unusable equipment, but developers are not
> aware of those, even if fixes are trivial.
> 
> Such mode has a also a negative effect: if a developer has a device
> with an offending driver, he can miss the warning message, since the driver
> just works.

Exactly, let's fix the bugs.  These have been bugs for 10+ years now,
they should get fixed, it's not complex :)

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1630505

Hi,

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Tue, Apr 25, 2017 at 01:35:33PM +0300, Maksim Salau wrote:
>> > +			} else if (object_is_on_stack(urb->transfer_buffer)) {
>> > +				WARN_ONCE(1, "transfer buffer is on stack\n");
>> > +				ret = -EAGAIN;
>> >  			} else {
>> 
>> Hi,
>> 
>> Has anyone considered a fail-safe mode? I.e.: if a buffer is on stack,
>> kmemdup it and continue with a warning. This will give us both: functional
>> drivers (with possibly decreased efficiency in speed and memory footprint)
>> and warnings for developers that a particular driver requires attention.
>
> No, I do not want that, let's fix the drivers.
>
>> This mode will not affect drivers which obey the rules, but will make
>> offenders at least functional. My main concern is that not every user is able
>> to detect and report a problem, which prevents drivers from functioning.
>> Especially this is a problem for not wide spread devices.
>> Due to this users a seeing unusable equipment, but developers are not
>> aware of those, even if fixes are trivial.
>> 
>> Such mode has a also a negative effect: if a developer has a device
>> with an offending driver, he can miss the warning message, since the driver
>> just works.
>
> Exactly, let's fix the bugs.  These have been bugs for 10+ years now,
> they should get fixed, it's not complex :)

We should probably have a similar patch on
drivers/usb/gadget/udc/core.c::usb_gadget_map_request_by_dev()

-- 
balbi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web