[PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Patrick Delaunay]

Hi,

On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region
protected by a firewall. This region is reserved in device with "no-map"
property.

But sometime the platform boot failed in U-boot on a Cortex A7 access to
this region (depending of the binary and the issue can change with compiler
version or with code alignment), then the firewall raise a error,
for example:

E/TC:0   tzc_it_handler:19 TZC permission failure
E/TC:0   dump_fail_filter:420 Permission violation on filter 0
E/TC:0   dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged read,
         AXI ID 5c0
E/TC:0   Panic

After investigation, the forbidden access is a speculative request performed
by the Cortex A7 because all the DDR is mapped as MEMORY with CACHEABLE
property.

The issue is solved only when the region reserved by OP-TEE is no more
mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is
already done in Linux kernel.

I think that can be a general issue for ARM architecture: the no-map tag
in device should be respected by U-boot, so I propose a  generic solution
in arm/lib/cache-cp15.c:dram_bank_mmu_setup().

This serie is composed by 7 patches
- 1..4/7: preliminary steps to support flags in library in lmb
  (as it is done in memblock.c in Linux)
- 5/7: unitary test on the added feature in lmb lib
- 6/7: save the no-map flags in lmb when the device tree is parsed
- 7/7: update the generic behavior for "no-map" region in
       arm/lib/cache-cp15.c::dram_bank_mmu_setup()

It is a rebase on master branch of previous RFC [2].

I can change this last patch if this feature is note required by other
ARM architecture; it is a weak function so I can avoid to map the region
with "no-map" property in device tree only for STM32MP architecture
(in arch/arm/mach-stm32mp/cpu.c).

See also [1] which handle same speculative access on armv8 for area
with Executable attribute.

[1] http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-marek.bykowski at gmail.com/
[2] http://patchwork.ozlabs.org/project/uboot/list/?series=199486&archive=both&state=*

Regards
Patrick


Patrick Delaunay (7):
  lmb: Add support of flags for no-map properties
  lmb: add lmb_is_reserved_flags
  lmb: remove lmb_region.size
  lmb: add lmb_dump_region() function
  test: lmb: add test for lmb_reserve_flags
  image-fdt: save no-map parameter of reserve-memory
  arm: cache: cp15: don't map the reserved region with no-map property

 arch/arm/include/asm/system.h |   3 +
 arch/arm/lib/cache-cp15.c     |  19 ++++++-
 common/image-fdt.c            |  23 +++++---
 include/lmb.h                 |  22 +++++++-
 lib/lmb.c                     | 100 +++++++++++++++++++++++-----------
 test/lib/lmb.c                |  89 ++++++++++++++++++++++++++++++
 6 files changed, 212 insertions(+), 44 deletions(-)

-- 
2.17.1

[PATCH 1/7] lmb: Add support of flags for no-map properties

[Patrick Delaunay]

Add "flags" in lmp_property to save the "no-map" property of
reserved region and a new function lmb_reserve_flags() to check
this flag.

The default allocation use flags = LMB_NONE.

The adjacent reserved memory region are merged only when they have
the same flags value.

This patch is partially based on flags support done in Linux kernel
mm/memblock .c (previously lmb.c); it is why LMB_NOMAP = 0x4, it is
aligned with MEMBLOCK_NOMAP value.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 include/lmb.h | 20 ++++++++++++++++++++
 lib/lmb.c     | 52 ++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/include/lmb.h b/include/lmb.h
index e9f19b16ea..1c73f4851b 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -14,9 +14,20 @@
 
 #define MAX_LMB_REGIONS 8
 
+/**
+ * enum lmb_flags - definition of memory region attributes
+ * @LMB_NONE: no special request
+ * @LMB_NOMAP: don't add to mmu configuration
+ */
+enum lmb_flags {
+	LMB_NONE		= 0x0,	/* No special request */
+	LMB_NOMAP		= 0x4,	/* don't add to mmu config */
+};
+
 struct lmb_property {
 	phys_addr_t base;
 	phys_size_t size;
+	enum lmb_flags flags;
 };
 
 struct lmb_region {
@@ -37,6 +48,8 @@ extern void lmb_init_and_reserve_range(struct lmb *lmb, phys_addr_t base,
 				       phys_size_t size, void *fdt_blob);
 extern long lmb_add(struct lmb *lmb, phys_addr_t base, phys_size_t size);
 extern long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size);
+extern long lmb_reserve_flags(struct lmb *lmb, phys_addr_t base,
+			      phys_size_t size, enum lmb_flags flags);
 extern phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align);
 extern phys_addr_t lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align,
 			    phys_addr_t max_addr);
@@ -60,6 +73,13 @@ lmb_size_bytes(struct lmb_region *type, unsigned long region_nr)
 void board_lmb_reserve(struct lmb *lmb);
 void arch_lmb_reserve(struct lmb *lmb);
 
+/* Low level functions */
+
+static inline bool lmb_is_nomap(struct lmb_property *m)
+{
+	return !!(m->flags & LMB_NOMAP);
+}
+
 #endif /* __KERNEL__ */
 
 #endif /* _LINUX_LMB_H */
diff --git a/lib/lmb.c b/lib/lmb.c
index d126f8dc04..44ab9cede2 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -27,6 +27,8 @@ void lmb_dump_all_force(struct lmb *lmb)
 		       (unsigned long long)lmb->memory.region[i].base);
 		printf("		   .size   = 0x%llx\n",
 		       (unsigned long long)lmb->memory.region[i].size);
+		printf("		   .flags   = 0x%x\n",
+		       lmb->memory.region[i].flags);
 	}
 
 	printf("\n    reserved.cnt	   = 0x%lx\n", lmb->reserved.cnt);
@@ -37,6 +39,8 @@ void lmb_dump_all_force(struct lmb *lmb)
 		       (unsigned long long)lmb->reserved.region[i].base);
 		printf("		     .size = 0x%llx\n",
 		       (unsigned long long)lmb->reserved.region[i].size);
+		printf("		     .flags = 0x%x\n",
+		       lmb->reserved.region[i].flags);
 	}
 }
 
@@ -85,6 +89,7 @@ static void lmb_remove_region(struct lmb_region *rgn, unsigned long r)
 	for (i = r; i < rgn->cnt - 1; i++) {
 		rgn->region[i].base = rgn->region[i + 1].base;
 		rgn->region[i].size = rgn->region[i + 1].size;
+		rgn->region[i].flags = rgn->region[i + 1].flags;
 	}
 	rgn->cnt--;
 }
@@ -141,7 +146,8 @@ void lmb_init_and_reserve_range(struct lmb *lmb, phys_addr_t base,
 }
 
 /* This routine called with relocation disabled. */
-static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t size)
+static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base,
+				 phys_size_t size, enum lmb_flags flags)
 {
 	unsigned long coalesced = 0;
 	long adjacent, i;
@@ -149,6 +155,7 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	if (rgn->cnt == 0) {
 		rgn->region[0].base = base;
 		rgn->region[0].size = size;
+		rgn->region[0].flags = flags;
 		rgn->cnt = 1;
 		return 0;
 	}
@@ -157,18 +164,27 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	for (i = 0; i < rgn->cnt; i++) {
 		phys_addr_t rgnbase = rgn->region[i].base;
 		phys_size_t rgnsize = rgn->region[i].size;
+		phys_size_t rgnflags = rgn->region[i].flags;
 
-		if ((rgnbase == base) && (rgnsize == size))
-			/* Already have this region, so we're done */
-			return 0;
+		if (rgnbase == base && rgnsize == size) {
+			if (flags == rgnflags)
+				/* Already have this region, so we're done */
+				return 0;
+			else
+				return -1; /* regions with new flags */
+		}
 
 		adjacent = lmb_addrs_adjacent(base, size, rgnbase, rgnsize);
 		if (adjacent > 0) {
+			if (flags != rgnflags)
+				break;
 			rgn->region[i].base -= size;
 			rgn->region[i].size += size;
 			coalesced++;
 			break;
 		} else if (adjacent < 0) {
+			if (flags != rgnflags)
+				break;
 			rgn->region[i].size += size;
 			coalesced++;
 			break;
@@ -179,8 +195,10 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	}
 
 	if ((i < rgn->cnt - 1) && lmb_regions_adjacent(rgn, i, i + 1)) {
-		lmb_coalesce_regions(rgn, i, i + 1);
-		coalesced++;
+		if (rgn->region[i].flags == rgn->region[i + 1].flags) {
+			lmb_coalesce_regions(rgn, i, i + 1);
+			coalesced++;
+		}
 	}
 
 	if (coalesced)
@@ -193,9 +211,11 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 		if (base < rgn->region[i].base) {
 			rgn->region[i + 1].base = rgn->region[i].base;
 			rgn->region[i + 1].size = rgn->region[i].size;
+			rgn->region[i + 1].flags = rgn->region[i].flags;
 		} else {
 			rgn->region[i + 1].base = base;
 			rgn->region[i + 1].size = size;
+			rgn->region[i + 1].flags = flags;
 			break;
 		}
 	}
@@ -203,6 +223,7 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	if (base < rgn->region[0].base) {
 		rgn->region[0].base = base;
 		rgn->region[0].size = size;
+		rgn->region[0].flags = flags;
 	}
 
 	rgn->cnt++;
@@ -210,6 +231,12 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	return 0;
 }
 
+static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base,
+			   phys_size_t size)
+{
+	return lmb_add_region_flags(rgn, base, size, LMB_NONE);
+}
+
 /* This routine may be called with relocation disabled. */
 long lmb_add(struct lmb *lmb, phys_addr_t base, phys_size_t size)
 {
@@ -264,14 +291,21 @@ long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size)
 	 * beginging of the hole and add the region after hole.
 	 */
 	rgn->region[i].size = base - rgn->region[i].base;
-	return lmb_add_region(rgn, end + 1, rgnend - end);
+	return lmb_add_region_flags(rgn, end + 1, rgnend - end,
+				    rgn->region[i].flags);
 }
 
-long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size)
+long lmb_reserve_flags(struct lmb *lmb, phys_addr_t base, phys_size_t size,
+		       enum lmb_flags flags)
 {
 	struct lmb_region *_rgn = &(lmb->reserved);
 
-	return lmb_add_region(_rgn, base, size);
+	return lmb_add_region_flags(_rgn, base, size, flags);
+}
+
+long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size)
+{
+	return lmb_reserve_flags(lmb, base, size, LMB_NONE);
 }
 
 static long lmb_overlaps_region(struct lmb_region *rgn, phys_addr_t base,
-- 
2.17.1

[PATCH 2/7] lmb: add lmb_is_reserved_flags

[Patrick Delaunay]

Add a new function lmb_is_reserved_flags to check is a
address is reserved with a specific flags.

This function can be used to check if an address had be
reserved with no-map flags with:

lmb_is_reserved_flags(lmb, addr, LMB_NOMAP);

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 include/lmb.h |  1 +
 lib/lmb.c     | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/lmb.h b/include/lmb.h
index 1c73f4851b..68e9cc007a 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -59,6 +59,7 @@ extern phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
 				  phys_size_t size);
 extern phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr);
 extern int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr);
+extern int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags);
 extern long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size);
 
 extern void lmb_dump_all(struct lmb *lmb);
diff --git a/lib/lmb.c b/lib/lmb.c
index 44ab9cede2..d237d0b65f 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -440,7 +440,7 @@ phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr)
 	return 0;
 }
 
-int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
+int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags)
 {
 	int i;
 
@@ -448,11 +448,17 @@ int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
 		phys_addr_t upper = lmb->reserved.region[i].base +
 			lmb->reserved.region[i].size - 1;
 		if ((addr >= lmb->reserved.region[i].base) && (addr <= upper))
-			return 1;
+			return !!((lmb->reserved.region[i].flags & flags)
+				   == flags);
 	}
 	return 0;
 }
 
+int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
+{
+	return lmb_is_reserved_flags(lmb, addr, LMB_NONE);
+}
+
 __weak void board_lmb_reserve(struct lmb *lmb)
 {
 	/* please define platform specific board_lmb_reserve() */
-- 
2.17.1

[PATCH 3/7] lmb: remove lmb_region.size

[Patrick Delaunay]

Remove the unused field size of struct lmb_region as it is initialized to 0
and never used after in lmb library.

See Linux kernel commit 4734b594c6ca ("memblock: Remove memblock_type.size
and add memblock.memory_size instead")

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 include/lmb.h | 1 -
 lib/lmb.c     | 6 ------
 2 files changed, 7 deletions(-)

diff --git a/include/lmb.h b/include/lmb.h
index 68e9cc007a..9be422103c 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -32,7 +32,6 @@ struct lmb_property {
 
 struct lmb_region {
 	unsigned long cnt;
-	phys_size_t size;
 	struct lmb_property region[MAX_LMB_REGIONS+1];
 };
 
diff --git a/lib/lmb.c b/lib/lmb.c
index d237d0b65f..7f66a99884 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -20,8 +20,6 @@ void lmb_dump_all_force(struct lmb *lmb)
 
 	printf("lmb_dump_all:\n");
 	printf("    memory.cnt		   = 0x%lx\n", lmb->memory.cnt);
-	printf("    memory.size		   = 0x%llx\n",
-	       (unsigned long long)lmb->memory.size);
 	for (i = 0; i < lmb->memory.cnt; i++) {
 		printf("    memory.reg[0x%lx].base   = 0x%llx\n", i,
 		       (unsigned long long)lmb->memory.region[i].base);
@@ -32,8 +30,6 @@ void lmb_dump_all_force(struct lmb *lmb)
 	}
 
 	printf("\n    reserved.cnt	   = 0x%lx\n", lmb->reserved.cnt);
-	printf("    reserved.size	   = 0x%llx\n",
-	       (unsigned long long)lmb->reserved.size);
 	for (i = 0; i < lmb->reserved.cnt; i++) {
 		printf("    reserved.reg[0x%lx].base = 0x%llx\n", i,
 		       (unsigned long long)lmb->reserved.region[i].base);
@@ -105,9 +101,7 @@ static void lmb_coalesce_regions(struct lmb_region *rgn, unsigned long r1,
 void lmb_init(struct lmb *lmb)
 {
 	lmb->memory.cnt = 0;
-	lmb->memory.size = 0;
 	lmb->reserved.cnt = 0;
-	lmb->reserved.size = 0;
 }
 
 static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
-- 
2.17.1

[PATCH 4/7] lmb: add lmb_dump_region() function

[Patrick Delaunay]

Add lmb_dump_region() function, to simplify lmb_dump_all_force().
This patch is based on Linux memblock dump function.

A example of bdinfo output is:

.....
fdt_size    = 0x000146a0
FB base     = 0xfdd00000
lmb_dump_all:
 memory.cnt  = 0x1
 memory[0]	[0xc0000000-0xffffffff], 0x40000000 bytes flags: 0
 reserved.cnt  = 0x6
 reserved[0]	[0x10000000-0x10045fff], 0x00046000 bytes flags: 4
 reserved[1]	[0x30000000-0x3003ffff], 0x00040000 bytes flags: 4
 reserved[2]	[0x38000000-0x3800ffff], 0x00010000 bytes flags: 4
 reserved[3]	[0xe8000000-0xefffffff], 0x08000000 bytes flags: 4
 reserved[4]	[0xfbaea344-0xfdffffff], 0x02515cbc bytes flags: 0
 reserved[5]	[0xfe000000-0xffffffff], 0x02000000 bytes flags: 4
arch_number = 0x00000000
TLB addr    = 0xfdff0000
....

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 lib/lmb.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index 7f66a99884..70b597d979 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -14,32 +14,32 @@
 
 #define LMB_ALLOC_ANYWHERE	0
 
-void lmb_dump_all_force(struct lmb *lmb)
+static void lmb_dump_region(struct lmb_region *rgn, char *name)
 {
-	unsigned long i;
+	unsigned long long base, size, end;
+	enum lmb_flags flags;
+	int i;
 
-	printf("lmb_dump_all:\n");
-	printf("    memory.cnt		   = 0x%lx\n", lmb->memory.cnt);
-	for (i = 0; i < lmb->memory.cnt; i++) {
-		printf("    memory.reg[0x%lx].base   = 0x%llx\n", i,
-		       (unsigned long long)lmb->memory.region[i].base);
-		printf("		   .size   = 0x%llx\n",
-		       (unsigned long long)lmb->memory.region[i].size);
-		printf("		   .flags   = 0x%x\n",
-		       lmb->memory.region[i].flags);
-	}
+	printf(" %s.cnt  = 0x%lx\n", name, rgn->cnt);
 
-	printf("\n    reserved.cnt	   = 0x%lx\n", lmb->reserved.cnt);
-	for (i = 0; i < lmb->reserved.cnt; i++) {
-		printf("    reserved.reg[0x%lx].base = 0x%llx\n", i,
-		       (unsigned long long)lmb->reserved.region[i].base);
-		printf("		     .size = 0x%llx\n",
-		       (unsigned long long)lmb->reserved.region[i].size);
-		printf("		     .flags = 0x%x\n",
-		       lmb->reserved.region[i].flags);
+	for (i = 0; i < rgn->cnt; i++) {
+		base = rgn->region[i].base;
+		size = rgn->region[i].size;
+		end = base + size - 1;
+		flags = rgn->region[i].flags;
+
+		printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x\n",
+		       name, i, base, end, size, flags);
 	}
 }
 
+void lmb_dump_all_force(struct lmb *lmb)
+{
+	printf("lmb_dump_all:\n");
+	lmb_dump_region(&lmb->memory, "memory");
+	lmb_dump_region(&lmb->reserved, "reserved");
+}
+
 void lmb_dump_all(struct lmb *lmb)
 {
 #ifdef DEBUG
-- 
2.17.1

[PATCH 5/7] test: lmb: add test for lmb_reserve_flags

[Patrick Delaunay]

Add a test to check the management of reserved region with flags.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/test/lib/lmb.c b/test/lib/lmb.c
index 644ee78758..d7bd826190 100644
--- a/test/lib/lmb.c
+++ b/test/lib/lmb.c
@@ -659,3 +659,92 @@ static int lib_test_lmb_get_free_size(struct unit_test_state *uts)
 
 DM_TEST(lib_test_lmb_get_free_size,
 	UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+static int lib_test_lmb_flags(struct unit_test_state *uts)
+{
+	const phys_addr_t ram = 0x40000000;
+	const phys_size_t ram_size = 0x20000000;
+	struct lmb lmb;
+	long ret;
+
+	lmb_init(&lmb);
+
+	ret = lmb_add(&lmb, ram, ram_size);
+	ut_asserteq(ret, 0);
+
+	/* reserve, same flag */
+	ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
+		   0, 0, 0, 0);
+
+	/* reserve again, same flag */
+	ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
+		   0, 0, 0, 0);
+
+	/* reserve again, new flag */
+	ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NONE);
+	ut_asserteq(ret, -1);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
+		   0, 0, 0, 0);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+
+	/* merge after */
+	ret = lmb_reserve_flags(&lmb, 0x40020000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 1);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x20000,
+		   0, 0, 0, 0);
+
+	/* merge before */
+	ret = lmb_reserve_flags(&lmb, 0x40000000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 1);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40000000, 0x30000,
+		   0, 0, 0, 0);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+
+	ret = lmb_reserve_flags(&lmb, 0x40030000, 0x10000, LMB_NONE);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, 0x40000000, 0x30000,
+		   0x40030000, 0x10000, 0, 0);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0);
+
+	/* test that old API use LMB_NONE */
+	ret = lmb_reserve(&lmb, 0x40040000, 0x10000);
+	ut_asserteq(ret, 1);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, 0x40000000, 0x30000,
+		   0x40030000, 0x20000, 0, 0);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0);
+
+	ret = lmb_reserve_flags(&lmb, 0x40070000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 3, 0x40000000, 0x30000,
+		   0x40030000, 0x20000, 0x40070000, 0x10000);
+
+	ret = lmb_reserve_flags(&lmb, 0x40050000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 4, 0x40000000, 0x30000,
+		   0x40030000, 0x20000, 0x40050000, 0x10000);
+
+	/* merge with 2 adjacent regions */
+	ret = lmb_reserve_flags(&lmb, 0x40060000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 2);
+	ASSERT_LMB(&lmb, ram, ram_size, 3, 0x40000000, 0x30000,
+		   0x40030000, 0x20000, 0x40050000, 0x30000);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0);
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[2]), 1);
+
+	return 0;
+}
+
+DM_TEST(lib_test_lmb_flags,
+	UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.17.1

[PATCH 6/7] image-fdt: save no-map parameter of reserve-memory

[Patrick Delaunay]

Save the no-map information present in reserve-memory node to allow
correct handling when the MMU is configured in board to avoid
speculative access.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 common/image-fdt.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/common/image-fdt.c b/common/image-fdt.c
index 3d6935ad40..55b3593762 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -74,18 +74,20 @@ static const image_header_t *image_get_fdt(ulong fdt_addr)
 #endif
 
 static void boot_fdt_reserve_region(struct lmb *lmb, uint64_t addr,
-				    uint64_t size)
+				    uint64_t size, enum lmb_flags flags)
 {
 	long ret;
 
-	ret = lmb_reserve(lmb, addr, size);
+	ret = lmb_reserve_flags(lmb, addr, size, flags);
 	if (ret >= 0) {
-		debug("   reserving fdt memory region: addr=%llx size=%llx\n",
-		      (unsigned long long)addr, (unsigned long long)size);
+		debug("   reserving fdt memory region: addr=%llx size=%llx flags=%x\n",
+		      (unsigned long long)addr,
+		      (unsigned long long)size, flags);
 	} else {
 		puts("ERROR: reserving fdt memory region failed ");
-		printf("(addr=%llx size=%llx)\n",
-		       (unsigned long long)addr, (unsigned long long)size);
+		printf("(addr=%llx size=%llx flags=%x)\n",
+		       (unsigned long long)addr,
+		       (unsigned long long)size, flags);
 	}
 }
 
