[PATCH] dma-mapping: Lift address space checks out of debug code

[PATCH] dma-mapping: Lift address space checks out of debug code

[Kees Cook]

As we've seen from USB and other areas, we need to always do runtime
checks for DMA operating on memory regions that might be remapped. This
consolidates the (existing!) checks and makes them on by default. A
warning will be triggered for any drivers still using DMA on the stack
(as has been seen in a few recent reports).

Suggested-by: Laura Abbott <labbott@redhat.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/dma-debug.h   |  8 --------
 include/linux/dma-mapping.h |  8 +++++++-
 kernel/dma/debug.c          | 16 ----------------
 3 files changed, 7 insertions(+), 25 deletions(-)

diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
index 4208f94d93f7..2af9765d9af7 100644
--- a/include/linux/dma-debug.h
+++ b/include/linux/dma-debug.h
@@ -18,9 +18,6 @@ struct bus_type;
 
 extern void dma_debug_add_bus(struct bus_type *bus);
 
-extern void debug_dma_map_single(struct device *dev, const void *addr,
-				 unsigned long len);
-
 extern void debug_dma_map_page(struct device *dev, struct page *page,
 			       size_t offset, size_t size,
 			       int direction, dma_addr_t dma_addr);
@@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
 {
 }
 
-static inline void debug_dma_map_single(struct device *dev, const void *addr,
-					unsigned long len)
-{
-}
-
 static inline void debug_dma_map_page(struct device *dev, struct page *page,
 				      size_t offset, size_t size,
 				      int direction, dma_addr_t dma_addr)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4a1c4fca475a..2d6b8382eab1 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct device *dev)
 static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
 		size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
-	debug_dma_map_single(dev, ptr, size);
+	/* DMA must never operate on stack or other remappable places. */
+	WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
+		"%s %s: driver maps %lu bytes from %s area\n",
+		dev ? dev_driver_string(dev) : "unknown driver",
+		dev ? dev_name(dev) : "unknown device", size,
+		is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
+
 	return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
 			size, dir, attrs);
 }
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 099002d84f46..aa1e6a1990b2 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -1232,22 +1232,6 @@ static void check_sg_segment(struct device *dev, struct scatterlist *sg)
 #endif
 }
 
-void debug_dma_map_single(struct device *dev, const void *addr,
-			    unsigned long len)
-{
-	if (unlikely(dma_debug_disabled()))
-		return;
-
-	if (!virt_addr_valid(addr))
-		err_printk(dev, NULL, "device driver maps memory from invalid area [addr=%p] [len=%lu]\n",
-			   addr, len);
-
-	if (is_vmalloc_addr(addr))
-		err_printk(dev, NULL, "device driver maps memory from vmalloc area [addr=%p] [len=%lu]\n",
-			   addr, len);
-}
-EXPORT_SYMBOL(debug_dma_map_single);
-
 void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
 			size_t size, int direction, dma_addr_t dma_addr)
 {
-- 
2.17.1


-- 
Kees Cook
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[Robin Murphy]

Hi Kees,

On 2019-10-02 9:46 pm, Kees Cook wrote:
> As we've seen from USB and other areas, we need to always do runtime
> checks for DMA operating on memory regions that might be remapped. This
> consolidates the (existing!) checks and makes them on by default. A
> warning will be triggered for any drivers still using DMA on the stack
> (as has been seen in a few recent reports).
> 
> Suggested-by: Laura Abbott <labbott@redhat.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   include/linux/dma-debug.h   |  8 --------
>   include/linux/dma-mapping.h |  8 +++++++-
>   kernel/dma/debug.c          | 16 ----------------
>   3 files changed, 7 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> index 4208f94d93f7..2af9765d9af7 100644
> --- a/include/linux/dma-debug.h
> +++ b/include/linux/dma-debug.h
> @@ -18,9 +18,6 @@ struct bus_type;
>   
>   extern void dma_debug_add_bus(struct bus_type *bus);
>   
> -extern void debug_dma_map_single(struct device *dev, const void *addr,
> -				 unsigned long len);
> -
>   extern void debug_dma_map_page(struct device *dev, struct page *page,
>   			       size_t offset, size_t size,
>   			       int direction, dma_addr_t dma_addr);
> @@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
>   {
>   }
>   
> -static inline void debug_dma_map_single(struct device *dev, const void *addr,
> -					unsigned long len)
> -{
> -}
> -
>   static inline void debug_dma_map_page(struct device *dev, struct page *page,
>   				      size_t offset, size_t size,
>   				      int direction, dma_addr_t dma_addr)
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4a1c4fca475a..2d6b8382eab1 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct device *dev)
>   static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
>   		size_t size, enum dma_data_direction dir, unsigned long attrs)
>   {
> -	debug_dma_map_single(dev, ptr, size);
> +	/* DMA must never operate on stack or other remappable places. */
> +	WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),

