[PATCH 0/3] ARMv7: mmu: fix setting eXecute Never for device memory

[PATCH 0/3] ARMv7: mmu: fix setting eXecute Never for device memory

[Ahmad Fatoum]

Greetings,

in 0198567c4 ("ARM: mmu: mark uncached regions as eXecute never on v7"),
I had my first attempt at supporting eXecute Never in barebox.
This was meant to prevent speculative execution from accessing
read-sensitive device memory and the erratic behavior it could entail.

The XN bit not only prevents speculation, but also any execution at all,
as the name suggests, so the patchset can be tested by just executing
the code and asserting that the prefetch abort occurs, something that
I unfortunately missed during the first time round.

This patchset rectifies this and now Prefetch Aborts are thrown as
expected. They weren't before barebox uses a domain with manager permissions
for all mappings. This means that no permission checks at all are conducted
and our new XN settings were without effect.

There are theoritical regressions with this patch: any ARMv7 barebox platform
that directly jumps into ROM code with the MMU enabled will cease to
work. Assuming all memory outside of the barebox text section and SDRAM to be
non-executable however seems the right thing to do. Platforms that do
call back into ROM code should explicitly indicate that they intend to
do so in the PBL.

These patches fix a cache corruption issue[1] I've observed on the i.MX6UL(L)
that resulted from speculative fetches into the MMDC region following the 512M
SDRAM on the EVKs.

This time I tested it by by jumping into IO memory with go -m, which I had
introduced in this patch:
https://www.spinics.net/lists/u-boot-v2/msg38947.html

Tested SoCs:

- i.MX6UL (Cortex-A7, barebox directly loaded into SDRAM)
- i.MX6Q  (Cortex-A9, barebox directly loaded into SDRAM)
- SAMA5D3 (Cortex-A5, barebox loaded into SRAM then SDRAM)

[1]: https://community.nxp.com/thread/511925

Cheers
Ahmad Fatoum (3):
  ARM: cache-armv7: remove duplicate domain initialization
  ARM: mmu: set R/W bits in ARMv7 translation table
  ARM: mmu: use client domain permissions to support ARMv7 eXecute Never

 arch/arm/cpu/cache-armv7.S |  2 --
 arch/arm/cpu/mmu-early.c   |  7 ++++++-
 arch/arm/cpu/mmu.c         | 18 ++++++++++++------
 arch/arm/cpu/mmu.h         |  1 +
 4 files changed, 19 insertions(+), 9 deletions(-)

-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/3] ARM: cache-armv7: remove duplicate domain initialization

[Ahmad Fatoum]

From: Ahmad Fatoum <ahmad@a3f.at>

We already call set_domain each time we do __mmu_cache_on. Writing the
DACR in the armv7 __mmu_cache_on is thus superfluous. Drop it.

This changes existing behavior, whereas all 16 memory domains had the same
access permissions set (manager) before, now only the first domain has.
This is ok, as we only ever use domain 0 in barebox and on non-armv7,
we don't bother with the other ones at all.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 arch/arm/cpu/cache-armv7.S | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/cpu/cache-armv7.S b/arch/arm/cpu/cache-armv7.S
index 6a8aff8bb12c..2aa34ab4d7d8 100644
--- a/arch/arm/cpu/cache-armv7.S
+++ b/arch/arm/cpu/cache-armv7.S
@@ -21,8 +21,6 @@ ENTRY(v7_mmu_cache_on)
 		orr	r0, r0, #1 << 25	@ big-endian page tables
 #endif
 		orrne	r0, r0, #1		@ MMU enabled
-		movne	r1, #-1
-		mcrne	p15, 0, r1, c3, c0, 0	@ load domain access control
 #endif
 		isb
 		mcr	p15, 0, r0, c1, c0, 0	@ load control register
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] ARM: mmu: set R/W bits in ARMv7 translation table

[Ahmad Fatoum]

From: Ahmad Fatoum <ahmad@a3f.at>

With barebox using the manager permissions for domain 0 that's used for
all page table entries and directories, we never had the need so far to
explicitly set R/W bits. We did so anyway for sections in the early MMU
code, but later on in the normal MMU setup, we didn't do so consistently.