@@ -105,6 +107,7 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob)
 	int i, total, ret;
 	int nodeoffset, subnode;
 	struct fdt_resource res;
+	enum lmb_flags flags;
 
 	if (fdt_check_header(fdt_blob) != 0)
 		return;
@@ -114,7 +117,7 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob)
 	for (i = 0; i < total; i++) {
 		if (fdt_get_mem_rsv(fdt_blob, i, &addr, &size) != 0)
 			continue;
-		boot_fdt_reserve_region(lmb, addr, size);
+		boot_fdt_reserve_region(lmb, addr, size, LMB_NONE);
 	}
 
 	/* process reserved-memory */
@@ -126,9 +129,13 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob)
 			ret = fdt_get_resource(fdt_blob, subnode, "reg", 0,
 					       &res);
 			if (!ret && fdtdec_get_is_enabled(fdt_blob, subnode)) {
+				flags = LMB_NONE;
+				if (fdtdec_get_bool(fdt_blob, subnode,
+						    "no-map"))
+					flags = LMB_NOMAP;
 				addr = res.start;
 				size = res.end - res.start + 1;
-				boot_fdt_reserve_region(lmb, addr, size);
+				boot_fdt_reserve_region(lmb, addr, size, flags);
 			}
 
 			subnode = fdt_next_subnode(fdt_blob, subnode);
-- 
2.17.1

[PATCH 7/7] arm: cache: cp15: don't map the reserved region with no-map property

[Patrick Delaunay]

No more map the reserved region with "no-map" property by marking
the corresponding TLB entries with invalid entry (=0) to avoid
speculative access.

This patch fixes an issue on STM32MP15x where predictive read access
on secure DDR area are caught by OP-TEE.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 arch/arm/include/asm/system.h |  3 +++
 arch/arm/lib/cache-cp15.c     | 19 +++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index ce552944b7..932f12af1c 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -458,6 +458,7 @@ static inline void set_dacr(unsigned int val)
 
 /* options available for data cache on each page */
 enum dcache_option {
+	INVALID_ENTRY = 0,
 	DCACHE_OFF = TTB_SECT | TTB_SECT_MAIR(0) | TTB_SECT_XN_MASK,
 	DCACHE_WRITETHROUGH = TTB_SECT | TTB_SECT_MAIR(1),
 	DCACHE_WRITEBACK = TTB_SECT | TTB_SECT_MAIR(2),
@@ -488,6 +489,7 @@ enum dcache_option {
  *   1    1  1   Outer/Inner Write-Back, Read-Allocate Write-Allocate
  */
 enum dcache_option {
+	INVALID_ENTRY = 0,
 	DCACHE_OFF = TTB_SECT_DOMAIN(0) | TTB_SECT_XN_MASK | TTB_SECT,
 	DCACHE_WRITETHROUGH = DCACHE_OFF | TTB_SECT_C_MASK,
 	DCACHE_WRITEBACK = DCACHE_WRITETHROUGH | TTB_SECT_B_MASK,
@@ -497,6 +499,7 @@ enum dcache_option {
 #define TTB_SECT_AP		(3 << 10)
 /* options available for data cache on each page */
 enum dcache_option {
+	INVALID_ENTRY = 0,
 	DCACHE_OFF = 0x12,
 	DCACHE_WRITETHROUGH = 0x1a,
 	DCACHE_WRITEBACK = 0x1e,
diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c
index abd81d21c7..9e778dfd06 100644
--- a/arch/arm/lib/cache-cp15.c
+++ b/arch/arm/lib/cache-cp15.c
@@ -6,6 +6,7 @@
 
 #include <common.h>
 #include <cpu_func.h>
+#include <lmb.h>
 #include <log.h>
 #include <asm/system.h>
 #include <asm/cache.h>
@@ -105,18 +106,32 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size,
 __weak void dram_bank_mmu_setup(int bank)
 {
 	struct bd_info *bd = gd->bd;
+	struct lmb lmb;
 	int	i;
 
 	/* bd->bi_dram is available only after relocation */
 	if ((gd->flags & GD_FLG_RELOC) == 0)
 		return;
 
+	/*
+	 * don't allow cache on reserved memory tagged 'no-map' in DT
+	 * => avoid speculative access to "secure" data
+	 */
+	if (IS_ENABLED(CONFIG_LMB))
+		lmb_init_and_reserve(&lmb, bd, (void *)gd->fdt_blob);
+
 	debug("%s: bank: %d\n", __func__, bank);
 	for (i = bd->bi_dram[bank].start >> MMU_SECTION_SHIFT;
 	     i < (bd->bi_dram[bank].start >> MMU_SECTION_SHIFT) +
 		 (bd->bi_dram[bank].size >> MMU_SECTION_SHIFT);
-	     i++)
-		set_section_dcache(i, DCACHE_DEFAULT_OPTION);
+	     i++) {
+		if (IS_ENABLED(CONFIG_LMB) &&
+		    lmb_is_reserved_flags(&lmb, i << MMU_SECTION_SHIFT,
+					  LMB_NOMAP))
+			set_section_dcache(i, INVALID_ENTRY);
+		else
+			set_section_dcache(i, DCACHE_DEFAULT_OPTION);
+	}
 }
 
 /* to activate the MMU we need to set up virtual memory: use 1M areas */
-- 
2.17.1

[PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
>
> Hi,
>
> On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region
> protected by a firewall. This region is reserved in device with "no-map"
> property.
>
> But sometime the platform boot failed in U-boot on a Cortex A7 access to
> this region (depending of the binary and the issue can change with compiler
> version or with code alignment), then the firewall raise a error,
> for example:
>
> E/TC:0   tzc_it_handler:19 TZC permission failure
> E/TC:0   dump_fail_filter:420 Permission violation on filter 0
> E/TC:0   dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged read,
>          AXI ID 5c0
> E/TC:0   Panic
>
> After investigation, the forbidden access is a speculative request performed
> by the Cortex A7 because all the DDR is mapped as MEMORY with CACHEABLE
> property.
>
> The issue is solved only when the region reserved by OP-TEE is no more
> mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is
> already done in Linux kernel.
>

Spurious peculative accesses to device regions would be a severe
silicon bug, so I wonder what is going on here.

(Apologies if we are rehashing stuff here that has already been
discussed - I don't remember the details)

Are you sure that the speculative accesses were not caused by
misconfigured CPU or page tables, missing TLB maintenance, etc etc?
Because it really does smell like a software issue not a hardware
issue.

> I think that can be a general issue for ARM architecture: the no-map tag
> in device should be respected by U-boot, so I propose a  generic solution
> in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
>
> This serie is composed by 7 patches
> - 1..4/7: preliminary steps to support flags in library in lmb
>   (as it is done in memblock.c in Linux)
> - 5/7: unitary test on the added feature in lmb lib
> - 6/7: save the no-map flags in lmb when the device tree is parsed
> - 7/7: update the generic behavior for "no-map" region in
>        arm/lib/cache-cp15.c::dram_bank_mmu_setup()
>
> It is a rebase on master branch of previous RFC [2].
>
> I can change this last patch if this feature is note required by other
> ARM architecture; it is a weak function so I can avoid to map the region
> with "no-map" property in device tree only for STM32MP architecture
> (in arch/arm/mach-stm32mp/cpu.c).
>
> See also [1] which handle same speculative access on armv8 for area
> with Executable attribute.
>
> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-marek.bykowski at gmail.com/
> [2] http://patchwork.ozlabs.org/project/uboot/list/?series=199486&archive=both&state=*
>
> Regards
> Patrick
>
>
> Patrick Delaunay (7):
>   lmb: Add support of flags for no-map properties
>   lmb: add lmb_is_reserved_flags
>   lmb: remove lmb_region.size
>   lmb: add lmb_dump_region() function
>   test: lmb: add test for lmb_reserve_flags
>   image-fdt: save no-map parameter of reserve-memory
>   arm: cache: cp15: don't map the reserved region with no-map property
>
>  arch/arm/include/asm/system.h |   3 +
>  arch/arm/lib/cache-cp15.c     |  19 ++++++-
>  common/image-fdt.c            |  23 +++++---
>  include/lmb.h                 |  22 +++++++-
>  lib/lmb.c                     | 100 +++++++++++++++++++++++-----------
>  test/lib/lmb.c                |  89 ++++++++++++++++++++++++++++++
>  6 files changed, 212 insertions(+), 44 deletions(-)
>
> --
> 2.17.1
>

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ahmad Fatoum]

Hello Ard, Patrick,

On 10/7/20 12:26 PM, Ard Biesheuvel wrote:
>> The issue is solved only when the region reserved by OP-TEE is no more
>> mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is
>> already done in Linux kernel.
>>
> 
> Spurious peculative accesses to device regions would be a severe
> silicon bug, so I wonder what is going on here.
> 
> (Apologies if we are rehashing stuff here that has already been
> discussed - I don't remember the details)
> 
> Are you sure that the speculative accesses were not caused by
> misconfigured CPU or page tables, missing TLB maintenance, etc etc?
> Because it really does smell like a software issue not a hardware
> issue.

I debugged a similar issue a year ago on an i.MX6 UltraLite (also a Cortex-A7)
that turned to ultimately be caused by barebox not mapping I/O memory as
non-executable. This led to very interesting effects.

My findings[1] back then were that U-Boot did set the eXecute Never bit only on
OMAP, but not for other platforms.  So I could imagine this being the root cause
of Patrick's issues as well:
The CPU is speculatively executing from the region that the firewalled DRAM
is mapped at.

barebox now configures XN for non-RAM before it turns on the MMU. You should
do that as well (in ARM arch code, not only for stm32mp1). Additionally,
you will want to XN map the region where your OP-TEE sits at.

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

Cheers
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ahmad Fatoum]

Hello,

On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> My findings[1] back then were that U-Boot did set the eXecute Never bit only on
> OMAP, but not for other platforms.  So I could imagine this being the root cause
> of Patrick's issues as well:

Rereading my own link, my memory is a little less fuzzy: eXecute Never was being
set, but was without effect due Manager mode being set in the DACR:

> 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, we'll need U-Boot to use client domain permissions, so the XN
> bit can function.
 
> This issue has come up before and was fixed in de63ac278
> ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> It's equally applicable to all ARMv7-A platforms where caches are    
> enabled.
> [1]: B3.7.2 - Execute-never restrictions on instruction fetching

Hope this helps,
Ahmad

> The CPU is speculatively executing from the region that the firewalled DRAM
> is mapped at.
> 
> barebox now configures XN for non-RAM before it turns on the MMU. You should
> do that as well (in ARM arch code, not only for stm32mp1). Additionally,
> you will want to XN map the region where your OP-TEE sits at.
> 
> [1]: https://community.nxp.com/thread/511925
> 
> Cheers
> Ahmad
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello,
>
> On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > My findings[1] back then were that U-Boot did set the eXecute Never bit only on
> > OMAP, but not for other platforms.  So I could imagine this being the root cause
> > of Patrick's issues as well:
>
> Rereading my own link, my memory is a little less fuzzy: eXecute Never was being
> set, but was without effect due Manager mode being set in the DACR:
>
> > 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, we'll need U-Boot to use client domain permissions, so the XN
> > bit can function.
>
> > This issue has come up before and was fixed in de63ac278
> > ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> > It's equally applicable to all ARMv7-A platforms where caches are
> > enabled.
> > [1]: B3.7.2 - Execute-never restrictions on instruction fetching
>
> Hope this helps,
> Ahmad
>

It most definitely does, thanks a lot.

U-boot's mmu_setup() currently sets DACR to manager for all domains,
so this is broken for all non-LPAE configurations running on v7 CPUs
(except OMAP and perhaps others that fixed it individually). This
affects all device mappings: not just secure DRAM for OP-TEE, but any
MMIO register for any peripheral that is mapped into the CPU's address
space.

Patrick, could you please check whether this fixes the issue as well?

--- a/arch/arm/lib/cache-cp15.c
+++ b/arch/arm/lib/cache-cp15.c
@@ -202,9 +202,9 @@ static inline void mmu_setup(void)
        asm volatile("mcr p15, 0, %0, c2, c0, 0"
                     : : "r" (gd->arch.tlb_addr) : "memory");
 #endif
-       /* Set the access control to all-supervisor */
+       /* Set the access control to client (0b01) for each of the 16 domains */
        asm volatile("mcr p15, 0, %0, c3, c0, 0"
-                    : : "r" (~0));
+                    : : "r" (0x55555555));

        arm_init_domains();

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Etienne Carriere]

Hello all,

On Wed, 7 Oct 2020 at 15:16, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > My findings[1] back then were that U-Boot did set the eXecute Never bit only on
> > > OMAP, but not for other platforms.  So I could imagine this being the root cause
> > > of Patrick's issues as well:
> >
> > Rereading my own link, my memory is a little less fuzzy: eXecute Never was being
> > set, but was without effect due Manager mode being set in the DACR:
> >
> > > 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, we'll need U-Boot to use client domain permissions, so the XN
> > > bit can function.
> >
> > > This issue has come up before and was fixed in de63ac278
> > > ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> > > It's equally applicable to all ARMv7-A platforms where caches are
> > > enabled.
> > > [1]: B3.7.2 - Execute-never restrictions on instruction fetching
> >
> > Hope this helps,
> > Ahmad
> >
>
> It most definitely does, thanks a lot.
>
> U-boot's mmu_setup() currently sets DACR to manager for all domains,
> so this is broken for all non-LPAE configurations running on v7 CPUs
> (except OMAP and perhaps others that fixed it individually). This
> affects all device mappings: not just secure DRAM for OP-TEE, but any
> MMIO register for any peripheral that is mapped into the CPU's address
> space.

Despite the change proposed below seems to fix permissions bypass,
I think that not mapping the memory address ranges that are explicitly
expected to be not mapped (as in Patrick proposal) seems a consistent approach.

regards,
etienne

>
> Patrick, could you please check whether this fixes the issue as well?
>
> --- a/arch/arm/lib/cache-cp15.c
> +++ b/arch/arm/lib/cache-cp15.c
> @@ -202,9 +202,9 @@ static inline void mmu_setup(void)
>         asm volatile("mcr p15, 0, %0, c2, c0, 0"
>                      : : "r" (gd->arch.tlb_addr) : "memory");
>  #endif
> -       /* Set the access control to all-supervisor */
> +       /* Set the access control to client (0b01) for each of the 16 domains */
>         asm volatile("mcr p15, 0, %0, c3, c0, 0"
> -                    : : "r" (~0));
> +                    : : "r" (0x55555555));
>
>         arm_init_domains();

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Wed, 7 Oct 2020 at 16:55, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hello all,
>
> On Wed, 7 Oct 2020 at 15:16, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> > >
> > > Hello,
> > >
> > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > > My findings[1] back then were that U-Boot did set the eXecute Never bit only on
> > > > OMAP, but not for other platforms.  So I could imagine this being the root cause
> > > > of Patrick's issues as well:
> > >
> > > Rereading my own link, my memory is a little less fuzzy: eXecute Never was being
> > > set, but was without effect due Manager mode being set in the DACR:
> > >
> > > > 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, we'll need U-Boot to use client domain permissions, so the XN
> > > > bit can function.
> > >
> > > > This issue has come up before and was fixed in de63ac278
> > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> > > > It's equally applicable to all ARMv7-A platforms where caches are
> > > > enabled.
> > > > [1]: B3.7.2 - Execute-never restrictions on instruction fetching
> > >
> > > Hope this helps,
> > > Ahmad
> > >
> >
> > It most definitely does, thanks a lot.
> >
> > U-boot's mmu_setup() currently sets DACR to manager for all domains,
> > so this is broken for all non-LPAE configurations running on v7 CPUs
> > (except OMAP and perhaps others that fixed it individually). This
> > affects all device mappings: not just secure DRAM for OP-TEE, but any
> > MMIO register for any peripheral that is mapped into the CPU's address
> > space.
>
> Despite the change proposed below seems to fix permissions bypass,
> I think that not mapping the memory address ranges that are explicitly
> expected to be not mapped (as in Patrick proposal) seems a consistent approach.
>

Agreed.

But the issue Patrick is addressing uncovered a real bug that affects
many platforms, so let's please take advantage of this, and get it
fixed for everyone.

Then, we can decide whether unmapping the secure DRAM is worth it or
not, on its own merit, and not based on the justification that it
fixes a bug that should be fixed in a different way in the first
place.

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Etienne Carriere]

On Wed, 7 Oct 2020 at 17:08, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 7 Oct 2020 at 16:55, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hello all,
> >
> > On Wed, 7 Oct 2020 at 15:16, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> > > >
> > > > Hello,
> > > >
> > > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > > > My findings[1] back then were that U-Boot did set the eXecute Never bit only on
> > > > > OMAP, but not for other platforms.  So I could imagine this being the root cause
> > > > > of Patrick's issues as well:
> > > >
> > > > Rereading my own link, my memory is a little less fuzzy: eXecute Never was being
> > > > set, but was without effect due Manager mode being set in the DACR:
> > > >
> > > > > 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, we'll need U-Boot to use client domain permissions, so the XN
> > > > > bit can function.
> > > >
> > > > > This issue has come up before and was fixed in de63ac278
> > > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> > > > > It's equally applicable to all ARMv7-A platforms where caches are
> > > > > enabled.
> > > > > [1]: B3.7.2 - Execute-never restrictions on instruction fetching
> > > >
> > > > Hope this helps,
> > > > Ahmad
> > > >
> > >
> > > It most definitely does, thanks a lot.
> > >
> > > U-boot's mmu_setup() currently sets DACR to manager for all domains,
> > > so this is broken for all non-LPAE configurations running on v7 CPUs
> > > (except OMAP and perhaps others that fixed it individually). This
> > > affects all device mappings: not just secure DRAM for OP-TEE, but any
> > > MMIO register for any peripheral that is mapped into the CPU's address
> > > space.
> >
> > Despite the change proposed below seems to fix permissions bypass,
> > I think that not mapping the memory address ranges that are explicitly
> > expected to be not mapped (as in Patrick proposal) seems a consistent approach.
> >
>
> Agreed.
>
> But the issue Patrick is addressing uncovered a real bug that affects
> many platforms, so let's please take advantage of this, and get it
> fixed for everyone.
>
> Then, we can decide whether unmapping the secure DRAM is worth it or
> not, on its own merit, and not based on the justification that it
> fixes a bug that should be fixed in a different way in the first
> place.

Fair.

Etienne

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Patrick DELAUNAY]

