diff mbox

ARM: dma-mapping: don't allow DMA mappings to be marked executable

Message ID 20131024070547.GA16735@n2100.arm.linux.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Russell King - ARM Linux Oct. 24, 2013, 7:05 a.m. UTC 
While looking at some features which Kees talked about on Tuesday, I
discovered that our DMA mappings were incorrectly marked executable,
which means that they can be subject to speculative instruction
prefetches.  This turns off executable permission for these mappings.
I'm intending to push this for v3.12 as a fix.

8<====
From: Russell King <rmk+kernel@arm.linux.org.uk>
ARM: dma-mapping: don't allow DMA mappings to be marked executable

DMA mapping permissions were being derived from pgprot_kernel directly
without using PAGE_KERNEL.  This causes them to be marked with executable
permission, which is not what we want.  Fix this.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mm/dma-mapping.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Comments

Catalin Marinas Oct. 24, 2013, 9:04 a.m. UTC | #1 
Hi Russell,

On Thu, 2013-10-24 at 08:05 +0100, Russell King - ARM Linux wrote:
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -688,7 +688,7 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
>  void *arm_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
>  		    gfp_t gfp, struct dma_attrs *attrs)
>  {
> -	pgprot_t prot = __get_dma_pgprot(attrs, pgprot_kernel);
> +	pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL);

I think we lose the shareability attribute we add to pgprot_kernel when
SMP. So this creates a mismatched aliases and could have implications on
the barrier use (though I think we use the full system DSB in most cases
related to DMA). But architecturally I would feel better if we have the
same shareability domain.
Russell King - ARM Linux Oct. 24, 2013, 9:33 a.m. UTC | #2 
On Thu, Oct 24, 2013 at 10:04:23AM +0100, Catalin Marinas wrote:
> Hi Russell,
> 
> On Thu, 2013-10-24 at 08:05 +0100, Russell King - ARM Linux wrote:
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -688,7 +688,7 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
> >  void *arm_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
> >  		    gfp_t gfp, struct dma_attrs *attrs)
> >  {
> > -	pgprot_t prot = __get_dma_pgprot(attrs, pgprot_kernel);
> > +	pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL);
> 
> I think we lose the shareability attribute we add to pgprot_kernel when
> SMP. So this creates a mismatched aliases and could have implications on
> the barrier use (though I think we use the full system DSB in most cases
> related to DMA). But architecturally I would feel better if we have the
> same shareability domain.

We don't.

#define _MOD_PROT(p, b) __pgprot(pgprot_val(p) | (b))

#define PAGE_KERNEL             _MOD_PROT(pgprot_kernel, L_PTE_XN)
#define PAGE_KERNEL_EXEC        pgprot_kernel

PAGE_KERNEL is used in generic code to setup kernel mappings by things
like vmalloc() etc.
Catalin Marinas Oct. 24, 2013, 10:14 a.m. UTC | #3 
On Thu, 2013-10-24 at 10:33 +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 24, 2013 at 10:04:23AM +0100, Catalin Marinas wrote:
> > On Thu, 2013-10-24 at 08:05 +0100, Russell King - ARM Linux wrote:
> > > --- a/arch/arm/mm/dma-mapping.c
> > > +++ b/arch/arm/mm/dma-mapping.c
> > > @@ -688,7 +688,7 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
> > >  void *arm_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
> > >  		    gfp_t gfp, struct dma_attrs *attrs)
> > >  {
> > > -	pgprot_t prot = __get_dma_pgprot(attrs, pgprot_kernel);
> > > +	pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL);
> > 
> > I think we lose the shareability attribute we add to pgprot_kernel when
> > SMP. So this creates a mismatched aliases and could have implications on
> > the barrier use (though I think we use the full system DSB in most cases
> > related to DMA). But architecturally I would feel better if we have the
> > same shareability domain.
> 
> We don't.
> 
> #define _MOD_PROT(p, b) __pgprot(pgprot_val(p) | (b))
> 
> #define PAGE_KERNEL             _MOD_PROT(pgprot_kernel, L_PTE_XN)
> #define PAGE_KERNEL_EXEC        pgprot_kernel
> 
> PAGE_KERNEL is used in generic code to setup kernel mappings by things
> like vmalloc() etc.

Ah, yes, it works correctly then.
diff mbox

Patch

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 7f9b179..838e871 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -688,7 +688,7 @@  static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
 void *arm_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
 		    gfp_t gfp, struct dma_attrs *attrs)
 {
-	pgprot_t prot = __get_dma_pgprot(attrs, pgprot_kernel);
+	pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL);
 	void *memory;
 
 	if (dma_alloc_from_coherent(dev, size, handle, &memory))
@@ -701,7 +701,7 @@  void *arm_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
 static void *arm_coherent_dma_alloc(struct device *dev, size_t size,
 	dma_addr_t *handle, gfp_t gfp, struct dma_attrs *attrs)
 {
-	pgprot_t prot = __get_dma_pgprot(attrs, pgprot_kernel);
+	pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL);
 	void *memory;
 
 	if (dma_alloc_from_coherent(dev, size, handle, &memory))