| Message ID | 20211104023221.16391-1-walter-zh.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] dma-direct: improve DMA_ATTR_NO_KERNEL_MAPPING | expand |
On Thu, Nov 04, 2021 at 10:32:21AM +0800, Walter Wu wrote: > diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h > index f36be5166c19..6c7d1683339c 100644 > --- a/include/linux/set_memory.h > +++ b/include/linux/set_memory.h > @@ -7,11 +7,16 @@ > > #ifdef CONFIG_ARCH_HAS_SET_MEMORY > #include <asm/set_memory.h> > + > +#ifndef CONFIG_RODATA_FULL_DEFAULT_ENABLED This is an arm64-specific symbol, and one that only controls a default. I don't think it is suitable to key off stubs in common code. > +static inline int set_memory_valid(unsigned long addr, int numpages, int enable) { return 0; } Pleae avoid overly long lines. > + if (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) { > + kaddr = (unsigned long)phys_to_virt(dma_to_phys(dev, *dma_handle)); This can just use page_address. > + /* page remove kernel mapping for arm64 */ > + set_memory_valid(kaddr, size >> PAGE_SHIFT, 0); > + } But more importantly: set_memory_valid only exists on arm64, this will break compile everywhere else. And this API is complete crap. Passing kernel virtual addresses as unsigned long just sucks, and passing an integer argument for valid/non-valid also is a horrible API. Not to mention the overly long line. Same on the free side.
On Thu, 4 Nov 2021 at 09:53, Christoph Hellwig <hch@lst.de> wrote: > > On Thu, Nov 04, 2021 at 10:32:21AM +0800, Walter Wu wrote: > > diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h > > index f36be5166c19..6c7d1683339c 100644 > > --- a/include/linux/set_memory.h > > +++ b/include/linux/set_memory.h > > @@ -7,11 +7,16 @@ > > > > #ifdef CONFIG_ARCH_HAS_SET_MEMORY > > #include <asm/set_memory.h> > > + > > +#ifndef CONFIG_RODATA_FULL_DEFAULT_ENABLED > > This is an arm64-specific symbol, and one that only controls a > default. I don't think it is suitable to key off stubs in common > code. > > > +static inline int set_memory_valid(unsigned long addr, int numpages, int enable) { return 0; } > > Pleae avoid overly long lines. > > > + if (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) { > > + kaddr = (unsigned long)phys_to_virt(dma_to_phys(dev, *dma_handle)); > > This can just use page_address. > > > + /* page remove kernel mapping for arm64 */ > > + set_memory_valid(kaddr, size >> PAGE_SHIFT, 0); > > + } > > But more importantly: set_memory_valid only exists on arm64, this > will break compile everywhere else. And this API is complete crap. > Passing kernel virtual addresses as unsigned long just sucks, and > passing an integer argument for valid/non-valid also is a horrible > API. > ... and as I pointed out before, you can still pass rodata=off on arm64, and get the old behavior, in which case bad things will happen if you try to use an API that expects to operate on page mappings with a 1 GB block mapping. And you still haven't explained what the actual problem is: is this about CPU speculation corrupting non-cache coherent inbound DMA?
On Thu, 2021-11-04 at 09:53 +0100, Christoph Hellwig wrote: > On Thu, Nov 04, 2021 at 10:32:21AM +0800, Walter Wu wrote: > > diff --git a/include/linux/set_memory.h > > b/include/linux/set_memory.h > > index f36be5166c19..6c7d1683339c 100644 > > --- a/include/linux/set_memory.h > > +++ b/include/linux/set_memory.h > > @@ -7,11 +7,16 @@ > > > > #ifdef CONFIG_ARCH_HAS_SET_MEMORY > > #include <asm/set_memory.h> > > + > > +#ifndef CONFIG_RODATA_FULL_DEFAULT_ENABLED > > This is an arm64-specific symbol, and one that only controls a > default. I don't think it is suitable to key off stubs in common > code. > ok > > +static inline int set_memory_valid(unsigned long addr, int > > numpages, int enable) { return 0; } > > Pleae avoid overly long lines. > > > + if (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) { > > + kaddr = (unsigned > > long)phys_to_virt(dma_to_phys(dev, *dma_handle)); > > This can just use page_address. > > > + /* page remove kernel mapping for arm64 */ > > + set_memory_valid(kaddr, size >> PAGE_SHIFT, 0); > > + } > > But more importantly: set_memory_valid only exists on arm64, this > will break compile everywhere else. And this API is complete crap. > Passing kernel virtual addresses as unsigned long just sucks, and > passing an integer argument for valid/non-valid also is a horrible > API. > Would you think __kernel_map_page() is ok? Many arch support it, and only pass page and page number. but need to depend CONFIG_DEBUG_PAGEALLOC. Thanks. Walter > Not to mention the overly long line. Same on the free side.
On Thu, 2021-11-04 at 09:57 +0100, Ard Biesheuvel wrote: > On Thu, 4 Nov 2021 at 09:53, Christoph Hellwig <hch@lst.de> wrote: > > > > On Thu, Nov 04, 2021 at 10:32:21AM +0800, Walter Wu wrote: > > > diff --git a/include/linux/set_memory.h > > > b/include/linux/set_memory.h > > > index f36be5166c19..6c7d1683339c 100644 > > > --- a/include/linux/set_memory.h > > > +++ b/include/linux/set_memory.h > > > @@ -7,11 +7,16 @@ > > > > > > #ifdef CONFIG_ARCH_HAS_SET_MEMORY > > > #include <asm/set_memory.h> > > > + > > > +#ifndef CONFIG_RODATA_FULL_DEFAULT_ENABLED > > > > This is an arm64-specific symbol, and one that only controls a > > default. I don't think it is suitable to key off stubs in common > > code. > > > > > +static inline int set_memory_valid(unsigned long addr, int > > > numpages, int enable) { return 0; } > > > > Pleae avoid overly long lines. > > > > > + if (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) > > > { > > > + kaddr = (unsigned > > > long)phys_to_virt(dma_to_phys(dev, *dma_handle)); > > > > This can just use page_address. > > > > > + /* page remove kernel mapping for arm64 */ > > > + set_memory_valid(kaddr, size >> PAGE_SHIFT, > > > 0); > > > + } > > > > But more importantly: set_memory_valid only exists on arm64, this > > will break compile everywhere else. And this API is complete crap. > > Passing kernel virtual addresses as unsigned long just sucks, and > > passing an integer argument for valid/non-valid also is a horrible > > API. > > > > ... and as I pointed out before, you can still pass rodata=off on > arm64, and get the old behavior, in which case bad things will happen > if you try to use an API that expects to operate on page mappings > with > a 1 GB block mapping. > Thanks for your suggestion. > And you still haven't explained what the actual problem is: is this > about CPU speculation corrupting non-cache coherent inbound DMA? No corrupiton, only cpu read it, we hope to fix the behavior. Thanks. Walter
On Thu, 4 Nov 2021 at 13:31, Walter Wu <walter-zh.wu@mediatek.com> wrote: > > On Thu, 2021-11-04 at 09:57 +0100, Ard Biesheuvel wrote: > > On Thu, 4 Nov 2021 at 09:53, Christoph Hellwig <hch@lst.de> wrote: > > > > > > On Thu, Nov 04, 2021 at 10:32:21AM +0800, Walter Wu wrote: > > > > diff --git a/include/linux/set_memory.h > > > > b/include/linux/set_memory.h > > > > index f36be5166c19..6c7d1683339c 100644 > > > > --- a/include/linux/set_memory.h > > > > +++ b/include/linux/set_memory.h > > > > @@ -7,11 +7,16 @@ > > > > > > > > #ifdef CONFIG_ARCH_HAS_SET_MEMORY > > > > #include <asm/set_memory.h> > > > > + > > > > +#ifndef CONFIG_RODATA_FULL_DEFAULT_ENABLED > > > > > > This is an arm64-specific symbol, and one that only controls a > > > default. I don't think it is suitable to key off stubs in common > > > code. > > > > > > > +static inline int set_memory_valid(unsigned long addr, int > > > > numpages, int enable) { return 0; } > > > > > > Pleae avoid overly long lines. > > > > > > > + if (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) > > > > { > > > > + kaddr = (unsigned > > > > long)phys_to_virt(dma_to_phys(dev, *dma_handle)); > > > > > > This can just use page_address. > > > > > > > + /* page remove kernel mapping for arm64 */ > > > > + set_memory_valid(kaddr, size >> PAGE_SHIFT, > > > > 0); > > > > + } > > > > > > But more importantly: set_memory_valid only exists on arm64, this > > > will break compile everywhere else. And this API is complete crap. > > > Passing kernel virtual addresses as unsigned long just sucks, and > > > passing an integer argument for valid/non-valid also is a horrible > > > API. > > > > > > > ... and as I pointed out before, you can still pass rodata=off on > > arm64, and get the old behavior, in which case bad things will happen > > if you try to use an API that expects to operate on page mappings > > with > > a 1 GB block mapping. > > > > Thanks for your suggestion. > > > > And you still haven't explained what the actual problem is: is this > > about CPU speculation corrupting non-cache coherent inbound DMA? > > No corrupiton, only cpu read it, we hope to fix the behavior. > Fix which behavior? Please explain 1) the current behavior 2) why the current behavior is problematic for you 3) how this patch changes the current behavior 4) why the new behavior fixes your problem. There is no penalty for using too many words.
On Thu, 2021-11-04 at 13:47 +0100, Ard Biesheuvel wrote: > On Thu, 4 Nov 2021 at 13:31, Walter Wu <walter-zh.wu@mediatek.com> > wrote: > > > > On Thu, 2021-11-04 at 09:57 +0100, Ard Biesheuvel wrote: > > > On Thu, 4 Nov 2021 at 09:53, Christoph Hellwig <hch@lst.de> > > > wrote: > > > > > > > > On Thu, Nov 04, 2021 at 10:32:21AM +0800, Walter Wu wrote: > > > > > diff --git a/include/linux/set_memory.h > > > > > b/include/linux/set_memory.h > > > > > index f36be5166c19..6c7d1683339c 100644 > > > > > --- a/include/linux/set_memory.h > > > > > +++ b/include/linux/set_memory.h > > > > > @@ -7,11 +7,16 @@ > > > > > > > > > > #ifdef CONFIG_ARCH_HAS_SET_MEMORY > > > > > #include <asm/set_memory.h> > > > > > + > > > > > +#ifndef CONFIG_RODATA_FULL_DEFAULT_ENABLED > > > > > > > > This is an arm64-specific symbol, and one that only controls a > > > > default. I don't think it is suitable to key off stubs in > > > > common > > > > code. > > > > > > > > > +static inline int set_memory_valid(unsigned long addr, int > > > > > numpages, int enable) { return 0; } > > > > > > > > Pleae avoid overly long lines. > > > > > > > > > + if > > > > > (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) > > > > > { > > > > > + kaddr = (unsigned > > > > > long)phys_to_virt(dma_to_phys(dev, *dma_handle)); > > > > > > > > This can just use page_address. > > > > > > > > > + /* page remove kernel mapping for arm64 > > > > > */ > > > > > + set_memory_valid(kaddr, size >> > > > > > PAGE_SHIFT, > > > > > 0); > > > > > + } > > > > > > > > But more importantly: set_memory_valid only exists on arm64, > > > > this > > > > will break compile everywhere else. And this API is complete > > > > crap. > > > > Passing kernel virtual addresses as unsigned long just sucks, > > > > and > > > > passing an integer argument for valid/non-valid also is a > > > > horrible > > > > API. > > > > > > > > > > ... and as I pointed out before, you can still pass rodata=off on > > > arm64, and get the old behavior, in which case bad things will > > > happen > > > if you try to use an API that expects to operate on page mappings > > > with > > > a 1 GB block mapping. > > > > > > > Thanks for your suggestion. > > > > > > > And you still haven't explained what the actual problem is: is > > > this > > > about CPU speculation corrupting non-cache coherent inbound DMA? > > > > No corrupiton, only cpu read it, we hope to fix the behavior. > > > > Fix which behavior? Please explain > > 1) the current behavior We call dma_direct_alloc() with DMA_ATTR_NO_KERNEL_MAPPING to get the allocated buffer and the kernel mapping is exist. Our goal is this buffer doesn't allow to be accessed by cpu. Unfortunately, we see cpu speculation to read it. So we need to fix it and don't use no-map the way. > 2) why the current behavior is problematic for you dma_direct_alloc() with DMA_ATTR_NO_KERNEL_MAPPING have kernel mapping, so it still has cpu speculation read the buffer. Although we have hardware to protect the buffer, we still hope use software to fix it. > 3) how this patch changes the current behavior When call dma_direct_alloc() with DMA_ATTR_NO_KERNEL_MAPPING, then remove the kernel mapping which belong to the buffer. > 4) why the new behavior fixes your problem. If I understand correctly, want to block cpu speculation, then need unmap the buffer at stage 1 and stage 2 page table and tlb invalidate. This patch is to do stage 1 unmap at EL1. > > There is no penalty for using too many words. Thanks. Walter
On Thu, 4 Nov 2021 at 14:40, Walter Wu <walter-zh.wu@mediatek.com> wrote: > > On Thu, 2021-11-04 at 13:47 +0100, Ard Biesheuvel wrote: > > On Thu, 4 Nov 2021 at 13:31, Walter Wu <walter-zh.wu@mediatek.com> > > wrote: > > > > > > On Thu, 2021-11-04 at 09:57 +0100, Ard Biesheuvel wrote: > > > > On Thu, 4 Nov 2021 at 09:53, Christoph Hellwig <hch@lst.de> > > > > wrote: > > > > > > > > > > On Thu, Nov 04, 2021 at 10:32:21AM +0800, Walter Wu wrote: > > > > > > diff --git a/include/linux/set_memory.h > > > > > > b/include/linux/set_memory.h > > > > > > index f36be5166c19..6c7d1683339c 100644 > > > > > > --- a/include/linux/set_memory.h > > > > > > +++ b/include/linux/set_memory.h > > > > > > @@ -7,11 +7,16 @@ > > > > > > > > > > > > #ifdef CONFIG_ARCH_HAS_SET_MEMORY > > > > > > #include <asm/set_memory.h> > > > > > > + > > > > > > +#ifndef CONFIG_RODATA_FULL_DEFAULT_ENABLED > > > > > > > > > > This is an arm64-specific symbol, and one that only controls a > > > > > default. I don't think it is suitable to key off stubs in > > > > > common > > > > > code. > > > > > > > > > > > +static inline int set_memory_valid(unsigned long addr, int > > > > > > numpages, int enable) { return 0; } > > > > > > > > > > Pleae avoid overly long lines. > > > > > > > > > > > + if > > > > > > (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) > > > > > > { > > > > > > + kaddr = (unsigned > > > > > > long)phys_to_virt(dma_to_phys(dev, *dma_handle)); > > > > > > > > > > This can just use page_address. > > > > > > > > > > > + /* page remove kernel mapping for arm64 > > > > > > */ > > > > > > + set_memory_valid(kaddr, size >> > > > > > > PAGE_SHIFT, > > > > > > 0); > > > > > > + } > > > > > > > > > > But more importantly: set_memory_valid only exists on arm64, > > > > > this > > > > > will break compile everywhere else. And this API is complete > > > > > crap. > > > > > Passing kernel virtual addresses as unsigned long just sucks, > > > > > and > > > > > passing an integer argument for valid/non-valid also is a > > > > > horrible > > > > > API. > > > > > > > > > > > > > ... and as I pointed out before, you can still pass rodata=off on > > > > arm64, and get the old behavior, in which case bad things will > > > > happen > > > > if you try to use an API that expects to operate on page mappings > > > > with > > > > a 1 GB block mapping. > > > > > > > > > > Thanks for your suggestion. > > > > > > > > > > And you still haven't explained what the actual problem is: is > > > > this > > > > about CPU speculation corrupting non-cache coherent inbound DMA? > > > > > > No corrupiton, only cpu read it, we hope to fix the behavior. > > > > > > > Fix which behavior? Please explain > > > > 1) the current behavior > We call dma_direct_alloc() with DMA_ATTR_NO_KERNEL_MAPPING to get the > allocated buffer and the kernel mapping is exist. Our goal is this > buffer doesn't allow to be accessed by cpu. Unfortunately, we see cpu > speculation to read it. So we need to fix it and don't use no-map the > way. > > > 2) why the current behavior is problematic for you > dma_direct_alloc() with DMA_ATTR_NO_KERNEL_MAPPING have kernel mapping, > so it still has cpu speculation read the buffer. Although we have > hardware to protect the buffer, we still hope use software to fix it. > But *why* is this a problem? You are saying that the speculative accesses are not causing corruption, so they are causing other issues that you want to address. So which issues are we talking about here? > > 3) how this patch changes the current behavior > When call dma_direct_alloc() with DMA_ATTR_NO_KERNEL_MAPPING, then > remove the kernel mapping which belong to the buffer. > > > 4) why the new behavior fixes your problem. > If I understand correctly, want to block cpu speculation, then need > unmap the buffer at stage 1 and stage 2 page table and tlb invalidate. > This patch is to do stage 1 unmap at EL1. > > > > > There is no penalty for using too many words. > > Thanks. > Walter >
diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h index f36be5166c19..6c7d1683339c 100644 --- a/include/linux/set_memory.h +++ b/include/linux/set_memory.h @@ -7,11 +7,16 @@ #ifdef CONFIG_ARCH_HAS_SET_MEMORY #include <asm/set_memory.h> + +#ifndef CONFIG_RODATA_FULL_DEFAULT_ENABLED +static inline int set_memory_valid(unsigned long addr, int numpages, int enable) { return 0; } +#endif #else static inline int set_memory_ro(unsigned long addr, int numpages) { return 0; } static inline int set_memory_rw(unsigned long addr, int numpages) { return 0; } static inline int set_memory_x(unsigned long addr, int numpages) { return 0; } static inline int set_memory_nx(unsigned long addr, int numpages) { return 0; } +static inline int set_memory_valid(unsigned long addr, int numpages, int enable) { return 0; } #endif #ifndef CONFIG_ARCH_HAS_SET_DIRECT_MAP diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 4c6c5e0635e3..d5d03b51b708 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -155,6 +155,7 @@ void *dma_direct_alloc(struct device *dev, size_t size, struct page *page; void *ret; int err; + unsigned long kaddr; size = PAGE_ALIGN(size); if (attrs & DMA_ATTR_NO_WARN) @@ -169,6 +170,11 @@ void *dma_direct_alloc(struct device *dev, size_t size, if (!PageHighMem(page)) arch_dma_prep_coherent(page, size); *dma_handle = phys_to_dma_direct(dev, page_to_phys(page)); + if (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) { + kaddr = (unsigned long)phys_to_virt(dma_to_phys(dev, *dma_handle)); + /* page remove kernel mapping for arm64 */ + set_memory_valid(kaddr, size >> PAGE_SHIFT, 0); + } /* return the page pointer as the opaque cookie */ return page; } @@ -275,9 +281,16 @@ void dma_direct_free(struct device *dev, size_t size, void *cpu_addr, dma_addr_t dma_addr, unsigned long attrs) { unsigned int page_order = get_order(size); + unsigned long kaddr; if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) && !force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev)) { + if (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) { + size = PAGE_ALIGN(size); + kaddr = (unsigned long)phys_to_virt(dma_to_phys(dev, dma_addr)); + /* page create kernel mapping for arm64 */ + set_memory_valid(kaddr, size >> PAGE_SHIFT, 1); + } /* cpu_addr is a struct page cookie, not a kernel address */ dma_free_contiguous(dev, cpu_addr, size); return;
When the allocated buffers use dma coherent memory with DMA_ATTR_NO_KERNEL_MAPPING, then its kernel mapping is exist. The caller use that DMA_ATTR_NO_KERNEL_MAPPING mean they can't rely on kernel mapping, but removing kernel mapping have some improvements. The improvements are: a) Security improvement. In some cases, we don't hope the allocated buffer to be read by cpu speculative execution. Therefore, it need to remove kernel mapping, this patch improve DMA_ATTR_NO_KERNEL_MAPPING to remove a page from kernel mapping in order that cpu doesn't read it. b) Debugging improvement. If the allocated buffer map into user space, only access it in user space, nobody can access it in kernel space, so we can use this patch to see who try to access it in kernel space. This patch only works if the memory is mapping at page granularity in the linear region, so that current only support for ARM64. Signed-off-by: Walter Wu <walter-zh.wu@mediatek.com> Suggested-by: Christoph Hellwig <hch@lst.de> Suggested-by: Ard Biesheuvel <ardb@kernel.org> Cc: Christoph Hellwig <hch@lst.de> Cc: Marek Szyprowski <m.szyprowski@samsung.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Matthias Brugger <matthias.bgg@gmail.com> Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org> --- v2: 1. modify commit message and fix the removing mapping for arm64 2. fix build error for x86 --- include/linux/set_memory.h | 5 +++++ kernel/dma/direct.c | 13 +++++++++++++ 2 files changed, 18 insertions(+)