Hi Ard,

> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: mercredi 7 octobre 2020 15:16
> 
> On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > My findings[1] back then were that U-Boot did set the eXecute Never
> > > bit only on OMAP, but not for other platforms.  So I could imagine
> > > this being the root cause of Patrick's issues as well:
> >
> > Rereading my own link, my memory is a little less fuzzy: eXecute Never
> > was being set, but was without effect due Manager mode being set in the
> DACR:
> >
> > > 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, we'll need U-Boot to use client domain
> > > permissions, so the XN bit can function.
> >
> > > This issue has come up before and was fixed in de63ac278
> > > ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> > > It's equally applicable to all ARMv7-A platforms where caches are
> > > enabled.
> > > [1]: B3.7.2 - Execute-never restrictions on instruction fetching
> >
> > Hope this helps,
> > Ahmad
> >
> 
> It most definitely does, thanks a lot.
> 
> U-boot's mmu_setup() currently sets DACR to manager for all domains, so this is
> broken for all non-LPAE configurations running on v7 CPUs (except OMAP and
> perhaps others that fixed it individually). This affects all device mappings: not just
> secure DRAM for OP-TEE, but any MMIO register for any peripheral that is
> mapped into the CPU's address space.
> 
> Patrick, could you please check whether this fixes the issue as well?
> 
> --- a/arch/arm/lib/cache-cp15.c
> +++ b/arch/arm/lib/cache-cp15.c
> @@ -202,9 +202,9 @@ static inline void mmu_setup(void)
>         asm volatile("mcr p15, 0, %0, c2, c0, 0"
>                      : : "r" (gd->arch.tlb_addr) : "memory");  #endif
> -       /* Set the access control to all-supervisor */
> +       /* Set the access control to client (0b01) for each of the 16
> + domains */
>         asm volatile("mcr p15, 0, %0, c3, c0, 0"
> -                    : : "r" (~0));
> +                    : : "r" (0x55555555));
> 
>         arm_init_domains();

The test will take some time to be sure that solve my remaining issue because  issue is not always reproductible.

At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line :

	The XN attribute is not checked for domains marked as Manager. Read-sensitive memory must
	not be included in domains marked as Manager, because the XN bit does not prevent prefetches
	in these cases.

So, I need  to test your patch +  DCACHE_OFF instead of INVALID 
(to map with XN the OP-TEE region) in my patchset.

FYI: I found the same DACR configuration is done in:
	arch/arm/cpu/armv7/ls102xa/cpu.c:199

[1] https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-control/The-Execute-Never--XN--attribute-and-instruction-prefetching?lang=en

Patrick

For information:

At the beginning I wasn't sure that the current DACR configuration is an issue because in found
in pseudo code of  DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf

B3.13.3 Address translation
	if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite) then
		CheckPermission(tlbrecord.perms, mva, tlbrecord.sectionnotpage, iswrite, ispriv);

B3.13.4 Domain checking
	boolean CheckDomain(bits(4) domain, bits(32) mva, boolean sectionnotpage, boolean iswrite)
		bitpos = 2*UInt(domain);
		case DACR<bitpos+1:bitpos> of
			when ?00? DataAbort(mva, domain, sectionnotpage, iswrite, DAbort_Domain);
			when ?01? permissioncheck = TRUE;
			when ?10? UNPREDICTABLE;
			when ?11? permissioncheck = FALSE;
		return permissioncheck;

B2.4.8 Access permission checking
	// CheckPermission()
	// =================
	CheckPermission(Permissions perms, bits(32) mva,
		boolean sectionnotpage, bits(4) domain, boolean iswrite, boolean ispriv)

		if SCTLR.AFE == ?0? then
			perms.ap<0> = ?1?;
			case perms.ap of
				when ?000? abort = TRUE;
				when ?001? abort = !ispriv;
				when ?010? abort = !ispriv && iswrite;
				when ?011? abort = FALSE;
				when ?100? UNPREDICTABLE;
				when ?101? abort = !ispriv || iswrite;
				when ?110? abort = iswrite;
				when ?111?
			if MemorySystemArchitecture() == MemArch_VMSA then
				abort = iswrite
			else
				UNPREDICTABLE;
			if abort then
				DataAbort(mva, domain, sectionnotpage, iswrite, DAbort_Permission);
			return;

=> it seens only the read/write permission is checked here (perms.ap)
=> perms.xn is not used here

	access_control = DRACR[r];
	perms.ap = access_control<10:8>;
	perms.xn = access_control<12>;

with AP[2:0], bits [10:8]
	Access Permissions field. Indicates the read and write access permissions for unprivileged
	and privileged accesses to the memory region.

But now it is clear with [1]

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Tom Rini]

On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote:
> Hi Ard,
> 
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: mercredi 7 octobre 2020 15:16
> > 
> > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> > >
> > > Hello,
> > >
> > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > > My findings[1] back then were that U-Boot did set the eXecute Never
> > > > bit only on OMAP, but not for other platforms.  So I could imagine
> > > > this being the root cause of Patrick's issues as well:
> > >
> > > Rereading my own link, my memory is a little less fuzzy: eXecute Never
> > > was being set, but was without effect due Manager mode being set in the
> > DACR:
> > >
> > > > 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, we'll need U-Boot to use client domain
> > > > permissions, so the XN bit can function.
> > >
> > > > This issue has come up before and was fixed in de63ac278
> > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> > > > It's equally applicable to all ARMv7-A platforms where caches are
> > > > enabled.
> > > > [1]: B3.7.2 - Execute-never restrictions on instruction fetching
> > >
> > > Hope this helps,
> > > Ahmad
> > >
> > 
> > It most definitely does, thanks a lot.
> > 
> > U-boot's mmu_setup() currently sets DACR to manager for all domains, so this is
> > broken for all non-LPAE configurations running on v7 CPUs (except OMAP and
> > perhaps others that fixed it individually). This affects all device mappings: not just
> > secure DRAM for OP-TEE, but any MMIO register for any peripheral that is
> > mapped into the CPU's address space.
> > 
> > Patrick, could you please check whether this fixes the issue as well?
> > 
> > --- a/arch/arm/lib/cache-cp15.c
> > +++ b/arch/arm/lib/cache-cp15.c
> > @@ -202,9 +202,9 @@ static inline void mmu_setup(void)
> >         asm volatile("mcr p15, 0, %0, c2, c0, 0"
> >                      : : "r" (gd->arch.tlb_addr) : "memory");  #endif
> > -       /* Set the access control to all-supervisor */
> > +       /* Set the access control to client (0b01) for each of the 16
> > + domains */
> >         asm volatile("mcr p15, 0, %0, c3, c0, 0"
> > -                    : : "r" (~0));
> > +                    : : "r" (0x55555555));
> > 
> >         arm_init_domains();
> 
> The test will take some time to be sure that solve my remaining issue because  issue is not always reproductible.
> 
> At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line :
> 
> 	The XN attribute is not checked for domains marked as Manager. Read-sensitive memory must
> 	not be included in domains marked as Manager, because the XN bit does not prevent prefetches
> 	in these cases.
> 
> So, I need  to test your patch +  DCACHE_OFF instead of INVALID 
> (to map with XN the OP-TEE region) in my patchset.
> 
> FYI: I found the same DACR configuration is done in:
> 	arch/arm/cpu/armv7/ls102xa/cpu.c:199
> 
> [1] https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-control/The-Execute-Never--XN--attribute-and-instruction-prefetching?lang=en
> 
> Patrick
> 
> For information:
> 
> At the beginning I wasn't sure that the current DACR configuration is an issue because in found
> in pseudo code of  DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf
> 
> B3.13.3 Address translation
> 	if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite) then
> 		CheckPermission(tlbrecord.perms, mva, tlbrecord.sectionnotpage, iswrite, ispriv);
> 
> B3.13.4 Domain checking
> 	boolean CheckDomain(bits(4) domain, bits(32) mva, boolean sectionnotpage, boolean iswrite)
> 		bitpos = 2*UInt(domain);
> 		case DACR<bitpos+1:bitpos> of
> 			when ?00? DataAbort(mva, domain, sectionnotpage, iswrite, DAbort_Domain);
> 			when ?01? permissioncheck = TRUE;
> 			when ?10? UNPREDICTABLE;
> 			when ?11? permissioncheck = FALSE;
> 		return permissioncheck;
> 
> B2.4.8 Access permission checking
> 	// CheckPermission()
> 	// =================
> 	CheckPermission(Permissions perms, bits(32) mva,
> 		boolean sectionnotpage, bits(4) domain, boolean iswrite, boolean ispriv)
> 
> 		if SCTLR.AFE == ?0? then
> 			perms.ap<0> = ?1?;
> 			case perms.ap of
> 				when ?000? abort = TRUE;
> 				when ?001? abort = !ispriv;
> 				when ?010? abort = !ispriv && iswrite;
> 				when ?011? abort = FALSE;
> 				when ?100? UNPREDICTABLE;
> 				when ?101? abort = !ispriv || iswrite;
> 				when ?110? abort = iswrite;
> 				when ?111?
> 			if MemorySystemArchitecture() == MemArch_VMSA then
> 				abort = iswrite
> 			else
> 				UNPREDICTABLE;
> 			if abort then
> 				DataAbort(mva, domain, sectionnotpage, iswrite, DAbort_Permission);
> 			return;
> 
> => it seens only the read/write permission is checked here (perms.ap)
> => perms.xn is not used here
> 
> 	access_control = DRACR[r];
> 	perms.ap = access_control<10:8>;
> 	perms.xn = access_control<12>;
> 
> with AP[2:0], bits [10:8]
> 	Access Permissions field. Indicates the read and write access permissions for unprivileged
> 	and privileged accesses to the memory region.
> 
> But now it is clear with [1]

So, where did everything end up here?  I specifically didn't grab this
series as it sounded like there was concern the problem should be solved
via another patch.  Or would that be an in-addition-to?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201027/5a147f22/attachment.sig>

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Tue, 27 Oct 2020 at 18:25, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote:
> > Hi Ard,
> >
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: mercredi 7 octobre 2020 15:16
> > >
> > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> > > >
> > > > Hello,
> > > >
> > > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > > > My findings[1] back then were that U-Boot did set the eXecute Never
> > > > > bit only on OMAP, but not for other platforms.  So I could imagine
> > > > > this being the root cause of Patrick's issues as well:
> > > >
> > > > Rereading my own link, my memory is a little less fuzzy: eXecute Never
> > > > was being set, but was without effect due Manager mode being set in the
> > > DACR:
> > > >
> > > > > 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, we'll need U-Boot to use client domain
> > > > > permissions, so the XN bit can function.
> > > >
> > > > > This issue has come up before and was fixed in de63ac278
> > > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> > > > > It's equally applicable to all ARMv7-A platforms where caches are
> > > > > enabled.
> > > > > [1]: B3.7.2 - Execute-never restrictions on instruction fetching
> > > >
> > > > Hope this helps,
> > > > Ahmad
> > > >
> > >
> > > It most definitely does, thanks a lot.
> > >
> > > U-boot's mmu_setup() currently sets DACR to manager for all domains, so this is
> > > broken for all non-LPAE configurations running on v7 CPUs (except OMAP and
> > > perhaps others that fixed it individually). This affects all device mappings: not just
> > > secure DRAM for OP-TEE, but any MMIO register for any peripheral that is
> > > mapped into the CPU's address space.
> > >
> > > Patrick, could you please check whether this fixes the issue as well?
> > >
> > > --- a/arch/arm/lib/cache-cp15.c
> > > +++ b/arch/arm/lib/cache-cp15.c
> > > @@ -202,9 +202,9 @@ static inline void mmu_setup(void)
> > >         asm volatile("mcr p15, 0, %0, c2, c0, 0"
> > >                      : : "r" (gd->arch.tlb_addr) : "memory");  #endif
> > > -       /* Set the access control to all-supervisor */
> > > +       /* Set the access control to client (0b01) for each of the 16
> > > + domains */
> > >         asm volatile("mcr p15, 0, %0, c3, c0, 0"
> > > -                    : : "r" (~0));
> > > +                    : : "r" (0x55555555));
> > >
> > >         arm_init_domains();
> >
> > The test will take some time to be sure that solve my remaining issue because  issue is not always reproductible.
> >
> > At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line :
> >
> >       The XN attribute is not checked for domains marked as Manager. Read-sensitive memory must
> >       not be included in domains marked as Manager, because the XN bit does not prevent prefetches
> >       in these cases.
> >
> > So, I need  to test your patch +  DCACHE_OFF instead of INVALID
> > (to map with XN the OP-TEE region) in my patchset.
> >
> > FYI: I found the same DACR configuration is done in:
> >       arch/arm/cpu/armv7/ls102xa/cpu.c:199
> >
> > [1] https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-control/The-Execute-Never--XN--attribute-and-instruction-prefetching?lang=en
> >
> > Patrick
> >
> > For information:
> >
> > At the beginning I wasn't sure that the current DACR configuration is an issue because in found
> > in pseudo code of  DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf
> >
> > B3.13.3 Address translation
> >       if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite) then
> >               CheckPermission(tlbrecord.perms, mva, tlbrecord.sectionnotpage, iswrite, ispriv);
> >
> > B3.13.4 Domain checking
> >       boolean CheckDomain(bits(4) domain, bits(32) mva, boolean sectionnotpage, boolean iswrite)
> >               bitpos = 2*UInt(domain);
> >               case DACR<bitpos+1:bitpos> of
> >                       when ?00? DataAbort(mva, domain, sectionnotpage, iswrite, DAbort_Domain);
> >                       when ?01? permissioncheck = TRUE;
> >                       when ?10? UNPREDICTABLE;
> >                       when ?11? permissioncheck = FALSE;
> >               return permissioncheck;
> >
> > B2.4.8 Access permission checking
> >       // CheckPermission()
> >       // =================
> >       CheckPermission(Permissions perms, bits(32) mva,
> >               boolean sectionnotpage, bits(4) domain, boolean iswrite, boolean ispriv)
> >
> >               if SCTLR.AFE == ?0? then
> >                       perms.ap<0> = ?1?;
> >                       case perms.ap of
> >                               when ?000? abort = TRUE;
> >                               when ?001? abort = !ispriv;
> >                               when ?010? abort = !ispriv && iswrite;
> >                               when ?011? abort = FALSE;
> >                               when ?100? UNPREDICTABLE;
> >                               when ?101? abort = !ispriv || iswrite;
> >                               when ?110? abort = iswrite;
> >                               when ?111?
> >                       if MemorySystemArchitecture() == MemArch_VMSA then
> >                               abort = iswrite
> >                       else
> >                               UNPREDICTABLE;
> >                       if abort then
> >                               DataAbort(mva, domain, sectionnotpage, iswrite, DAbort_Permission);
> >                       return;
> >
> > => it seens only the read/write permission is checked here (perms.ap)
> > => perms.xn is not used here
> >
> >       access_control = DRACR[r];
> >       perms.ap = access_control<10:8>;
> >       perms.xn = access_control<12>;
> >
> > with AP[2:0], bits [10:8]
> >       Access Permissions field. Indicates the read and write access permissions for unprivileged
> >       and privileged accesses to the memory region.
> >
> > But now it is clear with [1]
>
> So, where did everything end up here?  I specifically didn't grab this
> series as it sounded like there was concern the problem should be solved
> via another patch.  Or would that be an in-addition-to?  Thanks!
>

There are three different problems:
- ARMv7 non-LPAE uses the wrong domain settings
- no-map non-secure memory should not be mapped by u-boot
- secure world-only memory should not be described as memory in the device tree

So I think this series definitely needs a respin at the very least.

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Patrick DELAUNAY]

Hi,

> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: mardi 27 octobre 2020 22:05
> 
> On Tue, 27 Oct 2020 at 18:25, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote:
> > > Hi Ard,
> > >
> > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > Sent: mercredi 7 octobre 2020 15:16
> > > >
> > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de>
> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > > > > My findings[1] back then were that U-Boot did set the eXecute
> > > > > > Never bit only on OMAP, but not for other platforms.  So I
> > > > > > could imagine this being the root cause of Patrick's issues as well:
> > > > >
> > > > > Rereading my own link, my memory is a little less fuzzy: eXecute
> > > > > Never was being set, but was without effect due Manager mode
> > > > > being set in the
> > > > DACR:
> > > > >
> > > > > > 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, we'll need U-Boot to use client domain
> > > > > > permissions, so the XN bit can function.
> > > > >
> > > > > > This issue has come up before and was fixed in de63ac278
> > > > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2
> only.
> > > > > > It's equally applicable to all ARMv7-A platforms where caches
> > > > > > are enabled.
> > > > > > [1]: B3.7.2 - Execute-never restrictions on instruction
> > > > > > fetching
> > > > >
> > > > > Hope this helps,
> > > > > Ahmad
> > > > >
> > > >
> > > > It most definitely does, thanks a lot.
> > > >
> > > > U-boot's mmu_setup() currently sets DACR to manager for all
> > > > domains, so this is broken for all non-LPAE configurations running
> > > > on v7 CPUs (except OMAP and perhaps others that fixed it
> > > > individually). This affects all device mappings: not just secure
> > > > DRAM for OP-TEE, but any MMIO register for any peripheral that is mapped
> into the CPU's address space.
> > > >
> > > > Patrick, could you please check whether this fixes the issue as well?
> > > >
> > > > --- a/arch/arm/lib/cache-cp15.c
> > > > +++ b/arch/arm/lib/cache-cp15.c
> > > > @@ -202,9 +202,9 @@ static inline void mmu_setup(void)
> > > >         asm volatile("mcr p15, 0, %0, c2, c0, 0"
> > > >                      : : "r" (gd->arch.tlb_addr) : "memory");  #endif
> > > > -       /* Set the access control to all-supervisor */
> > > > +       /* Set the access control to client (0b01) for each of the
> > > > + 16 domains */
> > > >         asm volatile("mcr p15, 0, %0, c3, c0, 0"
> > > > -                    : : "r" (~0));
> > > > +                    : : "r" (0x55555555));
> > > >
> > > >         arm_init_domains();
> > >
> > > The test will take some time to be sure that solve my remaining issue because
> issue is not always reproductible.
> > >
> > > At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line :
> > >
> > >       The XN attribute is not checked for domains marked as Manager. Read-
> sensitive memory must
> > >       not be included in domains marked as Manager, because the XN bit does
> not prevent prefetches
> > >       in these cases.
> > >
> > > So, I need  to test your patch +  DCACHE_OFF instead of INVALID (to
> > > map with XN the OP-TEE region) in my patchset.
> > >
> > > FYI: I found the same DACR configuration is done in:
> > >       arch/arm/cpu/armv7/ls102xa/cpu.c:199
> > >
> > > [1]
> > > https://developer.arm.com/documentation/ddi0406/b/System-Level-Archi
> > > tecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-cont
> > > rol/The-Execute-Never--XN--attribute-and-instruction-prefetching?lan
> > > g=en
> > >
> > > Patrick
> > >
> > > For information:
> > >
> > > At the beginning I wasn't sure that the current DACR configuration
> > > is an issue because in found in pseudo code of
> > > DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf
> > >
> > > B3.13.3 Address translation
> > >       if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite)
> then
> > >               CheckPermission(tlbrecord.perms, mva,
> > > tlbrecord.sectionnotpage, iswrite, ispriv);
> > >
> > > B3.13.4 Domain checking
> > >       boolean CheckDomain(bits(4) domain, bits(32) mva, boolean
> sectionnotpage, boolean iswrite)
> > >               bitpos = 2*UInt(domain);
> > >               case DACR<bitpos+1:bitpos> of
> > >                       when ?00? DataAbort(mva, domain, sectionnotpage, iswrite,
> DAbort_Domain);
> > >                       when ?01? permissioncheck = TRUE;
> > >                       when ?10? UNPREDICTABLE;
> > >                       when ?11? permissioncheck = FALSE;
> > >               return permissioncheck;
> > >
> > > B2.4.8 Access permission checking
> > >       // CheckPermission()
> > >       // =================
> > >       CheckPermission(Permissions perms, bits(32) mva,
> > >               boolean sectionnotpage, bits(4) domain, boolean
> > > iswrite, boolean ispriv)
> > >
> > >               if SCTLR.AFE == ?0? then
> > >                       perms.ap<0> = ?1?;
> > >                       case perms.ap of
> > >                               when ?000? abort = TRUE;
> > >                               when ?001? abort = !ispriv;
> > >                               when ?010? abort = !ispriv && iswrite;
> > >                               when ?011? abort = FALSE;
> > >                               when ?100? UNPREDICTABLE;
> > >                               when ?101? abort = !ispriv || iswrite;
> > >                               when ?110? abort = iswrite;
> > >                               when ?111?
> > >                       if MemorySystemArchitecture() == MemArch_VMSA then
> > >                               abort = iswrite
> > >                       else
> > >                               UNPREDICTABLE;
> > >                       if abort then
> > >                               DataAbort(mva, domain, sectionnotpage, iswrite,
> DAbort_Permission);
> > >                       return;
> > >
> > > => it seens only the read/write permission is checked here
> > > (perms.ap) => perms.xn is not used here
> > >
> > >       access_control = DRACR[r];
> > >       perms.ap = access_control<10:8>;
> > >       perms.xn = access_control<12>;
> > >
> > > with AP[2:0], bits [10:8]
> > >       Access Permissions field. Indicates the read and write access permissions
> for unprivileged
> > >       and privileged accesses to the memory region.
> > >
> > > But now it is clear with [1]
> >
> > So, where did everything end up here?  I specifically didn't grab this
> > series as it sounded like there was concern the problem should be
> > solved via another patch.  Or would that be an in-addition-to?  Thanks!
> >
> 
> There are three different problems:
> - ARMv7 non-LPAE uses the wrong domain settings
> - no-map non-secure memory should not be mapped by u-boot
> - secure world-only memory should not be described as memory in the device tree
> 
> So I think this series definitely needs a respin at the very least.

I gree: 3 differents issues in the thread

- ARMv7 non-LPAE uses the wrong domain settings

=> bad DRACR configuration as raised by Ard & patch proposed by Ard in thread (but not pushed in mailing list)
=> I need to test it to check if it is correct my issue : today I can't reproduce the issue, still in my TODO list

- no-map non-secure memory should not be mapped by u-boot

=> this serie, not ready for v2021.01, I think... I will push a V2 after my tests

- secure world-only memory should not be described as memory in the device tree

=> I let Etienne Carriere manage it for OP-TEE point of view: today the secure memory is added by OP-TEE
     in U-Boot device tree and U-Boot copy this node to Kernel Device tree 

I have no a clear opinion about it

Regards

PAtrick

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Etienne Carriere]

Dear all,

CC some fellow OP-TEE guys for this secure memory description topic.


On Wed, 28 Oct 2020 at 11:33, Patrick DELAUNAY <patrick.delaunay@st.com> wrote:
>
> Hi,
>
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: mardi 27 octobre 2020 22:05
> >
> > On Tue, 27 Oct 2020 at 18:25, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote:
> > > > Hi Ard,
> > > >
> > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > Sent: mercredi 7 octobre 2020 15:16
> > > > >
> > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de>
> > wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > > > > > My findings[1] back then were that U-Boot did set the eXecute
> > > > > > > Never bit only on OMAP, but not for other platforms.  So I
> > > > > > > could imagine this being the root cause of Patrick's issues as well:
> > > > > >
> > > > > > Rereading my own link, my memory is a little less fuzzy: eXecute
> > > > > > Never was being set, but was without effect due Manager mode
> > > > > > being set in the
> > > > > DACR:
> > > > > >
> > > > > > > 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, we'll need U-Boot to use client domain
> > > > > > > permissions, so the XN bit can function.
> > > > > >
> > > > > > > This issue has come up before and was fixed in de63ac278
> > > > > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2
> > only.
> > > > > > > It's equally applicable to all ARMv7-A platforms where caches
> > > > > > > are enabled.
> > > > > > > [1]: B3.7.2 - Execute-never restrictions on instruction
> > > > > > > fetching
> > > > > >
> > > > > > Hope this helps,
> > > > > > Ahmad
> > > > > >
> > > > >
> > > > > It most definitely does, thanks a lot.
> > > > >
> > > > > U-boot's mmu_setup() currently sets DACR to manager for all
> > > > > domains, so this is broken for all non-LPAE configurations running
> > > > > on v7 CPUs (except OMAP and perhaps others that fixed it
> > > > > individually). This affects all device mappings: not just secure
> > > > > DRAM for OP-TEE, but any MMIO register for any peripheral that is mapped
> > into the CPU's address space.
> > > > >
> > > > > Patrick, could you please check whether this fixes the issue as well?
> > > > >
> > > > > --- a/arch/arm/lib/cache-cp15.c
> > > > > +++ b/arch/arm/lib/cache-cp15.c
> > > > > @@ -202,9 +202,9 @@ static inline void mmu_setup(void)
> > > > >         asm volatile("mcr p15, 0, %0, c2, c0, 0"
> > > > >                      : : "r" (gd->arch.tlb_addr) : "memory");  #endif
> > > > > -       /* Set the access control to all-supervisor */
> > > > > +       /* Set the access control to client (0b01) for each of the
> > > > > + 16 domains */
> > > > >         asm volatile("mcr p15, 0, %0, c3, c0, 0"
> > > > > -                    : : "r" (~0));
> > > > > +                    : : "r" (0x55555555));
> > > > >
> > > > >         arm_init_domains();
> > > >
> > > > The test will take some time to be sure that solve my remaining issue because
> > issue is not always reproductible.
> > > >
> > > > At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line :
> > > >
> > > >       The XN attribute is not checked for domains marked as Manager. Read-
> > sensitive memory must
> > > >       not be included in domains marked as Manager, because the XN bit does
> > not prevent prefetches
> > > >       in these cases.
> > > >
> > > > So, I need  to test your patch +  DCACHE_OFF instead of INVALID (to
> > > > map with XN the OP-TEE region) in my patchset.
> > > >
> > > > FYI: I found the same DACR configuration is done in:
> > > >       arch/arm/cpu/armv7/ls102xa/cpu.c:199
> > > >
> > > > [1]
> > > > https://developer.arm.com/documentation/ddi0406/b/System-Level-Archi
> > > > tecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-cont
> > > > rol/The-Execute-Never--XN--attribute-and-instruction-prefetching?lan
> > > > g=en
> > > >
> > > > Patrick
> > > >
> > > > For information:
> > > >
> > > > At the beginning I wasn't sure that the current DACR configuration
> > > > is an issue because in found in pseudo code of
> > > > DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf
> > > >
> > > > B3.13.3 Address translation
> > > >       if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite)
> > then
> > > >               CheckPermission(tlbrecord.perms, mva,
> > > > tlbrecord.sectionnotpage, iswrite, ispriv);
> > > >
> > > > B3.13.4 Domain checking
> > > >       boolean CheckDomain(bits(4) domain, bits(32) mva, boolean
> > sectionnotpage, boolean iswrite)
> > > >               bitpos = 2*UInt(domain);
> > > >               case DACR<bitpos+1:bitpos> of
> > > >                       when ?00? DataAbort(mva, domain, sectionnotpage, iswrite,
> > DAbort_Domain);
> > > >                       when ?01? permissioncheck = TRUE;
> > > >                       when ?10? UNPREDICTABLE;
> > > >                       when ?11? permissioncheck = FALSE;
> > > >               return permissioncheck;
> > > >
> > > > B2.4.8 Access permission checking
> > > >       // CheckPermission()
> > > >       // =================
> > > >       CheckPermission(Permissions perms, bits(32) mva,
> > > >               boolean sectionnotpage, bits(4) domain, boolean
> > > > iswrite, boolean ispriv)
> > > >
> > > >               if SCTLR.AFE == ?0? then
> > > >                       perms.ap<0> = ?1?;
> > > >                       case perms.ap of
> > > >                               when ?000? abort = TRUE;
> > > >                               when ?001? abort = !ispriv;
> > > >                               when ?010? abort = !ispriv && iswrite;
> > > >                               when ?011? abort = FALSE;
> > > >                               when ?100? UNPREDICTABLE;
> > > >                               when ?101? abort = !ispriv || iswrite;
> > > >                               when ?110? abort = iswrite;
> > > >                               when ?111?
> > > >                       if MemorySystemArchitecture() == MemArch_VMSA then
> > > >                               abort = iswrite
> > > >                       else
> > > >                               UNPREDICTABLE;
> > > >                       if abort then
> > > >                               DataAbort(mva, domain, sectionnotpage, iswrite,
> > DAbort_Permission);
> > > >                       return;
> > > >
> > > > => it seens only the read/write permission is checked here
> > > > (perms.ap) => perms.xn is not used here
> > > >
> > > >       access_control = DRACR[r];
> > > >       perms.ap = access_control<10:8>;
> > > >       perms.xn = access_control<12>;
> > > >
> > > > with AP[2:0], bits [10:8]
> > > >       Access Permissions field. Indicates the read and write access permissions
> > for unprivileged
> > > >       and privileged accesses to the memory region.
> > > >
> > > > But now it is clear with [1]
> > >
> > > So, where did everything end up here?  I specifically didn't grab this
> > > series as it sounded like there was concern the problem should be
> > > solved via another patch.  Or would that be an in-addition-to?  Thanks!
> > >
> >
> > There are three different problems:
> > - ARMv7 non-LPAE uses the wrong domain settings
> > - no-map non-secure memory should not be mapped by u-boot
> > - secure world-only memory should not be described as memory in the device tree
> >
> > So I think this series definitely needs a respin at the very least.
>
> I gree: 3 differents issues in the thread
>
> - ARMv7 non-LPAE uses the wrong domain settings
>
> => bad DRACR configuration as raised by Ard & patch proposed by Ard in thread (but not pushed in mailing list)
> => I need to test it to check if it is correct my issue : today I can't reproduce the issue, still in my TODO list
>
> - no-map non-secure memory should not be mapped by u-boot
>
> => this serie, not ready for v2021.01, I think... I will push a V2 after my tests
>
> - secure world-only memory should not be described as memory in the device tree
>
> => I let Etienne Carriere manage it for OP-TEE point of view: today the secure memory is added by OP-TEE
>      in U-Boot device tree and U-Boot copy this node to Kernel Device tree
>
> I have no a clear opinion about it

From the overall system description, it is far more convenient for
platforms to describe the
full physical memory through memory at ... nodes and exclude specific areas with
reserved-memory nodes. Among those specific areas, using no-map for
secure address
ranges seems applicable since the property is also intended for that
purpose (as the
reference to speculative accesses in the binding description).
From my perspective, the issue discussed here seems rather more related to how
U-Boot handles FDT memory description. From my understanding, fixing
the Arm domain
mapping description in U-Boot should address the issue, as for secure
areas are concerned.

Whereas should secure areas not be covered at all by FDT memory nodes,
I have no real strong
opinion.
- There are platforms statically describing memory and reserved-memory
nodes in DTS files.
I guess these could update DTS files exclude secure memory from memory
nodes, rather
than using reserved-memory nodes.
- There are platforms using runtime (actually boot time) update of the
FDT: secure world
(booting before the non-secure world) update memory description with knowledge
of the secure ranges. Adding reserved-memory node(s) is quite
straightforward. I guess
replacing memory@ nodes with new nodes describing non-secure memory
ranges is also
feasible.
- If no-map is to be used by U-Boot to not map related memory (needed
for the non-secure
reserved memory as discussed in this thread), why not reusing this
scheme also for secure
memory. Here we discussed statically assigned memory. If we consider a
platform where
secure world can dynamically re-assigned memory to secure/non-secure world, such
areas are not secure or non-secure memory, they are just memory....
reserved to secure
services eco-system.


In a previous post, Ard asked:
> So the next question is then, why describe it in the first place? Does
> OP-TEE rely on the DT description to figure out where the secure DDR
> is? Does the agent that programs the firewall get this information
> from DT?

For the first question, I think the answer is that we can have a
unique description
for physical memory shared by both worlds. So I would say convenience
as I stated above.

As for the other questions, yes, DT can definitely help the secure
world to configure
firewalls for the non-secure allowed accesses which both secure
and non-secure rely on. The secure world relies on it because non-secure memory
is seen by the secure world as legitimate areas where both worlds can share
buffers for communication.

Best regards,
Etienne

