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

[PATCH] 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_start_wait_urb() for
urb->transfer_buffer to be located on the stack.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/usb/core/message.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 2184ef40a82a..abefddbe9243 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -8,6 +8,7 @@
 #include <linux/pci.h>	/* for scatterlist macros */
 #include <linux/usb.h>
 #include <linux/module.h>
+#include <linux/sched/task_stack.h> /* for object_is_on_stack */
 #include <linux/slab.h>
 #include <linux/mm.h>
 #include <linux/timer.h>
@@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
 	unsigned long expire;
 	int retval;
 
+	WARN_ON(object_is_on_stack(urb->transfer_buffer));
+
 	init_completion(&ctx.done);
 	urb->context = &ctx;
 	urb->actual_length = 0;
-- 
2.11.0

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

[Alan Stern]

On Sat, 22 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_start_wait_urb() for
> urb->transfer_buffer to be located on the stack.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/usb/core/message.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 2184ef40a82a..abefddbe9243 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -8,6 +8,7 @@
>  #include <linux/pci.h>	/* for scatterlist macros */
>  #include <linux/usb.h>
>  #include <linux/module.h>
> +#include <linux/sched/task_stack.h> /* for object_is_on_stack */
>  #include <linux/slab.h>
>  #include <linux/mm.h>
>  #include <linux/timer.h>
> @@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
>  	unsigned long expire;
>  	int retval;
>  
> +	WARN_ON(object_is_on_stack(urb->transfer_buffer));
> +
>  	init_completion(&ctx.done);
>  	urb->context = &ctx;
>  	urb->actual_length = 0;

Does this really help?  I mean, don't we already get warnings when 
the host controller drivers try to map on-stack buffers for DMA?

The only difference is that one wouldn't have to read as far back into
the stack trace.  But that hardly seems like an important
consideration.

Alan Stern

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

[Greg Kroah-Hartman]

On Sat, Apr 22, 2017 at 05:31:27PM -0400, Alan Stern wrote:
> On Sat, 22 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_start_wait_urb() for
> > urb->transfer_buffer to be located on the stack.
> > 
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> >  drivers/usb/core/message.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > index 2184ef40a82a..abefddbe9243 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/pci.h>	/* for scatterlist macros */
> >  #include <linux/usb.h>
> >  #include <linux/module.h>
> > +#include <linux/sched/task_stack.h> /* for object_is_on_stack */
> >  #include <linux/slab.h>
> >  #include <linux/mm.h>
> >  #include <linux/timer.h>
> > @@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
> >  	unsigned long expire;
> >  	int retval;
> >  
> > +	WARN_ON(object_is_on_stack(urb->transfer_buffer));
> > +
> >  	init_completion(&ctx.done);
> >  	urb->context = &ctx;
> >  	urb->actual_length = 0;
> 
> Does this really help?  I mean, don't we already get warnings when 
> the host controller drivers try to map on-stack buffers for DMA?
> 
> The only difference is that one wouldn't have to read as far back into
> the stack trace.  But that hardly seems like an important
> consideration.

I don't think this will show up if you don't have the VMALLOC_STACKS
option enabled (or whatever it is), so this warning is good to have.  I
didn't know we had that macro, as the USB stack has always required this
due to some platforms needing it, just not the "mainstream" ones...

I'll take this for the next kernel (after 4.12-rc1) and see what shakes
out in linux-next...

thanks,

greg k-h

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

[Alan Stern]

On Sun, 23 Apr 2017, Greg Kroah-Hartman wrote:

> On Sat, Apr 22, 2017 at 05:31:27PM -0400, Alan Stern wrote:
> > On Sat, 22 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_start_wait_urb() for
> > > urb->transfer_buffer to be located on the stack.
> > > 
> > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > ---
> > >  drivers/usb/core/message.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> > > index 2184ef40a82a..abefddbe9243 100644
> > > --- a/drivers/usb/core/message.c
> > > +++ b/drivers/usb/core/message.c
> > > @@ -8,6 +8,7 @@
> > >  #include <linux/pci.h>	/* for scatterlist macros */
> > >  #include <linux/usb.h>
> > >  #include <linux/module.h>
> > > +#include <linux/sched/task_stack.h> /* for object_is_on_stack */
> > >  #include <linux/slab.h>
> > >  #include <linux/mm.h>
> > >  #include <linux/timer.h>
> > > @@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
> > >  	unsigned long expire;
> > >  	int retval;
> > >  
> > > +	WARN_ON(object_is_on_stack(urb->transfer_buffer));
> > > +
> > >  	init_completion(&ctx.done);
> > >  	urb->context = &ctx;
> > >  	urb->actual_length = 0;
> > 
> > Does this really help?  I mean, don't we already get warnings when 
> > the host controller drivers try to map on-stack buffers for DMA?
> > 
> > The only difference is that one wouldn't have to read as far back into
> > the stack trace.  But that hardly seems like an important
> > consideration.
> 
> I don't think this will show up if you don't have the VMALLOC_STACKS
> option enabled (or whatever it is), so this warning is good to have.  I
> didn't know we had that macro, as the USB stack has always required this
> due to some platforms needing it, just not the "mainstream" ones...