This stands to absolutely cripple I/O performance on arm64, because 
every valid call will end up going off and scanning the memblock list, 
which is not something we want on a fastpath in non-debug 
configurations. We'd need a much better solution to the "pfn_valid() vs. 
EFI no-map" problem before this might be viable.

Robin.

> +		"%s %s: driver maps %lu bytes from %s area\n",
> +		dev ? dev_driver_string(dev) : "unknown driver",
> +		dev ? dev_name(dev) : "unknown device", size,
> +		is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
> +
>   	return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
>   			size, dir, attrs);
>   }
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 099002d84f46..aa1e6a1990b2 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -1232,22 +1232,6 @@ static void check_sg_segment(struct device *dev, struct scatterlist *sg)
>   #endif
>   }
>   
> -void debug_dma_map_single(struct device *dev, const void *addr,
> -			    unsigned long len)
> -{
> -	if (unlikely(dma_debug_disabled()))
> -		return;
> -
> -	if (!virt_addr_valid(addr))
> -		err_printk(dev, NULL, "device driver maps memory from invalid area [addr=%p] [len=%lu]\n",
> -			   addr, len);
> -
> -	if (is_vmalloc_addr(addr))
> -		err_printk(dev, NULL, "device driver maps memory from vmalloc area [addr=%p] [len=%lu]\n",
> -			   addr, len);
> -}
> -EXPORT_SYMBOL(debug_dma_map_single);
> -
>   void debug_dma_map_page(struct device *dev, struct page *page, size_t offset,
>   			size_t size, int direction, dma_addr_t dma_addr)
>   {
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]

Hi Kees,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc1 next-20191002]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Kees-Cook/dma-mapping-Lift-address-space-checks-out-of-debug-code/20191003-060622
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:15:0,
                    from include/linux/list.h:9,
                    from include/linux/module.h:9,
                    from init/do_mounts.c:2:
   include/linux/dma-mapping.h: In function 'dma_map_single_attrs':
>> include/linux/dma-mapping.h:588:3: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
      "%s %s: driver maps %lu bytes from %s area\n",
      ^
   include/linux/printk.h:137:10: note: in definition of macro 'no_printk'
      printk(fmt, ##__VA_ARGS__);  \
             ^~~
   include/asm-generic/bug.h:196:41: note: in expansion of macro 'WARN'
    #define WARN_ONCE(condition, format...) WARN(condition, format)
                                            ^~~~
>> include/linux/dma-mapping.h:587:2: note: in expansion of macro 'WARN_ONCE'
     WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
     ^~~~~~~~~

vim +588 include/linux/dma-mapping.h

   582	
   583	static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
   584			size_t size, enum dma_data_direction dir, unsigned long attrs)
   585	{
   586		/* DMA must never operate on stack or other remappable places. */
 > 587		WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
 > 588			"%s %s: driver maps %lu bytes from %s area\n",
   589			dev ? dev_driver_string(dev) : "unknown driver",
   590			dev ? dev_name(dev) : "unknown device", size,
   591			is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
   592	
   593		return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
   594				size, dir, attrs);
   595	}
   596	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7205 bytes --]

