[PATCH v2 0/6] Add support for privileged mappings

[PATCH v2 0/6] Add support for privileged mappings

[Mitchel Humpherys]

The following patch to the ARM SMMU driver:

    commit d346180e70b91b3d5a1ae7e5603e65593d4622bc
    Author: Robin Murphy <robin.murphy@arm.com>
    Date:   Tue Jan 26 18:06:34 2016 +0000
    
        iommu/arm-smmu: Treat all device transactions as unprivileged

started forcing all SMMU transactions to come through as "unprivileged".
The rationale given was that:

  (1) There is no way in the IOMMU API to even request privileged mappings.

  (2) It's difficult to implement a DMA mapper that correctly models the
      ARM VMSAv8 behavior of unprivileged-writeable =>
      privileged-execute-never.

This series rectifies (1) by introducing an IOMMU API for privileged
mappings and implements it in io-pgtable-arm.

This series rectifies (2) by introducing a new dma attribute
(DMA_ATTR_PRIVILEGED_EXECUTABLE) for users of the DMA API that need
privileged, executable mappings, and implements it in the arm64 IOMMU DMA
mapper.  The one known user (pl330.c) is converted over to the new
attribute.

Jordan and Jeremy can provide more info on the use case if needed, but the
high level is that it's a security feature to prevent attacks such as [1].

[1] https://github.com/robclark/kilroy

Changelog:

  v1..v2

    - Added a new DMA attribute to make executable privileged mappings
      work, and use that in the pl330 driver (suggested by Will).


Jeremy Gebben (1):
  iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

Mitchel Humpherys (5):
  iommu: add IOMMU_PRIV attribute
  Revert "iommu/arm-smmu: Treat all device transactions as unprivileged"
  common: DMA-mapping: add DMA_ATTR_PRIVILEGED_EXECUTABLE attribute
  arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED_EXECUTABLE
  dmaengine: pl330: Make sure microcode is privileged-executable

 Documentation/DMA-attributes.txt |  9 +++++++++
 arch/arm64/mm/dma-mapping.c      |  6 +++---
 drivers/dma/pl330.c              |  7 +++++--
 drivers/iommu/arm-smmu.c         |  5 +----
 drivers/iommu/dma-iommu.c        | 15 +++++++++++----
 drivers/iommu/io-pgtable-arm.c   | 16 +++++++++++-----
 include/linux/dma-attrs.h        |  1 +
 include/linux/dma-iommu.h        |  3 ++-
 include/linux/iommu.h            |  1 +
 9 files changed, 44 insertions(+), 19 deletions(-)

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 1/6] iommu: add IOMMU_PRIV attribute

[Mitchel Humpherys]

Add the IOMMU_PRIV attribute, which is used to indicate privileged
mappings.

Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
---
 include/linux/iommu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 664683aedcce..01c9f2667f2b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -31,6 +31,7 @@
 #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
 #define IOMMU_NOEXEC	(1 << 3)
 #define IOMMU_MMIO	(1 << 4) /* e.g. things like MSI doorbells */
+#define IOMMU_PRIV	(1 << 5)
 
 struct iommu_ops;
 struct iommu_group;
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/6] iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

[Mitchel Humpherys]

From: Jeremy Gebben <jgebben@codeaurora.org>

Allow the creation of privileged mode mappings, for stage 1 only.

Signed-off-by: Jeremy Gebben <jgebben@codeaurora.org>
---
 drivers/iommu/io-pgtable-arm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index a1ed1b73fed4..e9e7dd179708 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -101,8 +101,10 @@
 					 ARM_LPAE_PTE_ATTR_HI_MASK)
 
 /* Stage-1 PTE */
-#define ARM_LPAE_PTE_AP_UNPRIV		(((arm_lpae_iopte)1) << 6)
-#define ARM_LPAE_PTE_AP_RDONLY		(((arm_lpae_iopte)2) << 6)
+#define ARM_LPAE_PTE_AP_PRIV_RW		(((arm_lpae_iopte)0) << 6)
+#define ARM_LPAE_PTE_AP_RW		(((arm_lpae_iopte)1) << 6)
+#define ARM_LPAE_PTE_AP_PRIV_RO		(((arm_lpae_iopte)2) << 6)
+#define ARM_LPAE_PTE_AP_RO		(((arm_lpae_iopte)3) << 6)
 #define ARM_LPAE_PTE_ATTRINDX_SHIFT	2
 #define ARM_LPAE_PTE_nG			(((arm_lpae_iopte)1) << 11)
 