In that case, it would be better to move the warning to a central place
where it will always get triggered, such as map_urb_for_dma().  As it
is, the patch will only issue a warning for callers of usb_bulk_msg(),
usb_interrupt_msg(), or usb_control_msg().

Alan Stern

> I'll take this for the next kernel (after 4.12-rc1) and see what shakes
> out in linux-next...
> 
> thanks,
> 
> greg k-h

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

[Florian Fainelli]

On 04/23/2017 09:01 AM, Alan Stern wrote:
> On Sun, 23 Apr 2017, Greg Kroah-Hartman wrote:
> 
>> On Sat, Apr 22, 2017 at 05:31:27PM -0400, Alan Stern wrote:
>>> On Sat, 22 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_start_wait_urb() for
>>>> urb->transfer_buffer to be located on the stack.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>>  drivers/usb/core/message.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>>>> index 2184ef40a82a..abefddbe9243 100644
>>>> --- a/drivers/usb/core/message.c
>>>> +++ b/drivers/usb/core/message.c
>>>> @@ -8,6 +8,7 @@
>>>>  #include <linux/pci.h>	/* for scatterlist macros */
>>>>  #include <linux/usb.h>
>>>>  #include <linux/module.h>
>>>> +#include <linux/sched/task_stack.h> /* for object_is_on_stack */
>>>>  #include <linux/slab.h>
>>>>  #include <linux/mm.h>
>>>>  #include <linux/timer.h>
>>>> @@ -50,6 +51,8 @@ static int usb_start_wait_urb(struct urb *urb, int timeout, int *actual_length)
>>>>  	unsigned long expire;
>>>>  	int retval;
>>>>  
>>>> +	WARN_ON(object_is_on_stack(urb->transfer_buffer));
>>>> +
>>>>  	init_completion(&ctx.done);
>>>>  	urb->context = &ctx;
>>>>  	urb->actual_length = 0;
>>>
>>> Does this really help?  I mean, don't we already get warnings when 
>>> the host controller drivers try to map on-stack buffers for DMA?
>>>
>>> The only difference is that one wouldn't have to read as far back into
>>> the stack trace.  But that hardly seems like an important
>>> consideration.
>>
>> I don't think this will show up if you don't have the VMALLOC_STACKS
>> option enabled (or whatever it is), so this warning is good to have.  I
>> didn't know we had that macro, as the USB stack has always required this
>> due to some platforms needing it, just not the "mainstream" ones...
> 
> In that case, it would be better to move the warning to a central place
> where it will always get triggered, such as map_urb_for_dma().  As it
> is, the patch will only issue a warning for callers of usb_bulk_msg(),
> usb_interrupt_msg(), or usb_control_msg().

map_urb_for_dma() seems like a better location indeed, let me submit a
v2 with that then.

Most HCDs implementation that define a map_urb_for_dma function pointer
also call usb_hcd_map_urb_for_dma() (except for max3421-hcd.c) so would
it be better to move the is_object_on_stack() check in
usb_hcd_map_urb_for_dma() for consistency with the existing
is_vmalloc_addr() check?

Thanks!

NB: initially, I was looking for a compile-time warning, and first there
does not seem to be a warning available in GCC that could tell you if a
variable is on the stack (why would there be?) and second, we'd have to
annotate functions that require some of their arguments to come from the
heap anyway. The amount of work needed to annotate and fix these
build-time warnings versus looking at reports and fixing warnings
end-ups being pretty much the same anyway.
-- 
Florian

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

[Alan Stern]

On Sun, 23 Apr 2017, Florian Fainelli wrote:

> > In that case, it would be better to move the warning to a central place
> > where it will always get triggered, such as map_urb_for_dma().  As it
> > is, the patch will only issue a warning for callers of usb_bulk_msg(),
> > usb_interrupt_msg(), or usb_control_msg().
> 
> map_urb_for_dma() seems like a better location indeed, let me submit a
> v2 with that then.
> 
> Most HCDs implementation that define a map_urb_for_dma function pointer
> also call usb_hcd_map_urb_for_dma() (except for max3421-hcd.c) so would
> it be better to move the is_object_on_stack() check in
> usb_hcd_map_urb_for_dma() for consistency with the existing
> is_vmalloc_addr() check?

Or maybe move the is_vmalloc_addr() check to map_urb_for_dma().

The only issue is that some weird HCD might sometimes not need to do
the DMA mapping (it might use PIO for some types of transfer).  In
those cases we don't want to issue the warning.  So perhaps 
usb_hcd_map_urb_for_dma() would be better for both checks.

Alan Stern