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@xxxxxxxxx>
> > > ---
> > >  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