linux-kernel - Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

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: <dd2b5c78-4ed6-319a-9531-c0641af33a66@ti.com>
Date:   Wed, 31 May 2017 17:26:09 +0530
From:   Vignesh R <vigneshr@...com>
To:     Wolfram Sang <wsa@...-dreams.de>,
        Florian Fainelli <f.fainelli@...il.com>
CC:     <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <balbi@...nel.org>,
        Peter Chen <peter.chen@....com>, Roger Quadros <rogerq@...com>,
        Alan Stern <stern@...land.harvard.edu>,
        Mathias Nyman <mathias.nyman@...ux.intel.com>,
        Javier Martinez Canillas <javier@....samsung.com>,
        Baoyou Xie <baoyou.xie@...aro.org>,
        Sekhar Nori <nsekhar@...com>, William wu <wulf@...k-chips.com>,
        Arnd Bergmann <arnd@...db.de>,
        Chris Bainbridge <chris.bainbridge@...il.com>,
        Wolfram Sang <wsa-dev@...g-engineering.com>,
        Krzysztof Opasiak <k.opasiak@...sung.com>,
        Felix Hädicke <felixhaedicke@....de>,
        Colin Ian King <colin.king@...onical.com>,
        "open list:USB SUBSYSTEM" <linux-usb@...r.kernel.org>,
        <clemens@...isch.de>, <maksim.salau@...il.com>
Subject: Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity



On Sunday 28 May 2017 09:33 PM, Wolfram Sang wrote:
> On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
>> On 04/25/2017 05:56 PM, Florian Fainelli wrote:
>>> Changes in v3:
>>>
>>> - added check in usb_gadget_map_request_by_dev (Felipe), new patch
>>> - improved commit message description (Clemens)
>>> - added additiona checks for urb->setup_packet (Alan)
>>>
>>> Changes in v2:
>>>
>>> - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
>>
>> Is this version looking good now? Thanks!
> 
> So, it seems I am in a similar situation with the I2C subsystem right
> now. I need to check the message buffers if they are DMA capable.
> 
> Because you have basically the same checks in 3 different places, and I
> need something similar for I2C, I wondered about a generic place to put
> these checks. Especially since we want future improvements to these
> checks applied everywhere immediately. Here is a small diff on what I
> have now:
> 
> ===
> 
> dma-mapping(?): introduce helper to check for DMA capable addresses
> 
> Introduce a helper to check if an address is DMA capable. Such a check
> is subtle, so it is good to have a centralized place for it.
> 
> Note: I am absolutely not sure if dma-mapping.h is a good place for such
> a function. I just couldn't think of a better one for now.
> 
> Second note: I am not even sure the checks are complete (kmapped mem?).
> I am not an MM expert. But that just strengthens the argument of having
> on centralized place IMO.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@...g-engineering.com>
> 
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 5dea98358c05c4..777a37b395ff19 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1584,7 +1584,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  					ret = -EAGAIN;
>  				else
>  					urb->transfer_flags |= URB_DMA_MAP_PAGE;
> -			} else if (is_vmalloc_addr(urb->transfer_buffer)) {
> +			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {
>  				WARN_ONCE(1, "transfer buffer not dma capable\n");
>  				ret = -EAGAIN;
>  			} else {
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4f3eecedca2d7c..da8c1230302505 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -10,6 +10,8 @@
>  #include <linux/scatterlist.h>
>  #include <linux/kmemcheck.h>
>  #include <linux/bug.h>
> +#include <linux/mm.h>
> +#include <linux/sched/task_stack.h>
>  
>  /**
>   * List of possible attributes associated with a DMA mapping. The semantics
> @@ -818,4 +820,10 @@ static inline int dma_mmap_wc(struct device *dev,
>  #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
>  #endif
>  
> +/* only works in process context because of stack detection */
> +static inline bool is_dma_capable_addr(void *addr)
> +{
> +	return !(is_vmalloc_or_module_addr(addr) ||
> +		 object_is_on_stack(addr));

This does not catch kmap'ed buffers which are not directly DMA'able.
I would suggest to use virt_addr_valid() instead. Something like:

	return (virt_addr_valid(addr) && !object_is_on_stack(addr));

> +}
>  #endif
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 34a1c3e46ed725..47de0c0a700e7c 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -261,6 +261,7 @@ int is_vmalloc_or_module_addr(const void *x)
>  #endif
>  	return is_vmalloc_addr(x);
>  }
> +EXPORT_SYMBOL_GPL(is_vmalloc_or_module_addr);
>  
>  /*
>   * Walk a vmap address to the struct page it maps.
> 
> ===
> 
> The WIP branch containing also the I2C parts can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma
> 
> I think the whole series needs 1 or 2 days more before I send out an
> RFC, but I thought I'll let you know about my idea already.
> 
> Thanks and kind regards,
> 
>    Wolfram
> 

-- 
Regards
Vignesh