>
> Regards
>
> PAtrick

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Thu, 29 Oct 2020 at 11:40, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Dear all,
>
> CC some fellow OP-TEE guys for this secure memory description topic.
>
>
> On Wed, 28 Oct 2020 at 11:33, Patrick DELAUNAY <patrick.delaunay@st.com> wrote:
> >
> > Hi,
> >
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: mardi 27 octobre 2020 22:05
> > >
> > > On Tue, 27 Oct 2020 at 18:25, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote:
> > > > > Hi Ard,
> > > > >
> > > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > > Sent: mercredi 7 octobre 2020 15:16
> > > > > >
> > > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de>
> > > wrote:
> > > > > > >
> > > > > > > Hello,
> > > > > > >
> > > > > > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > > > > > > My findings[1] back then were that U-Boot did set the eXecute
> > > > > > > > Never bit only on OMAP, but not for other platforms.  So I
> > > > > > > > could imagine this being the root cause of Patrick's issues as well:
> > > > > > >
> > > > > > > Rereading my own link, my memory is a little less fuzzy: eXecute
> > > > > > > Never was being set, but was without effect due Manager mode
> > > > > > > being set in the
> > > > > > DACR:
> > > > > > >
> > > > > > > > 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, we'll need U-Boot to use client domain
> > > > > > > > permissions, so the XN bit can function.
> > > > > > >
> > > > > > > > This issue has come up before and was fixed in de63ac278
> > > > > > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2
> > > only.
> > > > > > > > It's equally applicable to all ARMv7-A platforms where caches
> > > > > > > > are enabled.
> > > > > > > > [1]: B3.7.2 - Execute-never restrictions on instruction
> > > > > > > > fetching
> > > > > > >
> > > > > > > Hope this helps,
> > > > > > > Ahmad
> > > > > > >
> > > > > >
> > > > > > It most definitely does, thanks a lot.
> > > > > >
> > > > > > U-boot's mmu_setup() currently sets DACR to manager for all
> > > > > > domains, so this is broken for all non-LPAE configurations running
> > > > > > on v7 CPUs (except OMAP and perhaps others that fixed it
> > > > > > individually). This affects all device mappings: not just secure
> > > > > > DRAM for OP-TEE, but any MMIO register for any peripheral that is mapped
> > > into the CPU's address space.
> > > > > >
> > > > > > Patrick, could you please check whether this fixes the issue as well?
> > > > > >
> > > > > > --- a/arch/arm/lib/cache-cp15.c
> > > > > > +++ b/arch/arm/lib/cache-cp15.c
> > > > > > @@ -202,9 +202,9 @@ static inline void mmu_setup(void)
> > > > > >         asm volatile("mcr p15, 0, %0, c2, c0, 0"
> > > > > >                      : : "r" (gd->arch.tlb_addr) : "memory");  #endif
> > > > > > -       /* Set the access control to all-supervisor */
> > > > > > +       /* Set the access control to client (0b01) for each of the
> > > > > > + 16 domains */
> > > > > >         asm volatile("mcr p15, 0, %0, c3, c0, 0"
> > > > > > -                    : : "r" (~0));
> > > > > > +                    : : "r" (0x55555555));
> > > > > >
> > > > > >         arm_init_domains();
> > > > >
> > > > > The test will take some time to be sure that solve my remaining issue because
> > > issue is not always reproductible.
> > > > >
> > > > > At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line :
> > > > >
> > > > >       The XN attribute is not checked for domains marked as Manager. Read-
> > > sensitive memory must
> > > > >       not be included in domains marked as Manager, because the XN bit does
> > > not prevent prefetches
> > > > >       in these cases.
> > > > >
> > > > > So, I need  to test your patch +  DCACHE_OFF instead of INVALID (to
> > > > > map with XN the OP-TEE region) in my patchset.
> > > > >
> > > > > FYI: I found the same DACR configuration is done in:
> > > > >       arch/arm/cpu/armv7/ls102xa/cpu.c:199
> > > > >
> > > > > [1]
> > > > > https://developer.arm.com/documentation/ddi0406/b/System-Level-Archi
> > > > > tecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-cont
> > > > > rol/The-Execute-Never--XN--attribute-and-instruction-prefetching?lan
> > > > > g=en
> > > > >
> > > > > Patrick
> > > > >
> > > > > For information:
> > > > >
> > > > > At the beginning I wasn't sure that the current DACR configuration
> > > > > is an issue because in found in pseudo code of
> > > > > DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf
> > > > >
> > > > > B3.13.3 Address translation
> > > > >       if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite)
> > > then
> > > > >               CheckPermission(tlbrecord.perms, mva,
> > > > > tlbrecord.sectionnotpage, iswrite, ispriv);
> > > > >
> > > > > B3.13.4 Domain checking
> > > > >       boolean CheckDomain(bits(4) domain, bits(32) mva, boolean
> > > sectionnotpage, boolean iswrite)
> > > > >               bitpos = 2*UInt(domain);
> > > > >               case DACR<bitpos+1:bitpos> of
> > > > >                       when ?00? DataAbort(mva, domain, sectionnotpage, iswrite,
> > > DAbort_Domain);
> > > > >                       when ?01? permissioncheck = TRUE;
> > > > >                       when ?10? UNPREDICTABLE;
> > > > >                       when ?11? permissioncheck = FALSE;
> > > > >               return permissioncheck;
> > > > >
> > > > > B2.4.8 Access permission checking
> > > > >       // CheckPermission()
> > > > >       // =================
> > > > >       CheckPermission(Permissions perms, bits(32) mva,
> > > > >               boolean sectionnotpage, bits(4) domain, boolean
> > > > > iswrite, boolean ispriv)
> > > > >
> > > > >               if SCTLR.AFE == ?0? then
> > > > >                       perms.ap<0> = ?1?;
> > > > >                       case perms.ap of
> > > > >                               when ?000? abort = TRUE;
> > > > >                               when ?001? abort = !ispriv;
> > > > >                               when ?010? abort = !ispriv && iswrite;
> > > > >                               when ?011? abort = FALSE;
> > > > >                               when ?100? UNPREDICTABLE;
> > > > >                               when ?101? abort = !ispriv || iswrite;
> > > > >                               when ?110? abort = iswrite;
> > > > >                               when ?111?
> > > > >                       if MemorySystemArchitecture() == MemArch_VMSA then
> > > > >                               abort = iswrite
> > > > >                       else
> > > > >                               UNPREDICTABLE;
> > > > >                       if abort then
> > > > >                               DataAbort(mva, domain, sectionnotpage, iswrite,
> > > DAbort_Permission);
> > > > >                       return;
> > > > >
> > > > > => it seens only the read/write permission is checked here
> > > > > (perms.ap) => perms.xn is not used here
> > > > >
> > > > >       access_control = DRACR[r];
> > > > >       perms.ap = access_control<10:8>;
> > > > >       perms.xn = access_control<12>;
> > > > >
> > > > > with AP[2:0], bits [10:8]
> > > > >       Access Permissions field. Indicates the read and write access permissions
> > > for unprivileged
> > > > >       and privileged accesses to the memory region.
> > > > >
> > > > > But now it is clear with [1]
> > > >
> > > > So, where did everything end up here?  I specifically didn't grab this
> > > > series as it sounded like there was concern the problem should be
> > > > solved via another patch.  Or would that be an in-addition-to?  Thanks!
> > > >
> > >
> > > There are three different problems:
> > > - ARMv7 non-LPAE uses the wrong domain settings
> > > - no-map non-secure memory should not be mapped by u-boot
> > > - secure world-only memory should not be described as memory in the device tree
> > >
> > > So I think this series definitely needs a respin at the very least.
> >
> > I gree: 3 differents issues in the thread
> >
> > - ARMv7 non-LPAE uses the wrong domain settings
> >
> > => bad DRACR configuration as raised by Ard & patch proposed by Ard in thread (but not pushed in mailing list)
> > => I need to test it to check if it is correct my issue : today I can't reproduce the issue, still in my TODO list
> >
> > - no-map non-secure memory should not be mapped by u-boot
> >
> > => this serie, not ready for v2021.01, I think... I will push a V2 after my tests
> >
> > - secure world-only memory should not be described as memory in the device tree
> >
> > => I let Etienne Carriere manage it for OP-TEE point of view: today the secure memory is added by OP-TEE
> >      in U-Boot device tree and U-Boot copy this node to Kernel Device tree
> >
> > I have no a clear opinion about it
>
> From the overall system description, it is far more convenient for
> platforms to describe the
> full physical memory through memory at ... nodes and exclude specific areas with
> reserved-memory nodes. Among those specific areas, using no-map for
> secure address
> ranges seems applicable since the property is also intended for that
> purpose (as the
> reference to speculative accesses in the binding description).

The point I made before was that secure and non-secure are two
disjoint address spaces. The fact that TZ firewalls exist where you
can move things from one side to the other does not imply that things
works like this in the general case.

E.g., you could have

secure DRAM at S 0x0
non-secure DRAM at NS 0x0

where the ranges are backed by *different* memory. Since the DT
description does not include the S/NS distinction, only the address
range, the only thing we can assume when looking at memory@ and
/reserved-memory is that everything it describes is NS.


> From my perspective, the issue discussed here seems rather more related to how
> U-Boot handles FDT memory description. From my understanding, fixing
> the Arm domain
> mapping description in U-Boot should address the issue, as for secure
> areas are concerned.
>
> Whereas should secure areas not be covered at all by FDT memory nodes,
> I have no real strong
> opinion.
> - There are platforms statically describing memory and reserved-memory
> nodes in DTS files.
> I guess these could update DTS files exclude secure memory from memory
> nodes, rather
> than using reserved-memory nodes.
> - There are platforms using runtime (actually boot time) update of the
> FDT: secure world
> (booting before the non-secure world) update memory description with knowledge
> of the secure ranges. Adding reserved-memory node(s) is quite
> straightforward. I guess
> replacing memory@ nodes with new nodes describing non-secure memory
> ranges is also
> feasible.
> - If no-map is to be used by U-Boot to not map related memory (needed
> for the non-secure
> reserved memory as discussed in this thread), why not reusing this
> scheme also for secure
> memory. Here we discussed statically assigned memory. If we consider a
> platform where
> secure world can dynamically re-assigned memory to secure/non-secure world, such
> areas are not secure or non-secure memory, they are just memory....
> reserved to secure
> services eco-system.
>
>
> In a previous post, Ard asked:
> > So the next question is then, why describe it in the first place? Does
> > OP-TEE rely on the DT description to figure out where the secure DDR
> > is? Does the agent that programs the firewall get this information
> > from DT?
>
> For the first question, I think the answer is that we can have a
> unique description
> for physical memory shared by both worlds. So I would say convenience
> as I stated above.
>
> As for the other questions, yes, DT can definitely help the secure
> world to configure
> firewalls for the non-secure allowed accesses which both secure
> and non-secure rely on. The secure world relies on it because non-secure memory
> is seen by the secure world as legitimate areas where both worlds can share
> buffers for communication.
>
> Best regards,
> Etienne
>
> >
> > Regards
> >
> > PAtrick

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Etienne Carriere]

On Thu, 29 Oct 2020 at 12:26, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 29 Oct 2020 at 11:40, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Dear all,
> >
> > CC some fellow OP-TEE guys for this secure memory description topic.
> >
> >
> > On Wed, 28 Oct 2020 at 11:33, Patrick DELAUNAY <patrick.delaunay@st.com> wrote:
> > >
> > > Hi,
> > >
> > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > Sent: mardi 27 octobre 2020 22:05
> > > >
> > > > On Tue, 27 Oct 2020 at 18:25, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote:
> > > > > > Hi Ard,
> > > > > >
> > > > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > Sent: mercredi 7 octobre 2020 15:16
> > > > > > >
> > > > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de>
> > > > wrote:
> > > > > > > >
> > > > > > > > Hello,
> > > > > > > >
> > > > > > > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > > > > > > > My findings[1] back then were that U-Boot did set the eXecute
> > > > > > > > > Never bit only on OMAP, but not for other platforms.  So I
> > > > > > > > > could imagine this being the root cause of Patrick's issues as well:
> > > > > > > >
> > > > > > > > Rereading my own link, my memory is a little less fuzzy: eXecute
> > > > > > > > Never was being set, but was without effect due Manager mode
> > > > > > > > being set in the
> > > > > > > DACR:
> > > > > > > >
> > > > > > > > > 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, we'll need U-Boot to use client domain
> > > > > > > > > permissions, so the XN bit can function.
> > > > > > > >
> > > > > > > > > This issue has come up before and was fixed in de63ac278
> > > > > > > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2
> > > > only.
> > > > > > > > > It's equally applicable to all ARMv7-A platforms where caches
> > > > > > > > > are enabled.
> > > > > > > > > [1]: B3.7.2 - Execute-never restrictions on instruction
> > > > > > > > > fetching
> > > > > > > >
> > > > > > > > Hope this helps,
> > > > > > > > Ahmad
> > > > > > > >
> > > > > > >
> > > > > > > It most definitely does, thanks a lot.
> > > > > > >
> > > > > > > U-boot's mmu_setup() currently sets DACR to manager for all
> > > > > > > domains, so this is broken for all non-LPAE configurations running
> > > > > > > on v7 CPUs (except OMAP and perhaps others that fixed it
> > > > > > > individually). This affects all device mappings: not just secure
> > > > > > > DRAM for OP-TEE, but any MMIO register for any peripheral that is mapped
> > > > into the CPU's address space.
> > > > > > >
> > > > > > > Patrick, could you please check whether this fixes the issue as well?
> > > > > > >
> > > > > > > --- a/arch/arm/lib/cache-cp15.c
> > > > > > > +++ b/arch/arm/lib/cache-cp15.c
> > > > > > > @@ -202,9 +202,9 @@ static inline void mmu_setup(void)
> > > > > > >         asm volatile("mcr p15, 0, %0, c2, c0, 0"
> > > > > > >                      : : "r" (gd->arch.tlb_addr) : "memory");  #endif
> > > > > > > -       /* Set the access control to all-supervisor */
> > > > > > > +       /* Set the access control to client (0b01) for each of the
> > > > > > > + 16 domains */
> > > > > > >         asm volatile("mcr p15, 0, %0, c3, c0, 0"
> > > > > > > -                    : : "r" (~0));
> > > > > > > +                    : : "r" (0x55555555));
> > > > > > >
> > > > > > >         arm_init_domains();
> > > > > >
> > > > > > The test will take some time to be sure that solve my remaining issue because
> > > > issue is not always reproductible.
> > > > > >
> > > > > > At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line :
> > > > > >
> > > > > >       The XN attribute is not checked for domains marked as Manager. Read-
> > > > sensitive memory must
> > > > > >       not be included in domains marked as Manager, because the XN bit does
> > > > not prevent prefetches
> > > > > >       in these cases.
> > > > > >
> > > > > > So, I need  to test your patch +  DCACHE_OFF instead of INVALID (to
> > > > > > map with XN the OP-TEE region) in my patchset.
> > > > > >
> > > > > > FYI: I found the same DACR configuration is done in:
> > > > > >       arch/arm/cpu/armv7/ls102xa/cpu.c:199
> > > > > >
> > > > > > [1]
> > > > > > https://developer.arm.com/documentation/ddi0406/b/System-Level-Archi
> > > > > > tecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-cont
> > > > > > rol/The-Execute-Never--XN--attribute-and-instruction-prefetching?lan
> > > > > > g=en
> > > > > >
> > > > > > Patrick
> > > > > >
> > > > > > For information:
> > > > > >
> > > > > > At the beginning I wasn't sure that the current DACR configuration
> > > > > > is an issue because in found in pseudo code of
> > > > > > DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf
> > > > > >
> > > > > > B3.13.3 Address translation
> > > > > >       if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite)
> > > > then
> > > > > >               CheckPermission(tlbrecord.perms, mva,
> > > > > > tlbrecord.sectionnotpage, iswrite, ispriv);
> > > > > >
> > > > > > B3.13.4 Domain checking
> > > > > >       boolean CheckDomain(bits(4) domain, bits(32) mva, boolean
> > > > sectionnotpage, boolean iswrite)
> > > > > >               bitpos = 2*UInt(domain);
> > > > > >               case DACR<bitpos+1:bitpos> of
> > > > > >                       when ?00? DataAbort(mva, domain, sectionnotpage, iswrite,
> > > > DAbort_Domain);
> > > > > >                       when ?01? permissioncheck = TRUE;
> > > > > >                       when ?10? UNPREDICTABLE;
> > > > > >                       when ?11? permissioncheck = FALSE;
> > > > > >               return permissioncheck;
> > > > > >
> > > > > > B2.4.8 Access permission checking
> > > > > >       // CheckPermission()
> > > > > >       // =================
> > > > > >       CheckPermission(Permissions perms, bits(32) mva,
> > > > > >               boolean sectionnotpage, bits(4) domain, boolean
> > > > > > iswrite, boolean ispriv)
> > > > > >
> > > > > >               if SCTLR.AFE == ?0? then
> > > > > >                       perms.ap<0> = ?1?;
> > > > > >                       case perms.ap of
> > > > > >                               when ?000? abort = TRUE;
> > > > > >                               when ?001? abort = !ispriv;
> > > > > >                               when ?010? abort = !ispriv && iswrite;
> > > > > >                               when ?011? abort = FALSE;
> > > > > >                               when ?100? UNPREDICTABLE;
> > > > > >                               when ?101? abort = !ispriv || iswrite;
> > > > > >                               when ?110? abort = iswrite;
> > > > > >                               when ?111?
> > > > > >                       if MemorySystemArchitecture() == MemArch_VMSA then
> > > > > >                               abort = iswrite
> > > > > >                       else
> > > > > >                               UNPREDICTABLE;
> > > > > >                       if abort then
> > > > > >                               DataAbort(mva, domain, sectionnotpage, iswrite,
> > > > DAbort_Permission);
> > > > > >                       return;
> > > > > >
> > > > > > => it seens only the read/write permission is checked here
> > > > > > (perms.ap) => perms.xn is not used here
> > > > > >
> > > > > >       access_control = DRACR[r];
> > > > > >       perms.ap = access_control<10:8>;
> > > > > >       perms.xn = access_control<12>;
> > > > > >
> > > > > > with AP[2:0], bits [10:8]
> > > > > >       Access Permissions field. Indicates the read and write access permissions
> > > > for unprivileged
> > > > > >       and privileged accesses to the memory region.
> > > > > >
> > > > > > But now it is clear with [1]
> > > > >
> > > > > So, where did everything end up here?  I specifically didn't grab this
> > > > > series as it sounded like there was concern the problem should be
> > > > > solved via another patch.  Or would that be an in-addition-to?  Thanks!
> > > > >
> > > >
> > > > There are three different problems:
> > > > - ARMv7 non-LPAE uses the wrong domain settings
> > > > - no-map non-secure memory should not be mapped by u-boot
> > > > - secure world-only memory should not be described as memory in the device tree
> > > >
> > > > So I think this series definitely needs a respin at the very least.
> > >
> > > I gree: 3 differents issues in the thread
> > >
> > > - ARMv7 non-LPAE uses the wrong domain settings
> > >
> > > => bad DRACR configuration as raised by Ard & patch proposed by Ard in thread (but not pushed in mailing list)
> > > => I need to test it to check if it is correct my issue : today I can't reproduce the issue, still in my TODO list
> > >
> > > - no-map non-secure memory should not be mapped by u-boot
> > >
> > > => this serie, not ready for v2021.01, I think... I will push a V2 after my tests
> > >
> > > - secure world-only memory should not be described as memory in the device tree
> > >
> > > => I let Etienne Carriere manage it for OP-TEE point of view: today the secure memory is added by OP-TEE
> > >      in U-Boot device tree and U-Boot copy this node to Kernel Device tree
> > >
> > > I have no a clear opinion about it
> >
> > From the overall system description, it is far more convenient for
> > platforms to describe the
> > full physical memory through memory at ... nodes and exclude specific areas with
> > reserved-memory nodes. Among those specific areas, using no-map for
> > secure address
> > ranges seems applicable since the property is also intended for that
> > purpose (as the
> > reference to speculative accesses in the binding description).
>
> The point I made before was that secure and non-secure are two
> disjoint address spaces. The fact that TZ firewalls exist where you
> can move things from one side to the other does not imply that things
> works like this in the general case.
>
> E.g., you could have
>
> secure DRAM at S 0x0
> non-secure DRAM at NS 0x0
>
> where the ranges are backed by *different* memory. Since the DT
> description does not include the S/NS distinction, only the address
> range, the only thing we can assume when looking at memory@ and
> /reserved-memory is that everything it describes is NS.

From Arm Trustzone stand point, both secure and non-secure worlds
share the very same physical address space. I your example, physical
address 0x0 would refer to the same DRAM cell. Whether this cell is secure
or non-secure is a configuration set in the DRAM firmwall.

This is why memory node(s) describe physical DRAM that is meaningful
to both worlds but device configuration (firewall configuration) will make
DRAM address ranges legitimate to be accessed by either secure
or non-secure attribute of the memory mapping.