[-- Attachment #3: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[Kees Cook]

On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
> Hi Kees,
> 
> On 2019-10-02 9:46 pm, Kees Cook wrote:
> > As we've seen from USB and other areas, we need to always do runtime
> > checks for DMA operating on memory regions that might be remapped. This
> > consolidates the (existing!) checks and makes them on by default. A
> > warning will be triggered for any drivers still using DMA on the stack
> > (as has been seen in a few recent reports).
> > 
> > Suggested-by: Laura Abbott <labbott@redhat.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >   include/linux/dma-debug.h   |  8 --------
> >   include/linux/dma-mapping.h |  8 +++++++-
> >   kernel/dma/debug.c          | 16 ----------------
> >   3 files changed, 7 insertions(+), 25 deletions(-)
> > 
> > diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> > index 4208f94d93f7..2af9765d9af7 100644
> > --- a/include/linux/dma-debug.h
> > +++ b/include/linux/dma-debug.h
> > @@ -18,9 +18,6 @@ struct bus_type;
> >   extern void dma_debug_add_bus(struct bus_type *bus);
> > -extern void debug_dma_map_single(struct device *dev, const void *addr,
> > -				 unsigned long len);
> > -
> >   extern void debug_dma_map_page(struct device *dev, struct page *page,
> >   			       size_t offset, size_t size,
> >   			       int direction, dma_addr_t dma_addr);
> > @@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
> >   {
> >   }
> > -static inline void debug_dma_map_single(struct device *dev, const void *addr,
> > -					unsigned long len)
> > -{
> > -}
> > -
> >   static inline void debug_dma_map_page(struct device *dev, struct page *page,
> >   				      size_t offset, size_t size,
> >   				      int direction, dma_addr_t dma_addr)
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index 4a1c4fca475a..2d6b8382eab1 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct device *dev)
> >   static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> >   		size_t size, enum dma_data_direction dir, unsigned long attrs)
> >   {
> > -	debug_dma_map_single(dev, ptr, size);
> > +	/* DMA must never operate on stack or other remappable places. */
> > +	WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
> 
> This stands to absolutely cripple I/O performance on arm64, because every
> valid call will end up going off and scanning the memblock list, which is
> not something we want on a fastpath in non-debug configurations. We'd need a
> much better solution to the "pfn_valid() vs. EFI no-map" problem before this
> might be viable.

Ah! Interesting. I didn't realize this was fast-path (I don't know the
DMA code at all). I thought it was more of a "one time setup" before
actual DMA activity started.

Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is the
most important part of this as far as catching stack-based DMA attempts.
I thought virt_addr_valid() was cheap too, but I see it's much heavier on
arm64.

I just went to compare what the existing USB check does, and it happens
immediately before its call to dma_map_single(). Both checks are simple
bounds checks, so it shouldn't be an issue:

			if (is_vmalloc_addr(urb->setup_packet)) {
				WARN_ONCE(1, "setup packet is not dma capable\n");
				return -EAGAIN;
			} else if (object_is_on_stack(urb->setup_packet)) {
				WARN_ONCE(1, "setup packet is on stack\n");
				return -EAGAIN;
			}

			urb->setup_dma = dma_map_single(
					hcd->self.sysdev,
					urb->setup_packet,
					sizeof(struct usb_ctrlrequest),


In the USB case, it'll actually refuse to do the operation. Should
dma_map_single() similarly fail? I could push these checks down into
dma_map_single(), which would be a no-change on behavior for USB and
gain the checks on all other callers...

-- 
Kees Cook
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[Kees Cook]

On Wed, Oct 02, 2019 at 04:58:39PM -0700, Kees Cook wrote:
> In the USB case, it'll actually refuse to do the operation. Should
> dma_map_single() similarly fail? I could push these checks down into
> dma_map_single(), which would be a no-change on behavior for USB and
> gain the checks on all other callers...

Which begs the question: are all callers actually checking the result of
dma_map_single(). Many are paired with dma_mapping_error(), but lots
more aren't...

-- 
Kees Cook
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3147 bytes --]