@@ -350,10 +352,14 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 
 	if (data->iop.fmt == ARM_64_LPAE_S1 ||
 	    data->iop.fmt == ARM_32_LPAE_S1) {
-		pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
+		pte = ARM_LPAE_PTE_nG;
 
-		if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
-			pte |= ARM_LPAE_PTE_AP_RDONLY;
+		if (prot & IOMMU_WRITE)
+			pte |= (prot & IOMMU_PRIV) ? ARM_LPAE_PTE_AP_PRIV_RW
+					: ARM_LPAE_PTE_AP_RW;
+		else
+			pte |= (prot & IOMMU_PRIV) ? ARM_LPAE_PTE_AP_PRIV_RO
+					: ARM_LPAE_PTE_AP_RO;
 
 		if (prot & IOMMU_MMIO)
 			pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/6] Revert "iommu/arm-smmu: Treat all device transactions as unprivileged"

[Mitchel Humpherys]

This reverts commit d346180e70b9 ("iommu/arm-smmu: Treat all device
transactions as unprivileged") since some platforms actually make use of
privileged transactions.

Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 9345a3fcb706..d0627ef26b05 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -178,9 +178,6 @@
 #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
 #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
 
-#define S2CR_PRIVCFG_SHIFT		24
-#define S2CR_PRIVCFG_UNPRIV		(2 << S2CR_PRIVCFG_SHIFT)
-
 /* Context bank attribute registers */
 #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
 #define CBAR_VMID_SHIFT			0
@@ -1175,7 +1172,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 		u32 idx, s2cr;
 
 		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
-		s2cr = S2CR_TYPE_TRANS | S2CR_PRIVCFG_UNPRIV |
+		s2cr = S2CR_TYPE_TRANS |
 		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/6] common: DMA-mapping: add DMA_ATTR_PRIVILEGED_EXECUTABLE attribute

[Mitchel Humpherys]

This patch adds the DMA_ATTR_PRIVILEGED_EXECUTABLE attribute to the
DMA-mapping subsystem.

Some architectures require that writable mappings also be non-executable at
lesser-privileged levels of execution.  This attribute is used to indicate
to the DMA-mapping subsystem that it should do whatever is necessary to
ensure that the buffer is executable at an elevated privilege level (by
making it read-only at the lesser-privileged levels, for example).

Cc: linux-doc at vger.kernel.org
Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
---
 Documentation/DMA-attributes.txt | 9 +++++++++
 include/linux/dma-attrs.h        | 1 +
 2 files changed, 10 insertions(+)

diff --git a/Documentation/DMA-attributes.txt b/Documentation/DMA-attributes.txt
index e8cf9cf873b3..6a22d4307008 100644
--- a/Documentation/DMA-attributes.txt
+++ b/Documentation/DMA-attributes.txt
@@ -126,3 +126,12 @@ means that we won't try quite as hard to get them.
 
 NOTE: At the moment DMA_ATTR_ALLOC_SINGLE_PAGES is only implemented on ARM,
 though ARM64 patches will likely be posted soon.
+
+DMA_ATTR_PRIVILEGED_EXECUTABLE
+------------------------------
+
+Some architectures require that writable mappings also be non-executable at
+lesser-privileged levels of execution.  This attribute is used to indicate
+to the DMA-mapping subsystem that it should do whatever is necessary to
+ensure that the buffer is executable at an elevated privilege level (by
+making it read-only at the lesser-privileged levels, for example).
diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h
index 5246239a4953..8cf4dff6185b 100644
--- a/include/linux/dma-attrs.h
+++ b/include/linux/dma-attrs.h
@@ -19,6 +19,7 @@ enum dma_attr {
 	DMA_ATTR_SKIP_CPU_SYNC,
 	DMA_ATTR_FORCE_CONTIGUOUS,
 	DMA_ATTR_ALLOC_SINGLE_PAGES,
+	DMA_ATTR_PRIVILEGED_EXECUTABLE,
 	DMA_ATTR_MAX,
 };
 
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 5/6] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED_EXECUTABLE

[Mitchel Humpherys]

The newly added DMA_ATTR_PRIVILEGED_EXECUTABLE is useful for creating
mappings that are executable by privileged DMA engines.  Implement it in
dma-iommu.c so that the ARM64 DMA IOMMU mapper can make use of it.

Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
---
 arch/arm64/mm/dma-mapping.c |  6 +++---
 drivers/iommu/dma-iommu.c   | 15 +++++++++++----
 include/linux/dma-iommu.h   |  3 ++-
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index c566ec83719f..44f676268df6 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -543,7 +543,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 				 struct dma_attrs *attrs)
 {
 	bool coherent = is_device_dma_coherent(dev);
-	int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+	int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
 	size_t iosize = size;
 	void *addr;
 
@@ -697,7 +697,7 @@ static dma_addr_t __iommu_map_page(struct device *dev, struct page *page,
 				   struct dma_attrs *attrs)
 {
 	bool coherent = is_device_dma_coherent(dev);
-	int prot = dma_direction_to_prot(dir, coherent);
+	int prot = dma_direction_to_prot(dir, coherent, attrs);
 	dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
 
 	if (!iommu_dma_mapping_error(dev, dev_addr) &&
@@ -755,7 +755,7 @@ static int __iommu_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
 		__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
 
 	return iommu_dma_map_sg(dev, sgl, nelems,
-			dma_direction_to_prot(dir, coherent));
+			dma_direction_to_prot(dir, coherent, attrs));
 }
 
 static void __iommu_unmap_sg_attrs(struct device *dev,
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index ea5a9ebf0f78..ccc6219da228 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -132,23 +132,30 @@ EXPORT_SYMBOL(iommu_dma_init_domain);
  * dma_direction_to_prot - Translate DMA API directions to IOMMU API page flags
  * @dir: Direction of DMA transfer
  * @coherent: Is the DMA master cache-coherent?
+ * @attrs: DMA attributes for the mapping
  *
  * Return: corresponding IOMMU API page protection flags
  */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
+int dma_direction_to_prot(enum dma_data_direction dir, bool coherent,
+			  struct dma_attrs *attrs)
 {
 	int prot = coherent ? IOMMU_CACHE : 0;
 
 	switch (dir) {
 	case DMA_BIDIRECTIONAL:
-		return prot | IOMMU_READ | IOMMU_WRITE;
+		prot |= IOMMU_READ | IOMMU_WRITE;
 	case DMA_TO_DEVICE:
-		return prot | IOMMU_READ;
+		prot |= IOMMU_READ;
 	case DMA_FROM_DEVICE:
-		return prot | IOMMU_WRITE;
+		prot |= IOMMU_WRITE;
 	default:
 		return 0;
 	}
+	if (dma_get_attr(DMA_ATTR_PRIVILEGED_EXECUTABLE, attrs)) {
+		prot &= ~IOMMU_WRITE;
+		prot |= IOMMU_PRIV;
+	}
+	return prot;
 }
 
 static struct iova *__alloc_iova(struct iova_domain *iovad, size_t size,
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 8443bbb5c071..d5a37e58d29b 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -32,7 +32,8 @@ void iommu_put_dma_cookie(struct iommu_domain *domain);
 int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, u64 size);
 
 /* General helpers for DMA-API <-> IOMMU-API interaction */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent);
+int dma_direction_to_prot(enum dma_data_direction dir, bool coherent,
+			  struct dma_attrs *attrs);
 
 /*
  * These implement the bulk of the relevant DMA mapping callbacks, but require
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 6/6] dmaengine: pl330: Make sure microcode is privileged-executable

[Mitchel Humpherys]

The PL330 can perform privileged instruction fetches.  This can result
in SMMU permission faults on SMMUs that implement the ARMv8 VMSA, which
specifies that mappings that are writeable at one execution level shall
not be executable at any higher-privileged level.  Fix this by using the
DMA_ATTR_PRIVILEGED_EXECUTABLE attribute, which will ensure that the
microcode IOMMU mapping is not writeable.

Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Jassi Brar <jassi.brar@samsung.com>
Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
---
 drivers/dma/pl330.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 372b4359da97..25bc49d47c45 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1854,14 +1854,17 @@ static int dmac_alloc_resources(struct pl330_dmac *pl330)
 {
 	int chans = pl330->pcfg.num_chan;
 	int ret;
+	DEFINE_DMA_ATTRS(attrs);
 
+	dma_set_attr(DMA_ATTR_PRIVILEGED_EXECUTABLE, &attrs);
 	/*
 	 * Alloc MicroCode buffer for 'chans' Channel threads.
 	 * A channel's buffer offset is (Channel_Id * MCODE_BUFF_PERCHAN)
 	 */
-	pl330->mcode_cpu = dma_alloc_coherent(pl330->ddma.dev,
+	pl330->mcode_cpu = dma_alloc_attrs(pl330->ddma.dev,
 				chans * pl330->mcbufsz,
-				&pl330->mcode_bus, GFP_KERNEL);
+				&pl330->mcode_bus, GFP_KERNEL,
+				&attrs);
 	if (!pl330->mcode_cpu) {
 		dev_err(pl330->ddma.dev, "%s:%d Can't allocate memory!\n",
 			__func__, __LINE__);
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 0/6] Add support for privileged mappings

[Robin Murphy]

Hey Mitch,

Thanks for having the necessary go at the DMA API - I think the series
looks broadly workable now.

On 09/07/16 03:09, Mitchel Humpherys wrote:
> The following patch to the ARM SMMU driver:
> 
>     commit d346180e70b91b3d5a1ae7e5603e65593d4622bc
>     Author: Robin Murphy <robin.murphy@arm.com>
>     Date:   Tue Jan 26 18:06:34 2016 +0000
>     
>         iommu/arm-smmu: Treat all device transactions as unprivileged
> 
> started forcing all SMMU transactions to come through as "unprivileged".
> The rationale given was that:
> 
>   (1) There is no way in the IOMMU API to even request privileged mappings.
> 
>   (2) It's difficult to implement a DMA mapper that correctly models the
>       ARM VMSAv8 behavior of unprivileged-writeable =>
>       privileged-execute-never.
> 
> This series rectifies (1) by introducing an IOMMU API for privileged
> mappings and implements it in io-pgtable-arm.
> 
> This series rectifies (2) by introducing a new dma attribute
> (DMA_ATTR_PRIVILEGED_EXECUTABLE) for users of the DMA API that need
> privileged, executable mappings, and implements it in the arm64 IOMMU DMA
> mapper.  The one known user (pl330.c) is converted over to the new
> attribute.
> 
> Jordan and Jeremy can provide more info on the use case if needed, but the
> high level is that it's a security feature to prevent attacks such as [1].

My understanding of that hack is that it involves switching the GPU into
privileged mode to get at the mapping of the IOMMU registers in the
first place, so I don't see at a glance how having privileged mappings
defends against that. What it clearly does do, however, is get us _to_
the point where it's necessary to do such a privilege switch in the
first place, as opposed to everything being trivially wide-open, which
is no bad thing.

Robin.

> 
> [1] https://github.com/robclark/kilroy
> 
> Changelog:
> 
>   v1..v2
> 
>     - Added a new DMA attribute to make executable privileged mappings
>       work, and use that in the pl330 driver (suggested by Will).
> 
> 
> Jeremy Gebben (1):
>   iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag
> 
> Mitchel Humpherys (5):
>   iommu: add IOMMU_PRIV attribute
>   Revert "iommu/arm-smmu: Treat all device transactions as unprivileged"
>   common: DMA-mapping: add DMA_ATTR_PRIVILEGED_EXECUTABLE attribute
>   arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED_EXECUTABLE
>   dmaengine: pl330: Make sure microcode is privileged-executable
> 
>  Documentation/DMA-attributes.txt |  9 +++++++++
>  arch/arm64/mm/dma-mapping.c      |  6 +++---
>  drivers/dma/pl330.c              |  7 +++++--
>  drivers/iommu/arm-smmu.c         |  5 +----
>  drivers/iommu/dma-iommu.c        | 15 +++++++++++----
>  drivers/iommu/io-pgtable-arm.c   | 16 +++++++++++-----
>  include/linux/dma-attrs.h        |  1 +
>  include/linux/dma-iommu.h        |  3 ++-
>  include/linux/iommu.h            |  1 +
>  9 files changed, 44 insertions(+), 19 deletions(-)
> 

[PATCH v2 1/6] iommu: add IOMMU_PRIV attribute

[Robin Murphy]

On 09/07/16 03:09, Mitchel Humpherys wrote:
> Add the IOMMU_PRIV attribute, which is used to indicate privileged
> mappings.
> 
> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
> ---
>  include/linux/iommu.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 664683aedcce..01c9f2667f2b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -31,6 +31,7 @@
>  #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
>  #define IOMMU_NOEXEC	(1 << 3)
>  #define IOMMU_MMIO	(1 << 4) /* e.g. things like MSI doorbells */
> +#define IOMMU_PRIV	(1 << 5)

I agree on the IOMMU_PRIV name, but can't help thinking it might be
worth a brief comment to make it abundantly clear@a glance that it
isn't "private" or "priority 5" or whatever ;)

Robin.

>  struct iommu_ops;
>  struct iommu_group;
> 

[PATCH v2 2/6] iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag

[Robin Murphy]

On 09/07/16 03:09, Mitchel Humpherys wrote:
> From: Jeremy Gebben <jgebben@codeaurora.org>
> 
> Allow the creation of privileged mode mappings, for stage 1 only.
> 
> Signed-off-by: Jeremy Gebben <jgebben@codeaurora.org>
> ---
>  drivers/iommu/io-pgtable-arm.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index a1ed1b73fed4..e9e7dd179708 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -101,8 +101,10 @@
>  					 ARM_LPAE_PTE_ATTR_HI_MASK)
>  
>  /* Stage-1 PTE */
> -#define ARM_LPAE_PTE_AP_UNPRIV		(((arm_lpae_iopte)1) << 6)
> -#define ARM_LPAE_PTE_AP_RDONLY		(((arm_lpae_iopte)2) << 6)
> +#define ARM_LPAE_PTE_AP_PRIV_RW		(((arm_lpae_iopte)0) << 6)
> +#define ARM_LPAE_PTE_AP_RW		(((arm_lpae_iopte)1) << 6)
> +#define ARM_LPAE_PTE_AP_PRIV_RO		(((arm_lpae_iopte)2) << 6)
> +#define ARM_LPAE_PTE_AP_RO		(((arm_lpae_iopte)3) << 6)