>
>
> > From my perspective, the issue discussed here seems rather more related to how
> > U-Boot handles FDT memory description. From my understanding, fixing
> > the Arm domain
> > mapping description in U-Boot should address the issue, as for secure
> > areas are concerned.
> >
> > Whereas should secure areas not be covered at all by FDT memory nodes,
> > I have no real strong
> > opinion.
> > - There are platforms statically describing memory and reserved-memory
> > nodes in DTS files.
> > I guess these could update DTS files exclude secure memory from memory
> > nodes, rather
> > than using reserved-memory nodes.
> > - There are platforms using runtime (actually boot time) update of the
> > FDT: secure world
> > (booting before the non-secure world) update memory description with knowledge
> > of the secure ranges. Adding reserved-memory node(s) is quite
> > straightforward. I guess
> > replacing memory@ nodes with new nodes describing non-secure memory
> > ranges is also
> > feasible.
> > - If no-map is to be used by U-Boot to not map related memory (needed
> > for the non-secure
> > reserved memory as discussed in this thread), why not reusing this
> > scheme also for secure
> > memory. Here we discussed statically assigned memory. If we consider a
> > platform where
> > secure world can dynamically re-assigned memory to secure/non-secure world, such
> > areas are not secure or non-secure memory, they are just memory....
> > reserved to secure
> > services eco-system.
> >
> >
> > In a previous post, Ard asked:
> > > So the next question is then, why describe it in the first place? Does
> > > OP-TEE rely on the DT description to figure out where the secure DDR
> > > is? Does the agent that programs the firewall get this information
> > > from DT?
> >
> > For the first question, I think the answer is that we can have a
> > unique description
> > for physical memory shared by both worlds. So I would say convenience
> > as I stated above.
> >
> > As for the other questions, yes, DT can definitely help the secure
> > world to configure
> > firewalls for the non-secure allowed accesses which both secure
> > and non-secure rely on. The secure world relies on it because non-secure memory
> > is seen by the secure world as legitimate areas where both worlds can share
> > buffers for communication.
> >
> > Best regards,
> > Etienne
> >
> > >
> > > Regards
> > >
> > > PAtrick

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Thu, 29 Oct 2020 at 17:06, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> On Thu, 29 Oct 2020 at 12:26, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 29 Oct 2020 at 11:40, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > Dear all,
> > >
> > > CC some fellow OP-TEE guys for this secure memory description topic.
> > >
> > >
> > > On Wed, 28 Oct 2020 at 11:33, Patrick DELAUNAY <patrick.delaunay@st.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > Sent: mardi 27 octobre 2020 22:05
> > > > >
> > > > > On Tue, 27 Oct 2020 at 18:25, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote:
> > > > > > > Hi Ard,
> > > > > > >
> > > > > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > > > > > Sent: mercredi 7 octobre 2020 15:16
> > > > > > > >
> > > > > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum@pengutronix.de>
> > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hello,
> > > > > > > > >
> > > > > > > > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > > > > > > > > My findings[1] back then were that U-Boot did set the eXecute
> > > > > > > > > > Never bit only on OMAP, but not for other platforms.  So I
> > > > > > > > > > could imagine this being the root cause of Patrick's issues as well:
> > > > > > > > >
> > > > > > > > > Rereading my own link, my memory is a little less fuzzy: eXecute
> > > > > > > > > Never was being set, but was without effect due Manager mode
> > > > > > > > > being set in the
> > > > > > > > DACR:
> > > > > > > > >
> > > > > > > > > > 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, we'll need U-Boot to use client domain
> > > > > > > > > > permissions, so the XN bit can function.
> > > > > > > > >
> > > > > > > > > > This issue has come up before and was fixed in de63ac278
> > > > > > > > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2
> > > > > only.
> > > > > > > > > > It's equally applicable to all ARMv7-A platforms where caches
> > > > > > > > > > are enabled.
> > > > > > > > > > [1]: B3.7.2 - Execute-never restrictions on instruction
> > > > > > > > > > fetching
> > > > > > > > >
> > > > > > > > > Hope this helps,
> > > > > > > > > Ahmad
> > > > > > > > >
> > > > > > > >
> > > > > > > > It most definitely does, thanks a lot.
> > > > > > > >
> > > > > > > > U-boot's mmu_setup() currently sets DACR to manager for all
> > > > > > > > domains, so this is broken for all non-LPAE configurations running
> > > > > > > > on v7 CPUs (except OMAP and perhaps others that fixed it
> > > > > > > > individually). This affects all device mappings: not just secure
> > > > > > > > DRAM for OP-TEE, but any MMIO register for any peripheral that is mapped
> > > > > into the CPU's address space.
> > > > > > > >
> > > > > > > > Patrick, could you please check whether this fixes the issue as well?
> > > > > > > >
> > > > > > > > --- a/arch/arm/lib/cache-cp15.c
> > > > > > > > +++ b/arch/arm/lib/cache-cp15.c
> > > > > > > > @@ -202,9 +202,9 @@ static inline void mmu_setup(void)
> > > > > > > >         asm volatile("mcr p15, 0, %0, c2, c0, 0"
> > > > > > > >                      : : "r" (gd->arch.tlb_addr) : "memory");  #endif
> > > > > > > > -       /* Set the access control to all-supervisor */
> > > > > > > > +       /* Set the access control to client (0b01) for each of the
> > > > > > > > + 16 domains */
> > > > > > > >         asm volatile("mcr p15, 0, %0, c3, c0, 0"
> > > > > > > > -                    : : "r" (~0));
> > > > > > > > +                    : : "r" (0x55555555));
> > > > > > > >
> > > > > > > >         arm_init_domains();
> > > > > > >
> > > > > > > The test will take some time to be sure that solve my remaining issue because
> > > > > issue is not always reproductible.
> > > > > > >
> > > > > > > At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line :
> > > > > > >
> > > > > > >       The XN attribute is not checked for domains marked as Manager. Read-
> > > > > sensitive memory must
> > > > > > >       not be included in domains marked as Manager, because the XN bit does
> > > > > not prevent prefetches
> > > > > > >       in these cases.
> > > > > > >
> > > > > > > So, I need  to test your patch +  DCACHE_OFF instead of INVALID (to
> > > > > > > map with XN the OP-TEE region) in my patchset.
> > > > > > >
> > > > > > > FYI: I found the same DACR configuration is done in:
> > > > > > >       arch/arm/cpu/armv7/ls102xa/cpu.c:199
> > > > > > >
> > > > > > > [1]
> > > > > > > https://developer.arm.com/documentation/ddi0406/b/System-Level-Archi
> > > > > > > tecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-cont
> > > > > > > rol/The-Execute-Never--XN--attribute-and-instruction-prefetching?lan
> > > > > > > g=en
> > > > > > >
> > > > > > > Patrick
> > > > > > >
> > > > > > > For information:
> > > > > > >
> > > > > > > At the beginning I wasn't sure that the current DACR configuration
> > > > > > > is an issue because in found in pseudo code of
> > > > > > > DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf
> > > > > > >
> > > > > > > B3.13.3 Address translation
> > > > > > >       if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite)
> > > > > then
> > > > > > >               CheckPermission(tlbrecord.perms, mva,
> > > > > > > tlbrecord.sectionnotpage, iswrite, ispriv);
> > > > > > >
> > > > > > > B3.13.4 Domain checking
> > > > > > >       boolean CheckDomain(bits(4) domain, bits(32) mva, boolean
> > > > > sectionnotpage, boolean iswrite)
> > > > > > >               bitpos = 2*UInt(domain);
> > > > > > >               case DACR<bitpos+1:bitpos> of
> > > > > > >                       when ?00? DataAbort(mva, domain, sectionnotpage, iswrite,
> > > > > DAbort_Domain);
> > > > > > >                       when ?01? permissioncheck = TRUE;
> > > > > > >                       when ?10? UNPREDICTABLE;
> > > > > > >                       when ?11? permissioncheck = FALSE;
> > > > > > >               return permissioncheck;
> > > > > > >
> > > > > > > B2.4.8 Access permission checking
> > > > > > >       // CheckPermission()
> > > > > > >       // =================
> > > > > > >       CheckPermission(Permissions perms, bits(32) mva,
> > > > > > >               boolean sectionnotpage, bits(4) domain, boolean
> > > > > > > iswrite, boolean ispriv)
> > > > > > >
> > > > > > >               if SCTLR.AFE == ?0? then
> > > > > > >                       perms.ap<0> = ?1?;
> > > > > > >                       case perms.ap of
> > > > > > >                               when ?000? abort = TRUE;
> > > > > > >                               when ?001? abort = !ispriv;
> > > > > > >                               when ?010? abort = !ispriv && iswrite;
> > > > > > >                               when ?011? abort = FALSE;
> > > > > > >                               when ?100? UNPREDICTABLE;
> > > > > > >                               when ?101? abort = !ispriv || iswrite;
> > > > > > >                               when ?110? abort = iswrite;
> > > > > > >                               when ?111?
> > > > > > >                       if MemorySystemArchitecture() == MemArch_VMSA then
> > > > > > >                               abort = iswrite
> > > > > > >                       else
> > > > > > >                               UNPREDICTABLE;
> > > > > > >                       if abort then
> > > > > > >                               DataAbort(mva, domain, sectionnotpage, iswrite,
> > > > > DAbort_Permission);
> > > > > > >                       return;
> > > > > > >
> > > > > > > => it seens only the read/write permission is checked here
> > > > > > > (perms.ap) => perms.xn is not used here
> > > > > > >
> > > > > > >       access_control = DRACR[r];
> > > > > > >       perms.ap = access_control<10:8>;
> > > > > > >       perms.xn = access_control<12>;
> > > > > > >
> > > > > > > with AP[2:0], bits [10:8]
> > > > > > >       Access Permissions field. Indicates the read and write access permissions
> > > > > for unprivileged
> > > > > > >       and privileged accesses to the memory region.
> > > > > > >
> > > > > > > But now it is clear with [1]
> > > > > >
> > > > > > So, where did everything end up here?  I specifically didn't grab this
> > > > > > series as it sounded like there was concern the problem should be
> > > > > > solved via another patch.  Or would that be an in-addition-to?  Thanks!
> > > > > >
> > > > >
> > > > > There are three different problems:
> > > > > - ARMv7 non-LPAE uses the wrong domain settings
> > > > > - no-map non-secure memory should not be mapped by u-boot
> > > > > - secure world-only memory should not be described as memory in the device tree
> > > > >
> > > > > So I think this series definitely needs a respin at the very least.
> > > >
> > > > I gree: 3 differents issues in the thread
> > > >
> > > > - ARMv7 non-LPAE uses the wrong domain settings
> > > >
> > > > => bad DRACR configuration as raised by Ard & patch proposed by Ard in thread (but not pushed in mailing list)
> > > > => I need to test it to check if it is correct my issue : today I can't reproduce the issue, still in my TODO list
> > > >
> > > > - no-map non-secure memory should not be mapped by u-boot
> > > >
> > > > => this serie, not ready for v2021.01, I think... I will push a V2 after my tests
> > > >
> > > > - secure world-only memory should not be described as memory in the device tree
> > > >
> > > > => I let Etienne Carriere manage it for OP-TEE point of view: today the secure memory is added by OP-TEE
> > > >      in U-Boot device tree and U-Boot copy this node to Kernel Device tree
> > > >
> > > > I have no a clear opinion about it
> > >
> > > From the overall system description, it is far more convenient for
> > > platforms to describe the
> > > full physical memory through memory at ... nodes and exclude specific areas with
> > > reserved-memory nodes. Among those specific areas, using no-map for
> > > secure address
> > > ranges seems applicable since the property is also intended for that
> > > purpose (as the
> > > reference to speculative accesses in the binding description).
> >
> > The point I made before was that secure and non-secure are two
> > disjoint address spaces. The fact that TZ firewalls exist where you
> > can move things from one side to the other does not imply that things
> > works like this in the general case.
> >
> > E.g., you could have
> >
> > secure DRAM at S 0x0
> > non-secure DRAM at NS 0x0
> >
> > where the ranges are backed by *different* memory. Since the DT
> > description does not include the S/NS distinction, only the address
> > range, the only thing we can assume when looking at memory@ and
> > /reserved-memory is that everything it describes is NS.
>
> From Arm Trustzone stand point, both secure and non-secure worlds
> share the very same physical address space. I your example, physical
> address 0x0 would refer to the same DRAM cell. Whether this cell is secure
> or non-secure is a configuration set in the DRAM firmwall.
>

How interesting! I looked this up in the ARM ARM, and the ARMv7 ARM has

(DDI0406C B1.5.1) The memory system provides mechanisms that prevent
the Non-secure state accessing regions of the physical memory
designated as Secure.

which implies that there is one physical address space, with some
parts marked secure and some parts marked as non-secure.

However, the latest ARM ARM has

(DDI0487E D1.4) The Armv8-A architecture provides two Security states,
each with an associated physical memory address space

So this means that what you are saying is correct for 32-bit ARM but
not for 64-bit ARM, which is a bit disappointing.


> This is why memory node(s) describe physical DRAM that is meaningful
> to both worlds but device configuration (firewall configuration) will make
> DRAM address ranges legitimate to be accessed by either secure
> or non-secure attribute of the memory mapping.
>

Yeah, so the problem is that this is no longer true. The same physical
address value may be valid in both address spaces, and so we either
need to quality addresses in DT as S or NS, or omit S addresses
entirely.

> >
> >
> > > From my perspective, the issue discussed here seems rather more related to how
> > > U-Boot handles FDT memory description. From my understanding, fixing
> > > the Arm domain
> > > mapping description in U-Boot should address the issue, as for secure
> > > areas are concerned.
> > >
> > > Whereas should secure areas not be covered at all by FDT memory nodes,
> > > I have no real strong
> > > opinion.
> > > - There are platforms statically describing memory and reserved-memory
> > > nodes in DTS files.
> > > I guess these could update DTS files exclude secure memory from memory
> > > nodes, rather
> > > than using reserved-memory nodes.
> > > - There are platforms using runtime (actually boot time) update of the
> > > FDT: secure world
> > > (booting before the non-secure world) update memory description with knowledge
> > > of the secure ranges. Adding reserved-memory node(s) is quite
> > > straightforward. I guess
> > > replacing memory@ nodes with new nodes describing non-secure memory
> > > ranges is also
> > > feasible.
> > > - If no-map is to be used by U-Boot to not map related memory (needed
> > > for the non-secure
> > > reserved memory as discussed in this thread), why not reusing this
> > > scheme also for secure
> > > memory. Here we discussed statically assigned memory. If we consider a
> > > platform where
> > > secure world can dynamically re-assigned memory to secure/non-secure world, such
> > > areas are not secure or non-secure memory, they are just memory....
> > > reserved to secure
> > > services eco-system.
> > >
> > >
> > > In a previous post, Ard asked:
> > > > So the next question is then, why describe it in the first place? Does
> > > > OP-TEE rely on the DT description to figure out where the secure DDR
> > > > is? Does the agent that programs the firewall get this information
> > > > from DT?
> > >
> > > For the first question, I think the answer is that we can have a
> > > unique description
> > > for physical memory shared by both worlds. So I would say convenience
> > > as I stated above.
> > >
> > > As for the other questions, yes, DT can definitely help the secure
> > > world to configure
> > > firewalls for the non-secure allowed accesses which both secure
> > > and non-secure rely on. The secure world relies on it because non-secure memory
> > > is seen by the secure world as legitimate areas where both worlds can share
> > > buffers for communication.
> > >
> > > Best regards,
> > > Etienne
> > >
> > > >
> > > > Regards
> > > >
> > > > PAtrick

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Jerome Forissier]

On 10/29/20 5:06 PM, Etienne Carriere wrote:
> On Thu, 29 Oct 2020 at 12:26, Ard Biesheuvel <ardb@kernel.org> wrote:
>> The point I made before was that secure and non-secure are two
>> disjoint address spaces. The fact that TZ firewalls exist where you
>> can move things from one side to the other does not imply that things
>> works like this in the general case.
>>
>> E.g., you could have
>>
>> secure DRAM at S 0x0
>> non-secure DRAM at NS 0x0
>>
>> where the ranges are backed by *different* memory. Since the DT
>> description does not include the S/NS distinction, only the address
>> range, the only thing we can assume when looking at memory@ and
>> /reserved-memory is that everything it describes is NS.
> 
> From Arm Trustzone stand point, both secure and non-secure worlds
> share the very same physical address space. I your example, physical
> address 0x0 would refer to the same DRAM cell. Whether this cell is secure
> or non-secure is a configuration set in the DRAM firmwall.

No, like Ard said it is a possibility but it doesn't have to be the
case. See the Armv8-A ARM (DDI 0487F.c) section D5.1.3 VMSA address
types and address spaces, "Physical address (PA)".

If we need to differentiate between non-secure and secure PA I suppose
we could use the status and secure-status properties in the memory
nodes, consistent with the usual usage described in [1].

As Etienne says, it seems that a majority of systems actually have a
single PA space with access control added on top, and by default the
secure state can access non-secure memory. That goes well with memory
nodes without a status nor a secure-status property, yet other
configurations can easily be supported.

[1]
https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/secure.txt

-- 
Jerome

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Etienne Carriere]

On Thu, 29 Oct 2020 at 17:35, Jerome Forissier <jerome@forissier.org> wrote:
>
>
>
> On 10/29/20 5:06 PM, Etienne Carriere wrote:
> > On Thu, 29 Oct 2020 at 12:26, Ard Biesheuvel <ardb@kernel.org> wrote:
> >> The point I made before was that secure and non-secure are two
> >> disjoint address spaces. The fact that TZ firewalls exist where you
> >> can move things from one side to the other does not imply that things
> >> works like this in the general case.
> >>
> >> E.g., you could have
> >>
> >> secure DRAM at S 0x0
> >> non-secure DRAM at NS 0x0
> >>
> >> where the ranges are backed by *different* memory. Since the DT
> >> description does not include the S/NS distinction, only the address
> >> range, the only thing we can assume when looking at memory@ and
> >> /reserved-memory is that everything it describes is NS.
> >
> > From Arm Trustzone stand point, both secure and non-secure worlds
> > share the very same physical address space. I your example, physical
> > address 0x0 would refer to the same DRAM cell. Whether this cell is secure
> > or non-secure is a configuration set in the DRAM firmwall.
>
> No, like Ard said it is a possibility but it doesn't have to be the
> case. See the Armv8-A ARM (DDI 0487F.c) section D5.1.3 VMSA address
> types and address spaces, "Physical address (PA)".

Ok. I didn't know that. Thanks both to highlight this and thanks for the refs.

However, I think this does not change the question on whether or not a memory
node in non-secure world FDT can cover address ranges that are carved out
with reserved-memory/no-map because non-secure world generic mapping
cannot presume valid default mapping attributes.


> If we need to differentiate between non-secure and secure PA I suppose
> we could use the status and secure-status properties in the memory
> nodes, consistent with the usual usage described in [1].
>
> As Etienne says, it seems that a majority of systems actually have a
> single PA space with access control added on top, and by default the
> secure state can access non-secure memory. That goes well with memory
> nodes without a status nor a secure-status property, yet other
> configurations can easily be supported.
>
> [1]
> https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/secure.txt
>
> --
> Jerome

Cheers,
Etienne

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Patrick DELAUNAY]

Hi Ahmad,

> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Sent: mercredi 7 octobre 2020 13:24
> 
> Hello Ard, Patrick,
> 
> On 10/7/20 12:26 PM, Ard Biesheuvel wrote:
> >> The issue is solved only when the region reserved by OP-TEE is no
> >> more mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't
> enough)
> >> as it is already done in Linux kernel.
> >>
> >
> > Spurious peculative accesses to device regions would be a severe
> > silicon bug, so I wonder what is going on here.
> >
> > (Apologies if we are rehashing stuff here that has already been
> > discussed - I don't remember the details)
> >
> > Are you sure that the speculative accesses were not caused by
> > misconfigured CPU or page tables, missing TLB maintenance, etc etc?
> > Because it really does smell like a software issue not a hardware
> > issue.
> 
> I debugged a similar issue a year ago on an i.MX6 UltraLite (also a Cortex-A7)
> that turned to ultimately be caused by barebox not mapping I/O memory as non-
> executable. This led to very interesting effects.
> 
> My findings[1] back then were that U-Boot did set the eXecute Never bit only on
> OMAP, but not for other platforms.  So I could imagine this being the root cause of
> Patrick's issues as well:
> The CPU is speculatively executing from the region that the firewalled DRAM is
> mapped at.
> 
> barebox now configures XN for non-RAM before it turns on the MMU. You should
> do that as well (in ARM arch code, not only for stm32mp1). Additionally, you will
> want to XN map the region where your OP-TEE sits at.
> 
> [1]: https://community.nxp.com/thread/511925

Thanks to point me this thread.

I checked DACR behavior and CheckDomain /  CheckPermission

In my case the cortex A7 try to access to part of DDR / mapped cacheable and bufferable, protected by firewall.

So to use DACR I always need to configure the MMU with several Domain
- unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC)
- secure part of DDR as  Domain 1 (DCACHE_OFF)

For other part of MMU region, the I/O registers are mapped as register with Domain 0 (D_CACHE_OFF)

Then I can set DACR = 0x55555555
=> Client Accesses are checked against the access permission bits in the TLB entry

You ar right, the access permission is check only for domain configurated as client in DACR

But in ARM architecture

B2.4.8 Access permission checking

CheckPermission() pseudo code
Only check perms.ap is checked
And perms.xp is not checked 

But as the secure memory is mapped cacheable by secure OS, OP-TEE
I think to avoid to map the region is the ARM preconized solution
As explain in my answer to ard in [1]

[1] http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958
 
> Cheers
> Ahmad
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ahmad Fatoum]

Hello Patrick,

On 10/9/20 5:52 PM, Patrick DELAUNAY wrote:
> I checked DACR behavior and CheckDomain /  CheckPermission
> 
> In my case the cortex A7 try to access to part of DDR / mapped cacheable and bufferable, protected by firewall.
> 
> So to use DACR I always need to configure the MMU with several Domain
> - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC)
> - secure part of DDR as  Domain 1 (DCACHE_OFF)
> 
> For other part of MMU region, the I/O registers are mapped as register with Domain 0 (D_CACHE_OFF)
> 
> Then I can set DACR = 0x55555555
> => Client Accesses are checked against the access permission bits in the TLB entry
> 
> You ar right, the access permission is check only for domain configurated as client in DACR
> 
> But in ARM architecture
> 
> B2.4.8 Access permission checking
> 
> CheckPermission() pseudo code
> Only check perms.ap is checked
> And perms.xp is not checked 
> 
> But as the secure memory is mapped cacheable by secure OS, OP-TEE
> I think to avoid to map the region is the ARM preconized solution
> As explain in my answer to ard in [1]
> 
> [1] http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958

I don't think the aliasing described in "A3.5.7 Memory access restrictions" applies if the
same memory is mapped twice only, once in secure and another in normal world.
Data is already segregated in the caches by means of a NS bit. Only thing you should need
to do within normal world is mapping it XN, cacheable and not be in manager domain.
Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't using either.
Why handle OP-TEE DRAM specially?)

Cheers
Ahmad

>  
>> Cheers
>> Ahmad
>>
>> --
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ahmad Fatoum]

On 10/9/20 7:12 PM, Ahmad Fatoum wrote:
> to do within normal world is mapping it XN, cacheable and not be in manager domain.

s/cacheable/uncacheable/ of course.

> Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't using either.
> Why handle OP-TEE DRAM specially?)
> 
> Cheers
> Ahmad
> 
>>  
>>> Cheers
>>> Ahmad
>>>
>>> --
>>> Pengutronix e.K.                           |                             |
>>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Patrick,
>
> On 10/9/20 5:52 PM, Patrick DELAUNAY wrote:
> > I checked DACR behavior and CheckDomain /  CheckPermission
> >
> > In my case the cortex A7 try to access to part of DDR / mapped cacheable and bufferable, protected by firewall.
> >
> > So to use DACR I always need to configure the MMU with several Domain
> > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC)
> > - secure part of DDR as  Domain 1 (DCACHE_OFF)
> >
> > For other part of MMU region, the I/O registers are mapped as register with Domain 0 (D_CACHE_OFF)
> >
> > Then I can set DACR = 0x55555555
> > => Client Accesses are checked against the access permission bits in the TLB entry
> >
> > You ar right, the access permission is check only for domain configurated as client in DACR
> >
> > But in ARM architecture
> >
> > B2.4.8 Access permission checking
> >
> > CheckPermission() pseudo code
> > Only check perms.ap is checked
> > And perms.xp is not checked
> >
> > But as the secure memory is mapped cacheable by secure OS, OP-TEE
> > I think to avoid to map the region is the ARM preconized solution
> > As explain in my answer to ard in [1]
> >
> > [1] http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958
>
> I don't think the aliasing described in "A3.5.7 Memory access restrictions" applies if the
> same memory is mapped twice only, once in secure and another in normal world.
> Data is already segregated in the caches by means of a NS bit. Only thing you should need
> to do within normal world is mapping it XN, cacheable and not be in manager domain.
> Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't using either.
> Why handle OP-TEE DRAM specially?)
>

Ah good point. The secure DDR is in the secure physical address space,
whereas the non-secure mapping refers to non-secure physical memory.
So from a coherency point of view, they are really not aliases of one
another, and so the mismatched attribute concern does not apply.

But this actually reinforces my previous point too: the secure and
non-secure physical address spaces are disjoint, and the DT can only
describe non-secure memory, so these regions don't belong in the DT
given to the OS in the first place.



>
>
> >
> >> Cheers
> >> Ahmad
> >>
> >> --
> >> Pengutronix e.K.                           |                             |
> >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Etienne Carriere]

On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Patrick,
>
> On 10/9/20 5:52 PM, Patrick DELAUNAY wrote:
> > I checked DACR behavior and CheckDomain /  CheckPermission
> >
> > In my case the cortex A7 try to access to part of DDR / mapped cacheable and bufferable, protected by firewall.
> >
> > So to use DACR I always need to configure the MMU with several Domain
> > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC)
> > - secure part of DDR as  Domain 1 (DCACHE_OFF)
> >
> > For other part of MMU region, the I/O registers are mapped as register with Domain 0 (D_CACHE_OFF)
> >
> > Then I can set DACR = 0x55555555
> > => Client Accesses are checked against the access permission bits in the TLB entry
> >
> > You ar right, the access permission is check only for domain configurated as client in DACR
> >
> > But in ARM architecture
> >
> > B2.4.8 Access permission checking
> >
> > CheckPermission() pseudo code
> > Only check perms.ap is checked
> > And perms.xp is not checked
> >
> > But as the secure memory is mapped cacheable by secure OS, OP-TEE
> > I think to avoid to map the region is the ARM preconized solution
> > As explain in my answer to ard in [1]
> >
> > [1] http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958
>
> I don't think the aliasing described in "A3.5.7 Memory access restrictions" applies if the
> same memory is mapped twice only, once in secure and another in normal world.
> Data is already segregated in the caches by means of a NS bit. Only thing you should need
> to do within normal world is mapping it XN, cacheable and not be in manager domain.
> Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't using either.
> Why handle OP-TEE DRAM specially?)

This is right regarding the secure memory/NS=0. But the
reserved-memory node for OP-TEE can also cover non-secure (shared)
memory that OP-TEE secure world maps Normal/NS=1. So U-boot should not
map such memory as Device/NS=0. That would jeopardize non-secure
memory content.

Note that platforms can define either a single reserved-memory node in
the FDT for both contiguous secure and shared DDR
or platforms can alternatively define 2 reserved_memory nodes: one for
the secure DDR and one for the non-secure shared DDR.

In the 1st case, the no-map property of the single reserved-memory
node should really not map the area in U-Boot.

In the 2nd case, the no-map property on the secure DDR reserved-memory
node must prevent U-Boot speculative accesses while node for shared
memory obviously doesn't need no-map.

This is to say that mapping as Device memory that has the no-map
property can be an issue.

Etienne



>
> Cheers
> Ahmad
>
> >
> >> Cheers
> >> Ahmad
> >>
> >> --
> >> Pengutronix e.K.                           |                             |
> >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Mon, 12 Oct 2020 at 11:09, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >
> > Hello Patrick,
> >
> > On 10/9/20 5:52 PM, Patrick DELAUNAY wrote:
> > > I checked DACR behavior and CheckDomain /  CheckPermission
> > >
> > > In my case the cortex A7 try to access to part of DDR / mapped cacheable and bufferable, protected by firewall.
> > >
> > > So to use DACR I always need to configure the MMU with several Domain
> > > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC)
> > > - secure part of DDR as  Domain 1 (DCACHE_OFF)
> > >
> > > For other part of MMU region, the I/O registers are mapped as register with Domain 0 (D_CACHE_OFF)
> > >
> > > Then I can set DACR = 0x55555555
> > > => Client Accesses are checked against the access permission bits in the TLB entry
> > >
> > > You ar right, the access permission is check only for domain configurated as client in DACR
> > >
> > > But in ARM architecture
> > >
> > > B2.4.8 Access permission checking
> > >
> > > CheckPermission() pseudo code
> > > Only check perms.ap is checked
> > > And perms.xp is not checked
> > >
> > > But as the secure memory is mapped cacheable by secure OS, OP-TEE
> > > I think to avoid to map the region is the ARM preconized solution
> > > As explain in my answer to ard in [1]
> > >
> > > [1] http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958
> >
> > I don't think the aliasing described in "A3.5.7 Memory access restrictions" applies if the
> > same memory is mapped twice only, once in secure and another in normal world.
> > Data is already segregated in the caches by means of a NS bit. Only thing you should need
> > to do within normal world is mapping it XN, cacheable and not be in manager domain.
> > Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't using either.
> > Why handle OP-TEE DRAM specially?)
>
> This is right regarding the secure memory/NS=0. But the
> reserved-memory node for OP-TEE can also cover non-secure (shared)
> memory that OP-TEE secure world maps Normal/NS=1. So U-boot should not
> map such memory as Device/NS=0. That would jeopardize non-secure
> memory content.
>

Agreed. If secure and non-secure need to have a coherent, cacheable
view of that shared memory, non-secure device mappings must be
avoided.

But is no-map really needed for that memory? It needs to be mapped in
any case to be usable for the non-secure world to communicate with the
secure world, right?

I'd assume the no-map is only needed if cacheable, inner shareable
mappings are a problem.

> Note that platforms can define either a single reserved-memory node in
> the FDT for both contiguous secure and shared DDR
> or platforms can alternatively define 2 reserved_memory nodes: one for
> the secure DDR and one for the non-secure shared DDR.
>
> In the 1st case, the no-map property of the single reserved-memory
> node should really not map the area in U-Boot.
>
> In the 2nd case, the no-map property on the secure DDR reserved-memory
> node must prevent U-Boot speculative accesses while node for shared
> memory obviously doesn't need no-map.
>
> This is to say that mapping as Device memory that has the no-map
> property can be an issue.
>

So in summary, there are two separate requirements resulting from this:
- the DT should not describe secure world memory as ordinary memory,
as the S and NS address spaces could overlap, and the distinction only
makes sense for agents running in the secure world;
- no-map should be honored by u-boot, but it should only be used if
the existence of a cacheable, inner shareable mapping of that memory
poses a problem.

-- 
Ard.

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Etienne Carriere]

On Mon, 12 Oct 2020 at 11:20, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 12 Oct 2020 at 11:09, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> > >
> > > Hello Patrick,
> > >
> > > On 10/9/20 5:52 PM, Patrick DELAUNAY wrote:
> > > > I checked DACR behavior and CheckDomain /  CheckPermission
> > > >
> > > > In my case the cortex A7 try to access to part of DDR / mapped cacheable and bufferable, protected by firewall.
> > > >
> > > > So to use DACR I always need to configure the MMU with several Domain
> > > > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC)
> > > > - secure part of DDR as  Domain 1 (DCACHE_OFF)
> > > >
> > > > For other part of MMU region, the I/O registers are mapped as register with Domain 0 (D_CACHE_OFF)
> > > >
> > > > Then I can set DACR = 0x55555555
> > > > => Client Accesses are checked against the access permission bits in the TLB entry
> > > >
> > > > You ar right, the access permission is check only for domain configurated as client in DACR
> > > >
> > > > But in ARM architecture
> > > >
> > > > B2.4.8 Access permission checking
> > > >
> > > > CheckPermission() pseudo code
> > > > Only check perms.ap is checked
> > > > And perms.xp is not checked
> > > >
> > > > But as the secure memory is mapped cacheable by secure OS, OP-TEE
> > > > I think to avoid to map the region is the ARM preconized solution
> > > > As explain in my answer to ard in [1]
> > > >
> > > > [1] http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958
> > >
> > > I don't think the aliasing described in "A3.5.7 Memory access restrictions" applies if the
> > > same memory is mapped twice only, once in secure and another in normal world.
> > > Data is already segregated in the caches by means of a NS bit. Only thing you should need
> > > to do within normal world is mapping it XN, cacheable and not be in manager domain.
> > > Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't using either.
> > > Why handle OP-TEE DRAM specially?)
> >
> > This is right regarding the secure memory/NS=0. But the
> > reserved-memory node for OP-TEE can also cover non-secure (shared)
> > memory that OP-TEE secure world maps Normal/NS=1. So U-boot should not
> > map such memory as Device/NS=0. That would jeopardize non-secure
> > memory content.
> >
>
> Agreed. If secure and non-secure need to have a coherent, cacheable
> view of that shared memory, non-secure device mappings must be
> avoided.
>
> But is no-map really needed for that memory? It needs to be mapped in
> any case to be usable for the non-secure world to communicate with the
> secure world, right?
>
> I'd assume the no-map is only needed if cacheable, inner shareable
> mappings are a problem.

The non-secure (shared) reserved-memory gets mapped by the related
driver (drivers/tee/optee/) at run time.
Hmm... actually, U-Boot driver does map or use this shared memory,
only Linux does.
But even if U-Boot optee driver does not map the shared memory, OP-TEE
secure world still likely does.

>
> > Note that platforms can define either a single reserved-memory node in
> > the FDT for both contiguous secure and shared DDR
> > or platforms can alternatively define 2 reserved_memory nodes: one for
> > the secure DDR and one for the non-secure shared DDR.
> >
> > In the 1st case, the no-map property of the single reserved-memory
> > node should really not map the area in U-Boot.
> >
> > In the 2nd case, the no-map property on the secure DDR reserved-memory
> > node must prevent U-Boot speculative accesses while node for shared
> > memory obviously doesn't need no-map.
> >
> > This is to say that mapping as Device memory that has the no-map
> > property can be an issue.
> >
>
> So in summary, there are two separate requirements resulting from this:
> - the DT should not describe secure world memory as ordinary memory,
> as the S and NS address spaces could overlap, and the distinction only
> makes sense for agents running in the secure world;

I don't mean S and NS can overlap.
I say that reserved-memory with no-map could cover as well S only or
contiguous S and NS ranges.
So no-map does not specifically mean S. It means: only driver knows
how to map the thing.

> - no-map should be honored by u-boot, but it should only be used if
> the existence of a cacheable, inner shareable mapping of that memory
> poses a problem.

I would say yes, does this description really cover the requirements?
I think no-map should be honored for memory that doesn't suit U-Boot
default mapping strategy if this one is Device memory in arm arch.
Note that linux default memory mapping strategy is
Normal/Shared/Cached. No-map should be agnostic from what is software
mapping strategy.


etienne


>
> --
> Ard.

[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Mon, 12 Oct 2020 at 11:51, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> On Mon, 12 Oct 2020 at 11:20, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 12 Oct 2020 at 11:09, Etienne Carriere
> > <etienne.carriere@linaro.org> wrote:
> > >
> > > On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> > > >
> > > > Hello Patrick,
> > > >
> > > > On 10/9/20 5:52 PM, Patrick DELAUNAY wrote:
> > > > > I checked DACR behavior and CheckDomain /  CheckPermission
> > > > >
> > > > > In my case the cortex A7 try to access to part of DDR / mapped cacheable and bufferable, protected by firewall.
> > > > >
> > > > > So to use DACR I always need to configure the MMU with several Domain
> > > > > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC)
> > > > > - secure part of DDR as  Domain 1 (DCACHE_OFF)
> > > > >
> > > > > For other part of MMU region, the I/O registers are mapped as register with Domain 0 (D_CACHE_OFF)
> > > > >
> > > > > Then I can set DACR = 0x55555555
> > > > > => Client Accesses are checked against the access permission bits in the TLB entry
> > > > >
> > > > > You ar right, the access permission is check only for domain configurated as client in DACR
> > > > >
> > > > > But in ARM architecture
> > > > >
> > > > > B2.4.8 Access permission checking
> > > > >
> > > > > CheckPermission() pseudo code
> > > > > Only check perms.ap is checked
> > > > > And perms.xp is not checked
> > > > >
> > > > > But as the secure memory is mapped cacheable by secure OS, OP-TEE
> > > > > I think to avoid to map the region is the ARM preconized solution
> > > > > As explain in my answer to ard in [1]
> > > > >
> > > > > [1] http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958
> > > >
> > > > I don't think the aliasing described in "A3.5.7 Memory access restrictions" applies if the
> > > > same memory is mapped twice only, once in secure and another in normal world.
> > > > Data is already segregated in the caches by means of a NS bit. Only thing you should need
> > > > to do within normal world is mapping it XN, cacheable and not be in manager domain.
> > > > Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't using either.
> > > > Why handle OP-TEE DRAM specially?)
> > >
> > > This is right regarding the secure memory/NS=0. But the
> > > reserved-memory node for OP-TEE can also cover non-secure (shared)
> > > memory that OP-TEE secure world maps Normal/NS=1. So U-boot should not
> > > map such memory as Device/NS=0. That would jeopardize non-secure
> > > memory content.
> > >
> >
> > Agreed. If secure and non-secure need to have a coherent, cacheable
> > view of that shared memory, non-secure device mappings must be
> > avoided.
> >
> > But is no-map really needed for that memory? It needs to be mapped in
> > any case to be usable for the non-secure world to communicate with the
> > secure world, right?
> >
> > I'd assume the no-map is only needed if cacheable, inner shareable
> > mappings are a problem.
>
> The non-secure (shared) reserved-memory gets mapped by the related
> driver (drivers/tee/optee/) at run time.
> Hmm... actually, U-Boot driver does map or use this shared memory,
> only Linux does.
> But even if U-Boot optee driver does not map the shared memory, OP-TEE
> secure world still likely does.
>
> >
> > > Note that platforms can define either a single reserved-memory node in
> > > the FDT for both contiguous secure and shared DDR
> > > or platforms can alternatively define 2 reserved_memory nodes: one for
> > > the secure DDR and one for the non-secure shared DDR.
> > >
> > > In the 1st case, the no-map property of the single reserved-memory
> > > node should really not map the area in U-Boot.
> > >
> > > In the 2nd case, the no-map property on the secure DDR reserved-memory
> > > node must prevent U-Boot speculative accesses while node for shared
> > > memory obviously doesn't need no-map.
> > >
> > > This is to say that mapping as Device memory that has the no-map
> > > property can be an issue.
> > >
> >
> > So in summary, there are two separate requirements resulting from this:
> > - the DT should not describe secure world memory as ordinary memory,
> > as the S and NS address spaces could overlap, and the distinction only
> > makes sense for agents running in the secure world;
>
> I don't mean S and NS can overlap.
> I say that reserved-memory with no-map could cover as well S only or
> contiguous S and NS ranges.