Hi Kees,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.4-rc1 next-20191002]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Kees-Cook/dma-mapping-Lift-address-space-checks-out-of-debug-code/20191003-060622
config: i386-randconfig-c004-201939 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/x86/include/asm/bug.h:83:0,
                    from include/linux/bug.h:5,
                    from include/linux/debug_locks.h:7,
                    from include/linux/lockdep.h:28,
                    from include/linux/spinlock_types.h:18,
                    from include/linux/mutex.h:16,
                    from include/linux/kernfs.h:12,
                    from include/linux/sysfs.h:16,
                    from include/linux/kobject.h:20,
                    from include/linux/of.h:17,
                    from include/linux/irqdomain.h:35,
                    from include/linux/acpi.h:13,
                    from drivers/gpu/drm/i915/i915_drv.c:30:
   include/linux/dma-mapping.h: In function 'dma_map_single_attrs':
>> include/linux/dma-mapping.h:588:3: error: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t {aka unsigned int}' [-Werror=format=]
      "%s %s: driver maps %lu bytes from %s area\n",
      ^
   include/asm-generic/bug.h:92:17: note: in definition of macro '__WARN_printf'
      __warn_printk(arg);     \
                    ^~~
   include/asm-generic/bug.h:155:3: note: in expansion of macro 'WARN'
      WARN(1, format);    \
      ^~~~
   include/linux/dma-mapping.h:587:2: note: in expansion of macro 'WARN_ONCE'
     WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
     ^~~~~~~~~
   cc1: all warnings being treated as errors

vim +588 include/linux/dma-mapping.h

   582	
   583	static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
   584			size_t size, enum dma_data_direction dir, unsigned long attrs)
   585	{
   586		/* DMA must never operate on stack or other remappable places. */
   587		WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
 > 588			"%s %s: driver maps %lu bytes from %s area\n",
   589			dev ? dev_driver_string(dev) : "unknown driver",
   590			dev ? dev_name(dev) : "unknown device", size,
   591			is_vmalloc_addr(ptr) ? "vmalloc" : "invalid");
   592	
   593		return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
   594				size, dir, attrs);
   595	}
   596	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39725 bytes --]

[-- Attachment #3: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[Robin Murphy]

On 03/10/2019 00:58, Kees Cook wrote:
> On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
>> Hi Kees,
>>
>> On 2019-10-02 9:46 pm, Kees Cook wrote:
>>> As we've seen from USB and other areas, we need to always do runtime
>>> checks for DMA operating on memory regions that might be remapped. This
>>> consolidates the (existing!) checks and makes them on by default. A
>>> warning will be triggered for any drivers still using DMA on the stack
>>> (as has been seen in a few recent reports).
>>>
>>> Suggested-by: Laura Abbott <labbott@redhat.com>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>>    include/linux/dma-debug.h   |  8 --------
>>>    include/linux/dma-mapping.h |  8 +++++++-
>>>    kernel/dma/debug.c          | 16 ----------------
>>>    3 files changed, 7 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
>>> index 4208f94d93f7..2af9765d9af7 100644
>>> --- a/include/linux/dma-debug.h
>>> +++ b/include/linux/dma-debug.h
>>> @@ -18,9 +18,6 @@ struct bus_type;
>>>    extern void dma_debug_add_bus(struct bus_type *bus);
>>> -extern void debug_dma_map_single(struct device *dev, const void *addr,
>>> -				 unsigned long len);
>>> -
>>>    extern void debug_dma_map_page(struct device *dev, struct page *page,
>>>    			       size_t offset, size_t size,
>>>    			       int direction, dma_addr_t dma_addr);
>>> @@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
>>>    {
>>>    }
>>> -static inline void debug_dma_map_single(struct device *dev, const void *addr,
>>> -					unsigned long len)
>>> -{
>>> -}
>>> -
>>>    static inline void debug_dma_map_page(struct device *dev, struct page *page,
>>>    				      size_t offset, size_t size,
>>>    				      int direction, dma_addr_t dma_addr)
>>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>>> index 4a1c4fca475a..2d6b8382eab1 100644
>>> --- a/include/linux/dma-mapping.h
>>> +++ b/include/linux/dma-mapping.h
>>> @@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct device *dev)
>>>    static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
>>>    		size_t size, enum dma_data_direction dir, unsigned long attrs)
>>>    {
>>> -	debug_dma_map_single(dev, ptr, size);
>>> +	/* DMA must never operate on stack or other remappable places. */
>>> +	WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
>>
>> This stands to absolutely cripple I/O performance on arm64, because every
>> valid call will end up going off and scanning the memblock list, which is
>> not something we want on a fastpath in non-debug configurations. We'd need a
>> much better solution to the "pfn_valid() vs. EFI no-map" problem before this
>> might be viable.
> 
> Ah! Interesting. I didn't realize this was fast-path (I don't know the
> DMA code at all). I thought it was more of a "one time setup" before
> actual DMA activity started.

