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

From: Robin Murphy
Date: Fri Oct 04 2019 - 14:51:04 EST

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@xxxxxxxxxx>
Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
---
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.