In preparation for switching to DOMAIN_CLIENT for ARMv7, configure R/W
everywhere in normal MMU code as well.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 arch/arm/cpu/mmu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/cpu/mmu.c b/arch/arm/cpu/mmu.c
index 123e19e9e55c..f7158871fe6f 100644
--- a/arch/arm/cpu/mmu.c
+++ b/arch/arm/cpu/mmu.c
@@ -56,13 +56,14 @@ static inline void tlb_invalidate(void)
 	);
 }
 
-#define PTE_FLAGS_CACHED_V7 (PTE_EXT_TEX(1) | PTE_BUFFERABLE | PTE_CACHEABLE)
-#define PTE_FLAGS_WC_V7 (PTE_EXT_TEX(1) | PTE_EXT_XN)
-#define PTE_FLAGS_UNCACHED_V7 PTE_EXT_XN
+#define PTE_FLAGS_CACHED_V7 (PTE_EXT_TEX(1) | PTE_BUFFERABLE | PTE_CACHEABLE | \
+			     PTE_EXT_AP_URW_SRW)
+#define PTE_FLAGS_WC_V7 (PTE_EXT_TEX(1) | PTE_EXT_AP_URW_SRW | PTE_EXT_XN)
+#define PTE_FLAGS_UNCACHED_V7 (PTE_EXT_AP_URW_SRW | PTE_EXT_XN)
 #define PTE_FLAGS_CACHED_V4 (PTE_SMALL_AP_UNO_SRW | PTE_BUFFERABLE | PTE_CACHEABLE)
 #define PTE_FLAGS_UNCACHED_V4 PTE_SMALL_AP_UNO_SRW
-#define PGD_FLAGS_WC_V7 (PMD_SECT_TEX(1) | PMD_TYPE_SECT | PMD_SECT_BUFFERABLE | \
-			 PMD_SECT_XN)
+#define PGD_FLAGS_WC_V7 (PMD_SECT_TEX(1) | PMD_SECT_DEF_UNCACHED | \
+			 PMD_SECT_BUFFERABLE | PMD_SECT_XN)
 #define PGD_FLAGS_UNCACHED_V7 (PMD_SECT_DEF_UNCACHED | PMD_SECT_XN)
 
 /*
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] ARM: mmu: use client domain permissions to support ARMv7 eXecute Never

[Ahmad Fatoum]

From: Ahmad Fatoum <ahmad@a3f.at>

The ARM Architecture Reference Manual notes[1]:
> When using the Short-descriptor translation table format, the XN
> attribute is not checked for domains marked as Manager.
> Therefore, the system must not include read-sensitive memory in
> domains marked as Manager, because the XN bit does not prevent
> speculative fetches from a Manager domain.

To avoid speculative access to read-sensitive memory-mapped peripherals
on ARMv7, let's use client domain permissions for all memory, so the XN
bit (and also R/W bits) can function.
This aligns us with what Linux is doing on ARMv7.

This fixes cache corruption instances that had been observed on the
i.MX6UL(L) when the instruction prefetcher speculates into memory following
the end of a 512M SDRAM[2].

While this is not necessary to avoid speculative accesses on < ARMv7,
we could probably have everything there in client domain as well, but
due to lack of test coverage, we'll restrict the change to ARMv7.

[1]: B3.7.2 - Execute-never restrictions on instruction fetching
[2]: "Cache Corruption on MX6UL(L)": https://community.nxp.com/thread/511925

Fixes: 0198567c4 ("ARM: mmu: mark uncached regions as eXecute never on v7")
Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 arch/arm/cpu/mmu-early.c | 7 ++++++-
 arch/arm/cpu/mmu.c       | 7 ++++++-
 arch/arm/cpu/mmu.h       | 1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm/cpu/mmu-early.c b/arch/arm/cpu/mmu-early.c
index 2f5876fc46d8..7c30526b9499 100644
--- a/arch/arm/cpu/mmu-early.c
+++ b/arch/arm/cpu/mmu-early.c
@@ -29,7 +29,12 @@ void mmu_early_enable(unsigned long membase, unsigned long memsize,
 	arm_set_cache_functions();
 
 	set_ttbr(ttb);
-	set_domain(DOMAIN_MANAGER);
+
+	/* For the XN bit to take effect, we can't be using DOMAIN_MANAGER. */
+	if (cpu_architecture() >= CPU_ARCH_ARMv7)
+		set_domain(DOMAIN_CLIENT);
+	else
+		set_domain(DOMAIN_MANAGER);
 
 	/*
 	 * This marks the whole address space as uncachable as well as
diff --git a/arch/arm/cpu/mmu.c b/arch/arm/cpu/mmu.c
index f7158871fe6f..2c5c4b574928 100644
--- a/arch/arm/cpu/mmu.c
+++ b/arch/arm/cpu/mmu.c
@@ -446,7 +446,12 @@ void __mmu_init(bool mmu_on)
 		ttb = xmemalign(ARM_TTB_SIZE, ARM_TTB_SIZE);
 
 		set_ttbr(ttb);
-		set_domain(DOMAIN_MANAGER);
+
+		/* For the XN bit to take effect, we can't be using DOMAIN_MANAGER. */
+		if (cpu_architecture() >= CPU_ARCH_ARMv7)
+			set_domain(DOMAIN_CLIENT);
+		else
+			set_domain(DOMAIN_MANAGER);
 
 		create_flat_mapping(ttb);
 		__mmu_cache_flush();