That's strictly true, it's just that many workloads can involve tens of 
thousands of "one time"s per second ;)

Overhead on the dma_map_* paths has shown to have a direct impact on 
throughput in such situations, hence various optimisation effort in IOVA 
allocation for IOMMU-based DMA ops, and the recent work to remove 
indirect calls entirely for the common dma-direct/SWIOTLB cases.

> Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is the
> most important part of this as far as catching stack-based DMA attempts.
> I thought virt_addr_valid() was cheap too, but I see it's much heavier on
> arm64.
> 
> I just went to compare what the existing USB check does, and it happens
> immediately before its call to dma_map_single(). Both checks are simple
> bounds checks, so it shouldn't be an issue:
> 
> 			if (is_vmalloc_addr(urb->setup_packet)) {
> 				WARN_ONCE(1, "setup packet is not dma capable\n");
> 				return -EAGAIN;
> 			} else if (object_is_on_stack(urb->setup_packet)) {
> 				WARN_ONCE(1, "setup packet is on stack\n");
> 				return -EAGAIN;
> 			}
> 
> 			urb->setup_dma = dma_map_single(
> 					hcd->self.sysdev,
> 					urb->setup_packet,
> 					sizeof(struct usb_ctrlrequest),
> 
> 
> In the USB case, it'll actually refuse to do the operation. Should
> dma_map_single() similarly fail? I could push these checks down into
> dma_map_single(), which would be a no-change on behavior for USB and
> gain the checks on all other callers...

I think it would be reasonable to pull the is_vmalloc_addr() check 
inline, as that probably covers 90+% of badness (especially given 
vmapped stacks), and as you say should be reliably cheap everywhere. 
Callers are certainly expected to use dma_mapping_error() and handle 
failure, so refusing to do a bogus mapping operation should be OK 
API-wise - ultimately if a driver goes ahead and uses DMA_MAPPING_ERROR 
as an address anyway, that's not likely to be any *more* catastrophic 
than if it did the same with whatever nonsense virt_to_phys() of a 
vmalloc address had returned.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[Kees Cook]

On Thu, Oct 03, 2019 at 10:42:45AM +0100, Robin Murphy wrote:
> On 03/10/2019 00:58, Kees Cook wrote:
> > On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
> > > Hi Kees,
> > > 
> > > On 2019-10-02 9:46 pm, Kees Cook wrote:
> > > > As we've seen from USB and other areas, we need to always do runtime
> > > > checks for DMA operating on memory regions that might be remapped. This
> > > > consolidates the (existing!) checks and makes them on by default. A
> > > > warning will be triggered for any drivers still using DMA on the stack
> > > > (as has been seen in a few recent reports).
> > > > 
> > > > Suggested-by: Laura Abbott <labbott@redhat.com>
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > ---
> > > >    include/linux/dma-debug.h   |  8 --------
> > > >    include/linux/dma-mapping.h |  8 +++++++-
> > > >    kernel/dma/debug.c          | 16 ----------------
> > > >    3 files changed, 7 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
> > > > index 4208f94d93f7..2af9765d9af7 100644
> > > > --- a/include/linux/dma-debug.h
> > > > +++ b/include/linux/dma-debug.h
> > > > @@ -18,9 +18,6 @@ struct bus_type;
> > > >    extern void dma_debug_add_bus(struct bus_type *bus);
> > > > -extern void debug_dma_map_single(struct device *dev, const void *addr,
> > > > -				 unsigned long len);
> > > > -
> > > >    extern void debug_dma_map_page(struct device *dev, struct page *page,
> > > >    			       size_t offset, size_t size,
> > > >    			       int direction, dma_addr_t dma_addr);
> > > > @@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
> > > >    {
> > > >    }
> > > > -static inline void debug_dma_map_single(struct device *dev, const void *addr,
> > > > -					unsigned long len)
> > > > -{
> > > > -}
> > > > -
> > > >    static inline void debug_dma_map_page(struct device *dev, struct page *page,
> > > >    				      size_t offset, size_t size,
> > > >    				      int direction, dma_addr_t dma_addr)
> > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > index 4a1c4fca475a..2d6b8382eab1 100644
> > > > --- a/include/linux/dma-mapping.h
> > > > +++ b/include/linux/dma-mapping.h
> > > > @@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct device *dev)
> > > >    static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> > > >    		size_t size, enum dma_data_direction dir, unsigned long attrs)
> > > >    {
> > > > -	debug_dma_map_single(dev, ptr, size);
> > > > +	/* DMA must never operate on stack or other remappable places. */
> > > > +	WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
> > > 
> > > This stands to absolutely cripple I/O performance on arm64, because every
> > > valid call will end up going off and scanning the memblock list, which is
> > > not something we want on a fastpath in non-debug configurations. We'd need a
> > > much better solution to the "pfn_valid() vs. EFI no-map" problem before this
> > > might be viable.
> > 
> > Ah! Interesting. I didn't realize this was fast-path (I don't know the
> > DMA code at all). I thought it was more of a "one time setup" before
> > actual DMA activity started.
> 
> That's strictly true, it's just that many workloads can involve tens of
> thousands of "one time"s per second ;)
> 
> Overhead on the dma_map_* paths has shown to have a direct impact on
> throughput in such situations, hence various optimisation effort in IOVA
> allocation for IOMMU-based DMA ops, and the recent work to remove indirect
> calls entirely for the common dma-direct/SWIOTLB cases.
> 
> > Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is the
> > most important part of this as far as catching stack-based DMA attempts.
> > I thought virt_addr_valid() was cheap too, but I see it's much heavier on
> > arm64.
> > 
> > I just went to compare what the existing USB check does, and it happens
> > immediately before its call to dma_map_single(). Both checks are simple
> > bounds checks, so it shouldn't be an issue:
> > 
> > 			if (is_vmalloc_addr(urb->setup_packet)) {
> > 				WARN_ONCE(1, "setup packet is not dma capable\n");
> > 				return -EAGAIN;
> > 			} else if (object_is_on_stack(urb->setup_packet)) {
> > 				WARN_ONCE(1, "setup packet is on stack\n");
> > 				return -EAGAIN;
> > 			}
> > 
> > 			urb->setup_dma = dma_map_single(
> > 					hcd->self.sysdev,
> > 					urb->setup_packet,
> > 					sizeof(struct usb_ctrlrequest),
> > 
> > 
> > In the USB case, it'll actually refuse to do the operation. Should
> > dma_map_single() similarly fail? I could push these checks down into
> > dma_map_single(), which would be a no-change on behavior for USB and
> > gain the checks on all other callers...
> 
> I think it would be reasonable to pull the is_vmalloc_addr() check inline,
> as that probably covers 90+% of badness (especially given vmapped stacks),
> and as you say should be reliably cheap everywhere. Callers are certainly
> expected to use dma_mapping_error() and handle failure, so refusing to do a
> bogus mapping operation should be OK API-wise - ultimately if a driver goes
> ahead and uses DMA_MAPPING_ERROR as an address anyway, that's not likely to
> be any *more* catastrophic than if it did the same with whatever nonsense
> virt_to_phys() of a vmalloc address had returned.

What do you think about the object_is_on_stack() check? That does a
dereference through "current" to find the stack bounds...

-- 
Kees Cook
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[Robin Murphy]

On 03/10/2019 22:38, Kees Cook wrote:
> On Thu, Oct 03, 2019 at 10:42:45AM +0100, Robin Murphy wrote:
>> On 03/10/2019 00:58, Kees Cook wrote:
>>> On Wed, Oct 02, 2019 at 10:15:43PM +0100, Robin Murphy wrote:
>>>> Hi Kees,
>>>>
>>>> On 2019-10-02 9:46 pm, Kees Cook wrote:
>>>>> As we've seen from USB and other areas, we need to always do runtime
>>>>> checks for DMA operating on memory regions that might be remapped. This
>>>>> consolidates the (existing!) checks and makes them on by default. A
>>>>> warning will be triggered for any drivers still using DMA on the stack
>>>>> (as has been seen in a few recent reports).
>>>>>
>>>>> Suggested-by: Laura Abbott <labbott@redhat.com>
>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>> ---
>>>>>     include/linux/dma-debug.h   |  8 --------
>>>>>     include/linux/dma-mapping.h |  8 +++++++-
>>>>>     kernel/dma/debug.c          | 16 ----------------
>>>>>     3 files changed, 7 insertions(+), 25 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/dma-debug.h b/include/linux/dma-debug.h
>>>>> index 4208f94d93f7..2af9765d9af7 100644
>>>>> --- a/include/linux/dma-debug.h
>>>>> +++ b/include/linux/dma-debug.h
>>>>> @@ -18,9 +18,6 @@ struct bus_type;
>>>>>     extern void dma_debug_add_bus(struct bus_type *bus);
>>>>> -extern void debug_dma_map_single(struct device *dev, const void *addr,
>>>>> -				 unsigned long len);
>>>>> -
>>>>>     extern void debug_dma_map_page(struct device *dev, struct page *page,
>>>>>     			       size_t offset, size_t size,
>>>>>     			       int direction, dma_addr_t dma_addr);
>>>>> @@ -75,11 +72,6 @@ static inline void dma_debug_add_bus(struct bus_type *bus)
>>>>>     {
>>>>>     }
>>>>> -static inline void debug_dma_map_single(struct device *dev, const void *addr,
>>>>> -					unsigned long len)
>>>>> -{
>>>>> -}
>>>>> -
>>>>>     static inline void debug_dma_map_page(struct device *dev, struct page *page,
>>>>>     				      size_t offset, size_t size,
>>>>>     				      int direction, dma_addr_t dma_addr)
>>>>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>>>>> index 4a1c4fca475a..2d6b8382eab1 100644
>>>>> --- a/include/linux/dma-mapping.h
>>>>> +++ b/include/linux/dma-mapping.h
>>>>> @@ -583,7 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct device *dev)
>>>>>     static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
>>>>>     		size_t size, enum dma_data_direction dir, unsigned long attrs)
>>>>>     {
>>>>> -	debug_dma_map_single(dev, ptr, size);
>>>>> +	/* DMA must never operate on stack or other remappable places. */
>>>>> +	WARN_ONCE(is_vmalloc_addr(ptr) || !virt_addr_valid(ptr),
>>>>
>>>> This stands to absolutely cripple I/O performance on arm64, because every
>>>> valid call will end up going off and scanning the memblock list, which is
>>>> not something we want on a fastpath in non-debug configurations. We'd need a
>>>> much better solution to the "pfn_valid() vs. EFI no-map" problem before this
>>>> might be viable.
>>>
>>> Ah! Interesting. I didn't realize this was fast-path (I don't know the
>>> DMA code at all). I thought it was more of a "one time setup" before
>>> actual DMA activity started.
>>
>> That's strictly true, it's just that many workloads can involve tens of
>> thousands of "one time"s per second ;)
>>
>> Overhead on the dma_map_* paths has shown to have a direct impact on
>> throughput in such situations, hence various optimisation effort in IOVA
>> allocation for IOMMU-based DMA ops, and the recent work to remove indirect
>> calls entirely for the common dma-direct/SWIOTLB cases.
>>
>>> Regardless, is_vmalloc_addr() is extremely light (a bounds check), and is the
>>> most important part of this as far as catching stack-based DMA attempts.
>>> I thought virt_addr_valid() was cheap too, but I see it's much heavier on
>>> arm64.
>>>
>>> I just went to compare what the existing USB check does, and it happens
>>> immediately before its call to dma_map_single(). Both checks are simple
>>> bounds checks, so it shouldn't be an issue:
>>>
>>> 			if (is_vmalloc_addr(urb->setup_packet)) {
>>> 				WARN_ONCE(1, "setup packet is not dma capable\n");
>>> 				return -EAGAIN;
>>> 			} else if (object_is_on_stack(urb->setup_packet)) {
>>> 				WARN_ONCE(1, "setup packet is on stack\n");
>>> 				return -EAGAIN;
>>> 			}
>>>
>>> 			urb->setup_dma = dma_map_single(
>>> 					hcd->self.sysdev,
>>> 					urb->setup_packet,
>>> 					sizeof(struct usb_ctrlrequest),
>>>
>>>
>>> In the USB case, it'll actually refuse to do the operation. Should
>>> dma_map_single() similarly fail? I could push these checks down into
>>> dma_map_single(), which would be a no-change on behavior for USB and
>>> gain the checks on all other callers...
>>
>> I think it would be reasonable to pull the is_vmalloc_addr() check inline,
>> as that probably covers 90+% of badness (especially given vmapped stacks),
>> and as you say should be reliably cheap everywhere. Callers are certainly
>> expected to use dma_mapping_error() and handle failure, so refusing to do a
>> bogus mapping operation should be OK API-wise - ultimately if a driver goes
>> ahead and uses DMA_MAPPING_ERROR as an address anyway, that's not likely to
>> be any *more* catastrophic than if it did the same with whatever nonsense
>> virt_to_phys() of a vmalloc address had returned.
> 
> What do you think about the object_is_on_stack() check? That does a
> dereference through "current" to find the stack bounds...

I guess it depends what the aim is - is it just to bail out of 
operations which have near-zero chance of working correctly and every 
chance of going catastrophically wrong, or to lay down strict argument 
checking for the API in general? (for cache-coherent devices, or if the 
caller is careful to ensure the appropriate alignment, DMA from a 
non-virtually-mapped stack can be *technically* fine, it's just banned 
in general because those necessary assumptions can be tricky to meet and 
aren't at all portable).

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[Kees Cook]

On Fri, Oct 04, 2019 at 07:50:54PM +0100, Robin Murphy wrote:
> On 03/10/2019 22:38, Kees Cook wrote:
> > What do you think about the object_is_on_stack() check? That does a
> > dereference through "current" to find the stack bounds...
> 
> I guess it depends what the aim is - is it just to bail out of operations
> which have near-zero chance of working correctly and every chance of going
> catastrophically wrong, or to lay down strict argument checking for the API
> in general? (for cache-coherent devices, or if the caller is careful to
> ensure the appropriate alignment, DMA from a non-virtually-mapped stack can
> be *technically* fine, it's just banned in general because those necessary
> assumptions can be tricky to meet and aren't at all portable).

Okay, then since the vmap check is both the cheapest and the most
important to catch in the face of breaking everything, I'll move that
in and we can keep USB's other checks separately.

-- 
Kees Cook
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: Lift address space checks out of debug code

[Christoph Hellwig]

On Thu, Oct 03, 2019 at 02:38:43PM -0700, Kees Cook wrote:
> > I think it would be reasonable to pull the is_vmalloc_addr() check inline,
> > as that probably covers 90+% of badness (especially given vmapped stacks),
> > and as you say should be reliably cheap everywhere. Callers are certainly
> > expected to use dma_mapping_error() and handle failure, so refusing to do a
> > bogus mapping operation should be OK API-wise - ultimately if a driver goes
> > ahead and uses DMA_MAPPING_ERROR as an address anyway, that's not likely to
> > be any *more* catastrophic than if it did the same with whatever nonsense
> > virt_to_phys() of a vmalloc address had returned.
> 
> What do you think about the object_is_on_stack() check? That does a
> dereference through "current" to find the stack bounds...

I can be persuaded about just the vmalloc check as people tend to get
a lot of vmalloc alloctions without knowing these days.  But what I'd
really like to see is a new config option that enables relatively
cheap checks without the full dma debugging infrastructure.  That way
you can turn those on at least for all development builds, and can
easily benchmark having the checks vs not.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu