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

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

[Florian Fainelli]

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

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

[Clemens Ladisch]

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

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

[Alan Stern]

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,
> 

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

[Maksim Salau]

> +			} 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.

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

[Greg Kroah-Hartman]

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

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

[Felipe Balbi]

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