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

From: Alan Stern
Date: Sun Apr 23 2017 - 12:01:31 EST


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