linux-kernel - Re: [PATCH] usb: core: Warn if an URB's transfer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1704231155550.22469-100000@netrider.rowland.org>
Date:   Sun, 23 Apr 2017 12:01:14 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
cc:     Florian Fainelli <f.fainelli@...il.com>,
        <linux-kernel@...r.kernel.org>, <arnd@...db.de>,
        <maksim.salau@...il.com>, David Mosberger <davidm@...uge.net>,
        Roger Quadros <rogerq@...com>,
        Wolfram Sang <wsa-dev@...g-engineering.com>,
        Oliver Neukum <oneukum@...e.com>,
        Jaejoong Kim <climbbb.kim@...il.com>,
        "open list:USB SUBSYSTEM" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH] usb: core: Warn if an URB's transfer_buffer is on stack

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@...il.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