I'd much rather keep the existing per-bit definitions and compose them
in the code below (which by the look of it should then become a simple
one-liner of making the OR-ing in of AP_UNPRIV conditional.)

Robin.

>  #define ARM_LPAE_PTE_ATTRINDX_SHIFT	2
>  #define ARM_LPAE_PTE_nG			(((arm_lpae_iopte)1) << 11)
>  
> @@ -350,10 +352,14 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
>  
>  	if (data->iop.fmt == ARM_64_LPAE_S1 ||
>  	    data->iop.fmt == ARM_32_LPAE_S1) {
> -		pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
> +		pte = ARM_LPAE_PTE_nG;
>  
> -		if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
> -			pte |= ARM_LPAE_PTE_AP_RDONLY;
> +		if (prot & IOMMU_WRITE)
> +			pte |= (prot & IOMMU_PRIV) ? ARM_LPAE_PTE_AP_PRIV_RW
> +					: ARM_LPAE_PTE_AP_RW;
> +		else
> +			pte |= (prot & IOMMU_PRIV) ? ARM_LPAE_PTE_AP_PRIV_RO
> +					: ARM_LPAE_PTE_AP_RO;
>  
>  		if (prot & IOMMU_MMIO)
>  			pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
> 

[PATCH v2 3/6] Revert "iommu/arm-smmu: Treat all device transactions as unprivileged"

[Robin Murphy]

On 09/07/16 03:09, Mitchel Humpherys wrote:
> This reverts commit d346180e70b9 ("iommu/arm-smmu: Treat all device
> transactions as unprivileged") since some platforms actually make use of
> privileged transactions.

Super-nit: I know it makes bog-all difference in reality, but logically
it would be nicer to order this revert _after_ the alternative
functionality is fully in place. I'll tentatively retire the SMMUv3
equivalent[1] I currently have, too.

Robin.

[1]:http://article.gmane.org/gmane.linux.drivers.devicetree/175417

> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
> ---
>  drivers/iommu/arm-smmu.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 9345a3fcb706..d0627ef26b05 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -178,9 +178,6 @@
>  #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
>  #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
>  
> -#define S2CR_PRIVCFG_SHIFT		24
> -#define S2CR_PRIVCFG_UNPRIV		(2 << S2CR_PRIVCFG_SHIFT)
> -
>  /* Context bank attribute registers */
>  #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
>  #define CBAR_VMID_SHIFT			0
> @@ -1175,7 +1172,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>  		u32 idx, s2cr;
>  
>  		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
> -		s2cr = S2CR_TYPE_TRANS | S2CR_PRIVCFG_UNPRIV |
> +		s2cr = S2CR_TYPE_TRANS |
>  		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
>  		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
>  	}
> 

[PATCH v2 4/6] common: DMA-mapping: add DMA_ATTR_PRIVILEGED_EXECUTABLE attribute

[Robin Murphy]

On 09/07/16 03:09, Mitchel Humpherys wrote:
> This patch adds the DMA_ATTR_PRIVILEGED_EXECUTABLE attribute to the
> DMA-mapping subsystem.

DMA_ATTR_PRIVILEGED. We can worry about the (much bigger) executable vs.
NX issue some other time.

> Some architectures require that writable mappings also be non-executable at
> lesser-privileged levels of execution.  This attribute is used to indicate

I don't think "architectures" is really an appropriate fit here - for
what we're addressing, maybe something like "Some advanced peripherals
such as remote processors and GPUs perform accesses to DMA buffers in
both privileged "supervisor" and unprivileged "user" modes."

> to the DMA-mapping subsystem that it should do whatever is necessary to
> ensure that the buffer is executable at an elevated privilege level (by
> making it read-only at the lesser-privileged levels, for example).

and correspondingly "...the buffer is fully accessible at the elevated
privilege level (and ideally inaccessible or at least read-only at the
lesser-privileged levels)". What do you think?

I'm not sure what the latest status of Krzysztof's dma_attrs
coversion[1] is, but if you weren't aware of it this is just a heads-up
that there might need to be some coordination there.

What would be really cool if we could somehow remember which buffers
were allocated as privileged and refuse to expose them to userspace in
dma_mmap_attrs(), but I don't a feasible way to implement that without
impractically massive changes.

Robin.

[1]:http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.freedreno/164

> Cc: linux-doc at vger.kernel.org
> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
> ---
>  Documentation/DMA-attributes.txt | 9 +++++++++
>  include/linux/dma-attrs.h        | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/DMA-attributes.txt b/Documentation/DMA-attributes.txt
> index e8cf9cf873b3..6a22d4307008 100644
> --- a/Documentation/DMA-attributes.txt
> +++ b/Documentation/DMA-attributes.txt
> @@ -126,3 +126,12 @@ means that we won't try quite as hard to get them.
>  
>  NOTE: At the moment DMA_ATTR_ALLOC_SINGLE_PAGES is only implemented on ARM,
>  though ARM64 patches will likely be posted soon.
> +
> +DMA_ATTR_PRIVILEGED_EXECUTABLE
> +------------------------------
> +
> +Some architectures require that writable mappings also be non-executable at
> +lesser-privileged levels of execution.  This attribute is used to indicate
> +to the DMA-mapping subsystem that it should do whatever is necessary to
> +ensure that the buffer is executable at an elevated privilege level (by
> +making it read-only at the lesser-privileged levels, for example).
> diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h
> index 5246239a4953..8cf4dff6185b 100644
> --- a/include/linux/dma-attrs.h
> +++ b/include/linux/dma-attrs.h
> @@ -19,6 +19,7 @@ enum dma_attr {
>  	DMA_ATTR_SKIP_CPU_SYNC,
>  	DMA_ATTR_FORCE_CONTIGUOUS,
>  	DMA_ATTR_ALLOC_SINGLE_PAGES,
> +	DMA_ATTR_PRIVILEGED_EXECUTABLE,
>  	DMA_ATTR_MAX,
>  };
>  
> 

[PATCH v2 0/6] Add support for privileged mappings

[Jordan Crouse]

On Mon, Jul 11, 2016 at 03:02:24PM +0100, Robin Murphy wrote:
> Hey Mitch,
> 
> Thanks for having the necessary go at the DMA API - I think the series
> looks broadly workable now.
> 
> On 09/07/16 03:09, Mitchel Humpherys wrote:
> > The following patch to the ARM SMMU driver:
> > 
> >     commit d346180e70b91b3d5a1ae7e5603e65593d4622bc
> >     Author: Robin Murphy <robin.murphy@arm.com>
> >     Date:   Tue Jan 26 18:06:34 2016 +0000
> >     
> >         iommu/arm-smmu: Treat all device transactions as unprivileged
> > 
> > started forcing all SMMU transactions to come through as "unprivileged".
> > The rationale given was that:
> > 
> >   (1) There is no way in the IOMMU API to even request privileged mappings.
> > 
> >   (2) It's difficult to implement a DMA mapper that correctly models the
> >       ARM VMSAv8 behavior of unprivileged-writeable =>
> >       privileged-execute-never.
> > 
> > This series rectifies (1) by introducing an IOMMU API for privileged
> > mappings and implements it in io-pgtable-arm.
> > 
> > This series rectifies (2) by introducing a new dma attribute
> > (DMA_ATTR_PRIVILEGED_EXECUTABLE) for users of the DMA API that need
> > privileged, executable mappings, and implements it in the arm64 IOMMU DMA
> > mapper.  The one known user (pl330.c) is converted over to the new
> > attribute.
> > 
> > Jordan and Jeremy can provide more info on the use case if needed, but the
> > high level is that it's a security feature to prevent attacks such as [1].
> 
> My understanding of that hack is that it involves switching the GPU into
> privileged mode to get at the mapping of the IOMMU registers in the
> first place, so I don't see at a glance how having privileged mappings
> defends against that. What it clearly does do, however, is get us _to_
> the point where it's necessary to do such a privilege switch in the
> first place, as opposed to everything being trivially wide-open, which
> is no bad thing.

It was a two pronged attack.  First the attacker had to use the GPU to construct
a new pagetable and some various GPU command buffers and then they could switch
the context to the new pagetable to complete the attack.

The first part was made possible by vast tracts of read/write memory that is
mapped globally and accessible to all users so it was easy and legal to write
whatever the user wished including a pusedo-pagetable.

As you said, while the actual fix for the exploit was blocking access to the SMMU
registers it isn't a bad thing if we get in the habit of assuming that we shouldn't
leave a bunch of scratch memory around for somebody clever to take advantage of.

Jordan
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 5/6] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED_EXECUTABLE