No, it cannot. That is basically the point I am trying to clarify.

Even if in this particular case, the firewall configuration simply
moves part of the DDR between the secure and non-secure address
spaces, this is not required by the architecture. In the general case,
non-secure address N and secure address N could both be valid, and
refer to different things. When you describe something in the
/reserved-memory node, there is no way to disambiguate between S and
NS, and so the only assumption we can make is that all memory ranges
and reservations described in DT are non-secure.

Since the DDR region we are trying to protect is secure memory, it
should not be in the DT, since the DT only describes non-secure memory
to begin with.

> So no-map does not specifically mean S. It means: only driver knows
> how to map the thing.
>
> > - no-map should be honored by u-boot, but it should only be used if
> > the existence of a cacheable, inner shareable mapping of that memory
> > poses a problem.
>
> I would say yes, does this description really cover the requirements?
> I think no-map should be honored for memory that doesn't suit U-Boot
> default mapping strategy if this one is Device memory in arm arch.
> Note that linux default memory mapping strategy is
> Normal/Shared/Cached. No-map should be agnostic from what is software
> mapping strategy.
>

I assumed that U-boot maps all of memory cacheable, inner shareable,
but if that is not the case, I agree the non-secure shared region
should not be mapped at all, and so for this region, the no-map
attribute would be appropriate (and u-boot should honour it)

[PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Patrick DELAUNAY]

Hi Ard,

> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: mercredi 7 octobre 2020 12:27
> 
> On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay <patrick.delaunay@st.com>
> wrote:
> >
> >
> > Hi,
> >
> > On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region
> > protected by a firewall. This region is reserved in device with "no-map"
> > property.
> >
> > But sometime the platform boot failed in U-boot on a Cortex A7 access
> > to this region (depending of the binary and the issue can change with
> > compiler version or with code alignment), then the firewall raise a
> > error, for example:
> >
> > E/TC:0   tzc_it_handler:19 TZC permission failure
> > E/TC:0   dump_fail_filter:420 Permission violation on filter 0
> > E/TC:0   dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged
> read,
> >          AXI ID 5c0
> > E/TC:0   Panic
> >
> > After investigation, the forbidden access is a speculative request
> > performed by the Cortex A7 because all the DDR is mapped as MEMORY
> > with CACHEABLE property.
> >
> > The issue is solved only when the region reserved by OP-TEE is no more
> > mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it
> > is already done in Linux kernel.
> >
> 
> Spurious peculative accesses to device regions would be a severe silicon bug, so
> I wonder what is going on here.
> 
> (Apologies if we are rehashing stuff here that has already been discussed - I don't
> remember the details)
> 
> Are you sure that the speculative accesses were not caused by misconfigured
> CPU or page tables, missing TLB maintenance, etc etc?
> Because it really does smell like a software issue not a hardware issue.
> 
> > I think that can be a general issue for ARM architecture: the no-map
> > tag in device should be respected by U-boot, so I propose a  generic
> > solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
> >
> > This serie is composed by 7 patches
> > - 1..4/7: preliminary steps to support flags in library in lmb
> >   (as it is done in memblock.c in Linux)
> > - 5/7: unitary test on the added feature in lmb lib
> > - 6/7: save the no-map flags in lmb when the device tree is parsed
> > - 7/7: update the generic behavior for "no-map" region in
> >        arm/lib/cache-cp15.c::dram_bank_mmu_setup()
> >
> > It is a rebase on master branch of previous RFC [2].
> >
> > I can change this last patch if this feature is note required by other
> > ARM architecture; it is a weak function so I can avoid to map the
> > region with "no-map" property in device tree only for STM32MP
> > architecture (in arch/arm/mach-stm32mp/cpu.c).
> >
> > See also [1] which handle same speculative access on armv8 for area
> > with Executable attribute.
> >
> > [1]
> > http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-
> > marek.bykowski at gmail.com/ [2]
> > http://patchwork.ozlabs.org/project/uboot/list/?series=199486&archive=
> > both&state=*
> >
> > Regards
> > Patrick
> >
> >
> > Patrick Delaunay (7):
> >   lmb: Add support of flags for no-map properties
> >   lmb: add lmb_is_reserved_flags
> >   lmb: remove lmb_region.size
> >   lmb: add lmb_dump_region() function
> >   test: lmb: add test for lmb_reserve_flags
> >   image-fdt: save no-map parameter of reserve-memory
> >   arm: cache: cp15: don't map the reserved region with no-map property
> >
> >  arch/arm/include/asm/system.h |   3 +
> >  arch/arm/lib/cache-cp15.c     |  19 ++++++-
> >  common/image-fdt.c            |  23 +++++---
> >  include/lmb.h                 |  22 +++++++-
> >  lib/lmb.c                     | 100 +++++++++++++++++++++++-----------
> >  test/lib/lmb.c                |  89 ++++++++++++++++++++++++++++++
> >  6 files changed, 212 insertions(+), 44 deletions(-)
> >
> > --
> > 2.17.1
> >

No, I don't yet described the issue in details on mailing list.
So I will explain the investigation done and my conclusion.

On STM32MP15x platform we have firewall to protect the access to DDR (TZC400 IP).

When OP-TEE is used, the boot chain is:

1/ ROM-Code (secure)
2/ TF-A (BL2) in SYSRAM (secure)
3/ OP-TEE in  SYSRAM and DDR (secure)
4/ U-Boot in DDR
5/ Linux lernel 

OP-TEE is loaded by TF-A BL2
- in SYRAM (for pager part)
- in DDR (for pageable part).

U-Boot is loaded by TF-A BL2 in DDR.

When OP-TEE is execute,  it protects with TZC400 firewall the reserved part of DDR as defined in device tree :

For example : ./arch/arm/dts/stm32mp157a-ed1-u-boot.dtsi:42:
	reserved-memory {
		optee at fe000000 {
			reg = <0xfe000000 0x02000000>;
			no-map;
		};
	};

When OP-TEE is running in secure world (secure monitor is resident),
it jump to normal world (unsecure) = U-Boot

After relocation, U-Boot maps all the DDR as normal memory
(cacheable, bufferable, executable) and activate data cahe

Then, sometime (because depending of the compilation), the firewall detect an unsecure access
 to OP-TEE secured region when U-Boot is running.

We have investigate this access with gdb:
- it is not an direct requet done by U-Boot code : we have no issue whe code is executed step by step
- we have no issue when CACHE (instruction and data) is deactivated
- no cache or TLB maintenance in this part of code
- just after the PC,  we found in  memory a array whith content decoded as branch instruction
  to some address located in OP-TEE protected memory
  (even if the content of the array is not instruction but u32 values) 
  and it is exactly this address which cause the firewall error.

PS: if I modify the code (by adding printf for example), the array change: 
      it content is no more see as branch instruction and the issue disappears.
       
My conclusion:
  the A7 core "see" the branch instruction near the PC with address in DDR 
  and this address is marked as cacheable/executable in MMU
  So the instruction cache load this code in the cache instruction (preloaded for Cortex A7 pipeline).

I found this note in ARM documentation (found in armv8 but it is the same for armV7):

https://developer.arm.com/architectures/learn-the-architecture/armv8-a-memory-model/device-memory

Note: There is a subtle distinction here that is easy to miss. Marking a region as Device prevents speculative
          data accesses only.  Marking a region as non-executable prevents speculative instruction accesses.
         This means that, to prevent any speculative accesses, a region must be marked as both Device and non-executable.
  
For me to avoid any unexpected access to protected memory by firewall the OP-TEE reserved
memory must at least be configured as device memory and not executable...

But after tests, it wasn't enough.
Even if we set the OP-TEE memory with DCACHE_OFF value,
TZC400 see other issue for secure to no secure transition access
(during SMC execution for request to secure monitor).

Then I remember a other requirement in ARM architecture:
to avoid unexpected behavior the same region should be mapped not mapped as device and memory:
(OP-TEE mark the reserved region as normal memory / cacheable only accessible 
by secure world / protected by TZC400)

Reference = ARMv7 Architecture reference:

A3.5.7 Memory access restrictions

Behavior is UNPREDICTABLE if the same memory location:
? is marked as Shareable Normal and Non-shareable Normal
? is marked as having different memory types (Normal, Device, or Strongly-ordered)
? is marked as having different cacheability attributes
? is marked as being Shareable Device and Non-shareable Device memory.
Such memory marking contradictions can occur, for example, by the use of aliases in a virtual to
physical address mapping

It is why I propose this patchset to un-map the OP-TEE memory, as it is indicated in device tree;
I don't see other solution to respect the ARM requirements.

So for me it is not a SOC issue or a SW bug, but an ARM architecture constraint for speculative access
to normal memory region (marked cacheable/shareable/executable).

This prevent any issue for STM32MP15x platform but also for other platforms
when some part of memory must be protected (no access allowed/protected by firewall)
BEFORE U-Boot execution.

I hope that it is more clear now.

Regards

Patrick

[PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Fri, 9 Oct 2020 at 13:19, Patrick DELAUNAY <patrick.delaunay@st.com> wrote:
>
> Hi Ard,
>
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: mercredi 7 octobre 2020 12:27
> >
> > On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay <patrick.delaunay@st.com>
> > wrote:
> > >
> > >
> > > Hi,
> > >
> > > On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region
> > > protected by a firewall. This region is reserved in device with "no-map"
> > > property.
> > >
> > > But sometime the platform boot failed in U-boot on a Cortex A7 access
> > > to this region (depending of the binary and the issue can change with
> > > compiler version or with code alignment), then the firewall raise a
> > > error, for example:
> > >
> > > E/TC:0   tzc_it_handler:19 TZC permission failure
> > > E/TC:0   dump_fail_filter:420 Permission violation on filter 0
> > > E/TC:0   dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged
> > read,
> > >          AXI ID 5c0
> > > E/TC:0   Panic
> > >
> > > After investigation, the forbidden access is a speculative request
> > > performed by the Cortex A7 because all the DDR is mapped as MEMORY
> > > with CACHEABLE property.
> > >
> > > The issue is solved only when the region reserved by OP-TEE is no more
> > > mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it
> > > is already done in Linux kernel.
> > >
> >
> > Spurious peculative accesses to device regions would be a severe silicon bug, so
> > I wonder what is going on here.
> >
> > (Apologies if we are rehashing stuff here that has already been discussed - I don't
> > remember the details)
> >
> > Are you sure that the speculative accesses were not caused by misconfigured
> > CPU or page tables, missing TLB maintenance, etc etc?
> > Because it really does smell like a software issue not a hardware issue.
> >
> > > I think that can be a general issue for ARM architecture: the no-map
> > > tag in device should be respected by U-boot, so I propose a  generic
> > > solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
> > >
> > > This serie is composed by 7 patches
> > > - 1..4/7: preliminary steps to support flags in library in lmb
> > >   (as it is done in memblock.c in Linux)
> > > - 5/7: unitary test on the added feature in lmb lib
> > > - 6/7: save the no-map flags in lmb when the device tree is parsed
> > > - 7/7: update the generic behavior for "no-map" region in
> > >        arm/lib/cache-cp15.c::dram_bank_mmu_setup()
> > >
> > > It is a rebase on master branch of previous RFC [2].
> > >
> > > I can change this last patch if this feature is note required by other
> > > ARM architecture; it is a weak function so I can avoid to map the
> > > region with "no-map" property in device tree only for STM32MP
> > > architecture (in arch/arm/mach-stm32mp/cpu.c).
> > >
> > > See also [1] which handle same speculative access on armv8 for area
> > > with Executable attribute.
> > >
> > > [1]
> > > http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-
> > > marek.bykowski at gmail.com/ [2]
> > > http://patchwork.ozlabs.org/project/uboot/list/?series=199486&archive=
> > > both&state=*
> > >
> > > Regards
> > > Patrick
> > >
> > >
> > > Patrick Delaunay (7):
> > >   lmb: Add support of flags for no-map properties
> > >   lmb: add lmb_is_reserved_flags
> > >   lmb: remove lmb_region.size
> > >   lmb: add lmb_dump_region() function
> > >   test: lmb: add test for lmb_reserve_flags
> > >   image-fdt: save no-map parameter of reserve-memory
> > >   arm: cache: cp15: don't map the reserved region with no-map property
> > >
> > >  arch/arm/include/asm/system.h |   3 +
> > >  arch/arm/lib/cache-cp15.c     |  19 ++++++-
> > >  common/image-fdt.c            |  23 +++++---
> > >  include/lmb.h                 |  22 +++++++-
> > >  lib/lmb.c                     | 100 +++++++++++++++++++++++-----------
> > >  test/lib/lmb.c                |  89 ++++++++++++++++++++++++++++++
> > >  6 files changed, 212 insertions(+), 44 deletions(-)
> > >
> > > --
> > > 2.17.1
> > >
>
> No, I don't yet described the issue in details on mailing list.
> So I will explain the investigation done and my conclusion.
>
> On STM32MP15x platform we have firewall to protect the access to DDR (TZC400 IP).
>
> When OP-TEE is used, the boot chain is:
>
> 1/ ROM-Code (secure)
> 2/ TF-A (BL2) in SYSRAM (secure)
> 3/ OP-TEE in  SYSRAM and DDR (secure)
> 4/ U-Boot in DDR
> 5/ Linux lernel
>
> OP-TEE is loaded by TF-A BL2
> - in SYRAM (for pager part)
> - in DDR (for pageable part).
>
> U-Boot is loaded by TF-A BL2 in DDR.
>
> When OP-TEE is execute,  it protects with TZC400 firewall the reserved part of DDR as defined in device tree :
>
> For example : ./arch/arm/dts/stm32mp157a-ed1-u-boot.dtsi:42:
>         reserved-memory {
>                 optee at fe000000 {
>                         reg = <0xfe000000 0x02000000>;
>                         no-map;
>                 };
>         };
>
> When OP-TEE is running in secure world (secure monitor is resident),
> it jump to normal world (unsecure) = U-Boot
>
> After relocation, U-Boot maps all the DDR as normal memory
> (cacheable, bufferable, executable) and activate data cahe
>
> Then, sometime (because depending of the compilation), the firewall detect an unsecure access
>  to OP-TEE secured region when U-Boot is running.
>
> We have investigate this access with gdb:
> - it is not an direct requet done by U-Boot code : we have no issue whe code is executed step by step
> - we have no issue when CACHE (instruction and data) is deactivated
> - no cache or TLB maintenance in this part of code
> - just after the PC,  we found in  memory a array whith content decoded as branch instruction
>   to some address located in OP-TEE protected memory
>   (even if the content of the array is not instruction but u32 values)
>   and it is exactly this address which cause the firewall error.
>
> PS: if I modify the code (by adding printf for example), the array change:
>       it content is no more see as branch instruction and the issue disappears.
>
> My conclusion:
>   the A7 core "see" the branch instruction near the PC with address in DDR
>   and this address is marked as cacheable/executable in MMU
>   So the instruction cache load this code in the cache instruction (preloaded for Cortex A7 pipeline).
>
> I found this note in ARM documentation (found in armv8 but it is the same for armV7):
>
> https://developer.arm.com/architectures/learn-the-architecture/armv8-a-memory-model/device-memory
>
> Note: There is a subtle distinction here that is easy to miss. Marking a region as Device prevents speculative
>           data accesses only.  Marking a region as non-executable prevents speculative instruction accesses.
>          This means that, to prevent any speculative accesses, a region must be marked as both Device and non-executable.
>
> For me to avoid any unexpected access to protected memory by firewall the OP-TEE reserved
> memory must at least be configured as device memory and not executable...
>
> But after tests, it wasn't enough.
> Even if we set the OP-TEE memory with DCACHE_OFF value,
> TZC400 see other issue for secure to no secure transition access
> (during SMC execution for request to secure monitor).
>
> Then I remember a other requirement in ARM architecture:
> to avoid unexpected behavior the same region should be mapped not mapped as device and memory:
> (OP-TEE mark the reserved region as normal memory / cacheable only accessible
> by secure world / protected by TZC400)
>
> Reference = ARMv7 Architecture reference:
>
> A3.5.7 Memory access restrictions
>
> Behavior is UNPREDICTABLE if the same memory location:
> ? is marked as Shareable Normal and Non-shareable Normal
> ? is marked as having different memory types (Normal, Device, or Strongly-ordered)
> ? is marked as having different cacheability attributes
> ? is marked as being Shareable Device and Non-shareable Device memory.
> Such memory marking contradictions can occur, for example, by the use of aliases in a virtual to
> physical address mapping
>
> It is why I propose this patchset to un-map the OP-TEE memory, as it is indicated in device tree;
> I don't see other solution to respect the ARM requirements.
>
> So for me it is not a SOC issue or a SW bug, but an ARM architecture constraint for speculative access
> to normal memory region (marked cacheable/shareable/executable).
>
> This prevent any issue for STM32MP15x platform but also for other platforms
> when some part of memory must be protected (no access allowed/protected by firewall)
> BEFORE U-Boot execution.
>
> I hope that it is more clear now.
>

Yes, thanks for the elaborate explanation. You are correct that the
ARM architecture forbids memory aliases with mismatched attributes, so
it is definitely better not to map the memory at all.

So the next question is then, why describe it in the first place? Does
OP-TEE rely on the DT description to figure out where the secure DDR
is? Does the agent that programs the firewall get this information
from DT?

The /reserved-memory node and its contents are consumed by the OS, and
whether or not u-boot should honor them too is debatable. So I think
the robust way to deal with this on your platform would be not to
describe the secure DDR at all in the DT that is exposed to u-boot and
onwards.

And of course, your issue did uncover a serious problem in the !LPAE
code that still needs to get fixed as well.

Thanks,
Ard.