[PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
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.... [2] http://patchwork.ozlabs.org/project/uboot/list/?series=199486&archive=bo...
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(-)
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,
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() */
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)
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
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);
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);
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 */
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.... [2] http://patchwork.ozlabs.org/project/uboot/list/?series=199486&archive=bo...
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
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
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.
Cheers Ahmad
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();
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();
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.
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
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/...
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 DACRbitpos+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]
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/...
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 DACRbitpos+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!
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/...
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 DACRbitpos+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.
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 doesnot 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
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 doesnot 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@... 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
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 doesnot 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@... 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
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 doesnot 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@... 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
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 DACRbitpos+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@... 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
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
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
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.
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-reserve...
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 |
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-reserve...
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 |
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 |
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-reserve...
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 |
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-reserve...
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 |
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-reserve...
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.
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-reserve...
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.
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-reserve...
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)
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 5c0E/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@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@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-memor...
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
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 5c0E/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@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@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-memor...
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.
participants (7)
-
Ahmad Fatoum -
Ard Biesheuvel -
Etienne Carriere -
Jerome Forissier -
Patrick DELAUNAY -
Patrick Delaunay
-
Tom Rini