[Robin Murphy]

On 09/07/16 03:09, Mitchel Humpherys wrote:
> The newly added DMA_ATTR_PRIVILEGED_EXECUTABLE is useful for creating
> mappings that are executable by privileged DMA engines.  Implement it in
> dma-iommu.c so that the ARM64 DMA IOMMU mapper can make use of it.
> 
> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
> ---
>  arch/arm64/mm/dma-mapping.c |  6 +++---
>  drivers/iommu/dma-iommu.c   | 15 +++++++++++----
>  include/linux/dma-iommu.h   |  3 ++-
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index c566ec83719f..44f676268df6 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -543,7 +543,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
>  				 struct dma_attrs *attrs)
>  {
>  	bool coherent = is_device_dma_coherent(dev);
> -	int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
> +	int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
>  	size_t iosize = size;
>  	void *addr;
>  
> @@ -697,7 +697,7 @@ static dma_addr_t __iommu_map_page(struct device *dev, struct page *page,
>  				   struct dma_attrs *attrs)
>  {
>  	bool coherent = is_device_dma_coherent(dev);
> -	int prot = dma_direction_to_prot(dir, coherent);
> +	int prot = dma_direction_to_prot(dir, coherent, attrs);
>  	dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
>  
>  	if (!iommu_dma_mapping_error(dev, dev_addr) &&
> @@ -755,7 +755,7 @@ static int __iommu_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
>  		__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
>  
>  	return iommu_dma_map_sg(dev, sgl, nelems,
> -			dma_direction_to_prot(dir, coherent));
> +			dma_direction_to_prot(dir, coherent, attrs));
>  }
>  
>  static void __iommu_unmap_sg_attrs(struct device *dev,
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index ea5a9ebf0f78..ccc6219da228 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -132,23 +132,30 @@ EXPORT_SYMBOL(iommu_dma_init_domain);
>   * dma_direction_to_prot - Translate DMA API directions to IOMMU API page flags

Privilege really isn't a direction :(

If we're going to cram more into this function it really wants renaming
and redocumenting.

>   * @dir: Direction of DMA transfer
>   * @coherent: Is the DMA master cache-coherent?
> + * @attrs: DMA attributes for the mapping
>   *
>   * Return: corresponding IOMMU API page protection flags
>   */
> -int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
> +int dma_direction_to_prot(enum dma_data_direction dir, bool coherent,
> +			  struct dma_attrs *attrs)
>  {
>  	int prot = coherent ? IOMMU_CACHE : 0;
>  
>  	switch (dir) {
>  	case DMA_BIDIRECTIONAL:
> -		return prot | IOMMU_READ | IOMMU_WRITE;
> +		prot |= IOMMU_READ | IOMMU_WRITE;
>  	case DMA_TO_DEVICE:
> -		return prot | IOMMU_READ;
> +		prot |= IOMMU_READ;
>  	case DMA_FROM_DEVICE:
> -		return prot | IOMMU_WRITE;
> +		prot |= IOMMU_WRITE;
>  	default:
>  		return 0;
>  	}
> +	if (dma_get_attr(DMA_ATTR_PRIVILEGED_EXECUTABLE, attrs)) {
> +		prot &= ~IOMMU_WRITE;

Hey, we didn't say anything anywhere about anything being privileged
read-only! Frankly, I think this is going to create more problems that
it solves. Implementing IOMMU_PRIV as simply unprivileged no-access
should be sufficient.

Robin.

> +		prot |= IOMMU_PRIV;
> +	}
> +	return prot;
>  }
>  
>  static struct iova *__alloc_iova(struct iova_domain *iovad, size_t size,
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> index 8443bbb5c071..d5a37e58d29b 100644
> --- a/include/linux/dma-iommu.h
> +++ b/include/linux/dma-iommu.h
> @@ -32,7 +32,8 @@ void iommu_put_dma_cookie(struct iommu_domain *domain);
>  int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, u64 size);
>  
>  /* General helpers for DMA-API <-> IOMMU-API interaction */
> -int dma_direction_to_prot(enum dma_data_direction dir, bool coherent);
> +int dma_direction_to_prot(enum dma_data_direction dir, bool coherent,
> +			  struct dma_attrs *attrs);
>  
>  /*
>   * These implement the bulk of the relevant DMA mapping callbacks, but require
> 

[PATCH v2 6/6] dmaengine: pl330: Make sure microcode is privileged-executable

[Robin Murphy]

On 09/07/16 03:09, Mitchel Humpherys wrote:
> The PL330 can perform privileged instruction fetches.  This can result

Nit: "can" is a bit of an understatement. Instruction fetches on both
the manager and channel threads have the "privileged" and "instruction"
AxPROT bits hard-coded whether you like it or not. It's only the data
accesses by the channel threads which are in any way configurable.

Robin.

> in SMMU permission faults on SMMUs that implement the ARMv8 VMSA, which
> specifies that mappings that are writeable at one execution level shall
> not be executable at any higher-privileged level.  Fix this by using the
> DMA_ATTR_PRIVILEGED_EXECUTABLE attribute, which will ensure that the
> microcode IOMMU mapping is not writeable.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Jassi Brar <jassi.brar@samsung.com>
> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
> ---
>  drivers/dma/pl330.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 372b4359da97..25bc49d47c45 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -1854,14 +1854,17 @@ static int dmac_alloc_resources(struct pl330_dmac *pl330)
>  {
>  	int chans = pl330->pcfg.num_chan;
>  	int ret;
> +	DEFINE_DMA_ATTRS(attrs);
>  
> +	dma_set_attr(DMA_ATTR_PRIVILEGED_EXECUTABLE, &attrs);
>  	/*
>  	 * Alloc MicroCode buffer for 'chans' Channel threads.
>  	 * A channel's buffer offset is (Channel_Id * MCODE_BUFF_PERCHAN)
>  	 */
> -	pl330->mcode_cpu = dma_alloc_coherent(pl330->ddma.dev,
> +	pl330->mcode_cpu = dma_alloc_attrs(pl330->ddma.dev,
>  				chans * pl330->mcbufsz,
> -				&pl330->mcode_bus, GFP_KERNEL);
> +				&pl330->mcode_bus, GFP_KERNEL,
> +				&attrs);
>  	if (!pl330->mcode_cpu) {
>  		dev_err(pl330->ddma.dev, "%s:%d Can't allocate memory!\n",
>  			__func__, __LINE__);
>