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@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().

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