diff --git a/arch/arm/cpu/mmu.h b/arch/arm/cpu/mmu.h
index c911ee209f51..6e7a4c0350a1 100644
--- a/arch/arm/cpu/mmu.h
+++ b/arch/arm/cpu/mmu.h
@@ -36,6 +36,7 @@ static inline void set_ttbr(void *ttb)
 	asm volatile ("mcr  p15,0,%0,c2,c0,0" : : "r"(ttb) /*:*/);
 }
 
+#define DOMAIN_CLIENT	1
 #define DOMAIN_MANAGER	3
 
 static inline void set_domain(unsigned val)
-- 
2.23.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/3] ARMv7: mmu: fix setting eXecute Never for device memory

[Sascha Hauer]

On Wed, Oct 09, 2019 at 06:40:06PM +0200, Ahmad Fatoum wrote:
> Greetings,
> 
> in 0198567c4 ("ARM: mmu: mark uncached regions as eXecute never on v7"),
> I had my first attempt at supporting eXecute Never in barebox.
> This was meant to prevent speculative execution from accessing
> read-sensitive device memory and the erratic behavior it could entail.
> 
> The XN bit not only prevents speculation, but also any execution at all,
> as the name suggests, so the patchset can be tested by just executing
> the code and asserting that the prefetch abort occurs, something that
> I unfortunately missed during the first time round.
> 
> This patchset rectifies this and now Prefetch Aborts are thrown as
> expected. They weren't before barebox uses a domain with manager permissions
> for all mappings. This means that no permission checks at all are conducted
> and our new XN settings were without effect.
> 
> There are theoritical regressions with this patch: any ARMv7 barebox platform
> that directly jumps into ROM code with the MMU enabled will cease to
> work. Assuming all memory outside of the barebox text section and SDRAM to be
> non-executable however seems the right thing to do. Platforms that do
> call back into ROM code should explicitly indicate that they intend to
> do so in the PBL.
> 
> These patches fix a cache corruption issue[1] I've observed on the i.MX6UL(L)
> that resulted from speculative fetches into the MMDC region following the 512M
> SDRAM on the EVKs.
> 
> This time I tested it by by jumping into IO memory with go -m, which I had
> introduced in this patch:
> https://www.spinics.net/lists/u-boot-v2/msg38947.html
> 
> Tested SoCs:
> 
> - i.MX6UL (Cortex-A7, barebox directly loaded into SDRAM)
> - i.MX6Q  (Cortex-A9, barebox directly loaded into SDRAM)
> - SAMA5D3 (Cortex-A5, barebox loaded into SRAM then SDRAM)
> 
> [1]: https://community.nxp.com/thread/511925
> 
> Cheers
> Ahmad Fatoum (3):
>   ARM: cache-armv7: remove duplicate domain initialization
>   ARM: mmu: set R/W bits in ARMv7 translation table
>   ARM: mmu: use client domain permissions to support ARMv7 eXecute Never

Finally this is resolved \o/

Thanks Ahmad. Applied.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox