[PATCH v2 00/10] ARM: mmu: inhibit speculation into secure memory

[PATCH v2 00/10] ARM: mmu: inhibit speculation into secure memory

[Ahmad Fatoum]

v1 -> v2:
  - fold misplaced hunk changing %u added in [01/10] into 0x%x in
    [02/10] directly into [01/10] (Ulrich)
  - Correct typo in commit message (Sascha)

When setting up page tables, barebox marks all the address space as
eXecute Never and uncached, except for the memory banks. If we happen to
have secure memory, this is andequate as speculative execution may read
from secure memory or even attempt to execute it leading to spurious
data aborts. The way around this so far was either having OP-TEE in SRAM
(which normally isn't a barebox memory bank) or having it at the end of
DRAM, but adjusting size, so it's not covered by a memory bank.

This adds a generic solution to the issue. We already request the SDRAM
regions described by the reserved memory entries in the DT. We go a step
further and mark them as IORESOURCE_BUSY, which we can then evaluat in
the MMU setup code to map these regions uncached and eXecute Never.

There has been previous attempts by Rouven to achieve this, the latest
being:

  https://lore.barebox.org/barebox/20210803094418.475609-1-r.czerwinski@pengutronix.de/

While this series tries to achieve the same end goal, it goes about it
in a different manner: We don't use FDT fixup table to tell us what to
nstead have both the FDT fixup table and the /reserved-memory child
nodes feed into the barebox request_sdram_region allocator and then
use to apply caching attributes.

Note that this doesn't yet solve all problems. For example, PPA secure
monitor installation on Layerscape may happen with CONFIG_MMU_EARLY=y,
in which case barebox in EL2 may speculate into the secure memory
before any device tree reserved-memory settings are considered. For this
reason, both early MMU and normal MMU setup must be aware of the
reserved memory regions. The original patch set by Rouven used FDT
parsing in PBL to achieve this, but this is omitted here to limit
scope of the patch series. Instead we only handle the CONFIG_OPTEE_SIZE
case out-of-the-box.

Ahmad Fatoum (9):
  resource: add flags parameter to __request_region
  common: allow requesting SDRAM regions with custom flags
  memory: define reserve_sdram_region helper
  init: define new postmem_initcall()
  of: reserved-mem: reserve regions prior to mmu_initcall()
  ARM: mmu64: map reserved regions uncached
  ARM: mmu: define attrs_uncached_mem() helper
  ARM: early-mmu: don't cache/prefetch OPTEE_SIZE bytes from end of
    memory
  commands: iomem: point out [R]eserved regions

Rouven Czerwinski (1):
  ARM: mmu: use reserve mem entries to modify maps

 arch/arm/cpu/mmu-common.h         | 15 ++++++++++++
 arch/arm/cpu/mmu.c                | 40 ++++++++++++++++++++++---------
 arch/arm/cpu/mmu.h                |  9 +++++--
 arch/arm/cpu/mmu_64.c             | 10 +++++++-
 arch/arm/cpu/start.c              |  2 +-
 arch/arm/cpu/uncompress.c         |  2 +-
 commands/iomemport.c              |  9 ++++---
 common/memory.c                   | 27 ++++++++-------------
 common/resource.c                 | 13 +++++-----
 drivers/of/reserved-mem.c         | 34 +++++++++++++++++---------
 include/asm-generic/barebox.lds.h |  1 +
 include/init.h                    | 21 ++++++++--------
 include/linux/ioport.h            |  4 ++--
 include/memory.h                  | 25 +++++++++++++++++--
 include/of.h                      |  7 ------
 15 files changed, 145 insertions(+), 74 deletions(-)

-- 
2.30.2

[PATCH v2 01/10] resource: add flags parameter to __request_region

[Ahmad Fatoum]

__request_region allocates a child resource within the parent resource,
which so far always had a flags field of zero. Later commits will
use the flags field to mark reserved SDRAM regions, so MMU init code can
take that into consideration and ensure that CPU doesn't speculate into
these regions and risk aborts. Prepare for this by giving
__request_region a flags parameter.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/memory.c        |  4 ++--
 common/resource.c      | 13 +++++++------
 include/linux/ioport.h |  4 ++--
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/common/memory.c b/common/memory.c
index fd782c7f24f6..03fec1f1eb0e 100644
--- a/common/memory.c
+++ b/common/memory.c
@@ -208,8 +208,8 @@ struct resource *request_sdram_region(const char *name, resource_size_t start,
 	for_each_memory_bank(bank) {
 		struct resource *res;
 
-		res = __request_region(bank->res, name, start,
-				       start + size - 1);
+		res = __request_region(bank->res, start, start + size - 1,
+				       name, 0);
 		if (!IS_ERR(res))
 			return res;
 	}
diff --git a/common/resource.c b/common/resource.c
index f96cb94b5074..8678609229ab 100644
--- a/common/resource.c
+++ b/common/resource.c
@@ -28,8 +28,8 @@ static int init_resource(struct resource *res, const char *name)
  * resources.
  */
 struct resource *__request_region(struct resource *parent,
-		const char *name, resource_size_t start,
-		resource_size_t end)
+				  resource_size_t start, resource_size_t end,
+				  const char *name, unsigned flags)
 {
 	struct resource *r, *new;
 
@@ -73,15 +73,16 @@ struct resource *__request_region(struct resource *parent,
 	}
 
 ok:
-	debug("%s ok: 0x%08llx:0x%08llx\n", __func__,
+	debug("%s ok: 0x%08llx:0x%08llx flags=0x%x\n", __func__,
 			(unsigned long long)start,
-			(unsigned long long)end);
+			(unsigned long long)end, flags);
 
 	new = xzalloc(sizeof(*new));
 	init_resource(new, name);
 	new->start = start;
 	new->end = end;
 	new->parent = parent;
+	new->flags = flags;
 	list_add_tail(&new->sibling, &r->sibling);
 
 	return new;
@@ -138,7 +139,7 @@ struct resource iomem_resource = {
 struct resource *request_iomem_region(const char *name,
 		resource_size_t start, resource_size_t end)
 {
-	return __request_region(&iomem_resource, name, start, end);
+	return __request_region(&iomem_resource, start, end, name, 0);
 }
 
 /* The root resource for the whole io-mapped io space */
@@ -157,7 +158,7 @@ struct resource *request_ioport_region(const char *name,
 {
 	struct resource *res;
 
-	res = __request_region(&ioport_resource, name, start, end);
+	res = __request_region(&ioport_resource, start, end, name, 0);
 	if (IS_ERR(res))
 		return ERR_CAST(res);
 
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index ee9587ba0feb..c6328e9a7fc2 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -155,8 +155,8 @@ struct resource *request_ioport_region(const char *name,
 		resource_size_t start, resource_size_t end);
 
 struct resource *__request_region(struct resource *parent,
-		const char *name, resource_size_t end,
-		resource_size_t size);
+				  resource_size_t start, resource_size_t end,
+				  const char *name, unsigned flags);
 
 int __merge_regions(const char *name,
 		struct resource *resa, struct resource *resb);
-- 
2.30.2

[PATCH v2 02/10] common: allow requesting SDRAM regions with custom flags

[Ahmad Fatoum]

Now that __request_region accepts a flag parameter, define
__request_sdram_region, which also accepts a flag parameter and passes
it through. The default flags for request_sdram_region() will be
IORESOURCE_MEM as that ensures resource_contains behaves correctly when
comparing against another memory resource.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/memory.c  |  8 +++++---
 include/memory.h | 13 +++++++++++--
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/common/memory.c b/common/memory.c
index 03fec1f1eb0e..347f83fd4cf8 100644
--- a/common/memory.c
+++ b/common/memory.c
@@ -200,16 +200,18 @@ mmu_initcall(add_mem_devices);
 /*
  * Request a region from the registered sdram
  */
-struct resource *request_sdram_region(const char *name, resource_size_t start,
-		resource_size_t size)
+struct resource *__request_sdram_region(const char *name, unsigned flags,
+					resource_size_t start, resource_size_t size)
 {
 	struct memory_bank *bank;
 
+	flags |= IORESOURCE_MEM;
+
 	for_each_memory_bank(bank) {
 		struct resource *res;
 
 		res = __request_region(bank->res, start, start + size - 1,
-				       name, 0);
+				       name, flags);
 		if (!IS_ERR(res))
 			return res;
 	}
diff --git a/include/memory.h b/include/memory.h
index c793bb51ed77..31da5d74d568 100644
--- a/include/memory.h
+++ b/include/memory.h
@@ -23,8 +23,17 @@ int barebox_add_memory_bank(const char *name, resource_size_t start,
 
 #define for_each_memory_bank(mem)	list_for_each_entry(mem, &memory_banks, list)
 
-struct resource *request_sdram_region(const char *name, resource_size_t start,
-		resource_size_t size);
+struct resource *__request_sdram_region(const char *name, unsigned flags,
+					resource_size_t start, resource_size_t size);
+
+static inline struct resource *request_sdram_region(const char *name,
+						    resource_size_t start,
+						    resource_size_t size)
+{
+	/* IORESOURCE_MEM is implicit for all SDRAM regions */
+	return __request_sdram_region(name, 0, start, size);
+}
+
 int release_sdram_region(struct resource *res);
 
 void memory_bank_find_space(struct memory_bank *bank, resource_size_t *retstart,
-- 
2.30.2

[PATCH v2 03/10] memory: define reserve_sdram_region helper

[Ahmad Fatoum]

We use request_sdram_regions both for code that barebox wants to use for
itself (e.g. barebox stack or initrd) and to reserve regions for secure
firmware that barebox shouldn't overwrite. We need to differentiate
between them, so we can apply different caching flags depending on
region, so add a new reserve_sdram_region helper that may be used and
also add an for_each_reserved_region iterator for use in the MMU code
that applied the different caching flags later on.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/memory.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/memory.h b/include/memory.h
index 31da5d74d568..ffd66db02ba0 100644
--- a/include/memory.h
+++ b/include/memory.h
@@ -4,6 +4,7 @@
 
 #include <linux/types.h>
 #include <linux/list.h>
+#include <linux/ioport.h>
 
 void mem_malloc_init(void *start, void *end);
 ulong mem_malloc_start(void);
@@ -22,6 +23,9 @@ int barebox_add_memory_bank(const char *name, resource_size_t start,
 				    resource_size_t size);
 
 #define for_each_memory_bank(mem)	list_for_each_entry(mem, &memory_banks, list)
+#define for_each_reserved_region(mem, rsv) \
+	list_for_each_entry(rsv, &(mem)->res->children, sibling) \
+	if (((rsv)->flags & IORESOURCE_BUSY))
 
 struct resource *__request_sdram_region(const char *name, unsigned flags,
 					resource_size_t start, resource_size_t size);
@@ -34,6 +38,14 @@ static inline struct resource *request_sdram_region(const char *name,
 	return __request_sdram_region(name, 0, start, size);
 }
 
+/* use for secure firmware to inhibit speculation */
+static inline struct resource *reserve_sdram_region(const char *name,
+						    resource_size_t start,
+						    resource_size_t size)
+{
+	return __request_sdram_region(name, IORESOURCE_BUSY, start, size);
+}
+
 int release_sdram_region(struct resource *res);
 
 void memory_bank_find_space(struct memory_bank *bank, resource_size_t *retstart,
-- 
2.30.2

[PATCH v2 04/10] init: define new postmem_initcall()

[Ahmad Fatoum]

Memory banks are added in mem_initcall() and are used in mmu_initcall()
which directly follows it to set caching attributes for the banks.

Code that requires memory banks to be registered, thus has to use
mmu_initcall(), but this is not possible for code that reliably needs to
run before MMU init: We need to give board code and device tree parsing
code the chance to reserve_sdram_region parts of SDRAM that contain
secure firmware to avoid speculative execution into them once the MMU is
turned on. For this reason, define a new postmem_initcall() level and
already use it for add_mem_devices, which has nothing to do with
mmu_initcall. Another user that can't be mmu_initcall() will follow in a
later commit.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/memory.c                   |  2 +-
 include/asm-generic/barebox.lds.h |  1 +
 include/init.h                    | 21 +++++++++++----------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/common/memory.c b/common/memory.c
index 347f83fd4cf8..40c795d2cde1 100644
--- a/common/memory.c
+++ b/common/memory.c
@@ -195,7 +195,7 @@ static int add_mem_devices(void)
 
 	return 0;
 }
-mmu_initcall(add_mem_devices);
+postmem_initcall(add_mem_devices);
 
 /*
  * Request a region from the registered sdram
diff --git a/include/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
index a22e251c9d64..48c10b173852 100644
--- a/include/asm-generic/barebox.lds.h
+++ b/include/asm-generic/barebox.lds.h
@@ -35,6 +35,7 @@
 	KEEP(*(.initcall.13))			\
 	KEEP(*(.initcall.14))			\
 	KEEP(*(.initcall.15))			\
+	KEEP(*(.initcall.16))			\
 	__barebox_initcalls_end = .;
 
 #define BAREBOX_EXITCALLS			\
diff --git a/include/init.h b/include/init.h
index c695f99867ff..d0343fdf05cc 100644
--- a/include/init.h
+++ b/include/init.h
@@ -58,16 +58,17 @@ typedef void (*exitcall_t)(void);
 #define console_initcall(fn)		__define_initcall("3",fn,3)
 #define postconsole_initcall(fn)	__define_initcall("4",fn,4)
 #define mem_initcall(fn)		__define_initcall("5",fn,5)
-#define mmu_initcall(fn)		__define_initcall("6",fn,6)
-#define postmmu_initcall(fn)		__define_initcall("7",fn,7)
-#define coredevice_initcall(fn)		__define_initcall("8",fn,8)
-#define fs_initcall(fn)			__define_initcall("9",fn,9)
-#define device_initcall(fn)		__define_initcall("10",fn,10)
-#define crypto_initcall(fn)		__define_initcall("11",fn,11)
-#define of_populate_initcall(fn)	__define_initcall("12",fn,12)
-#define late_initcall(fn)		__define_initcall("13",fn,13)
-#define environment_initcall(fn)	__define_initcall("14",fn,14)
-#define postenvironment_initcall(fn)	__define_initcall("15",fn,15)
+#define postmem_initcall(fn)		__define_initcall("6",fn,6)
+#define mmu_initcall(fn)		__define_initcall("7",fn,7)
+#define postmmu_initcall(fn)		__define_initcall("8",fn,8)
+#define coredevice_initcall(fn)		__define_initcall("9",fn,9)
+#define fs_initcall(fn)			__define_initcall("10",fn,10)
+#define device_initcall(fn)		__define_initcall("11",fn,11)
+#define crypto_initcall(fn)		__define_initcall("12",fn,12)
+#define of_populate_initcall(fn)	__define_initcall("13",fn,13)
+#define late_initcall(fn)		__define_initcall("14",fn,14)
+#define environment_initcall(fn)	__define_initcall("15",fn,15)
+#define postenvironment_initcall(fn)	__define_initcall("16",fn,16)
 
 #define early_exitcall(fn)		__define_exitcall("0",fn,0)
 #define predevshutdown_exitcall(fn)	__define_exitcall("1",fn,1)
-- 
2.30.2

[PATCH v2 05/10] of: reserved-mem: reserve regions prior to mmu_initcall()

[Ahmad Fatoum]

Now that we both have a way to mark SDRAM regions requested as reserved
and an postmem_initcall() to do this add, change device tree memory
reservation parsing code to use them instead of requesting them as
normal memory at coredevice_initcall() level. This allows us to
reuse this information for MMU setup in the follow-up commit.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/memory.c           | 15 +++------------
 drivers/of/reserved-mem.c | 34 +++++++++++++++++++++++-----------
 include/of.h              |  7 -------
 3 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/common/memory.c b/common/memory.c
index 40c795d2cde1..7e24ecb2bd03 100644
--- a/common/memory.c
+++ b/common/memory.c
@@ -56,17 +56,6 @@ void mem_malloc_init(void *start, void *end)
 	mem_malloc_initialized = 1;
 }
 
-static int request_reservation(const struct resource *res)
-{
-	if (!(res->flags & IORESOURCE_EXCLUSIVE))
-		return 0;
-
-	pr_debug("region %s %pa-%pa\n", res->name, &res->start, &res->end);
-
-	request_sdram_region(res->name, res->start, resource_size(res));
-	return 0;
-}
-
 static int mem_malloc_resource(void)
 {
 #if !defined __SANDBOX__
@@ -96,7 +85,7 @@ static int mem_malloc_resource(void)
 	request_sdram_region("stack", STACK_BASE, STACK_SIZE);
 #endif
 
-	return of_reserved_mem_walk(request_reservation);
+	return 0;
 }
 coredevice_initcall(mem_malloc_resource);
 
@@ -173,6 +162,8 @@ int barebox_add_memory_bank(const char *name, resource_size_t start,
 	if (IS_ERR(res))
 		return PTR_ERR(res);
 
+	res->flags = IORESOURCE_MEM;
+
 	bank = xzalloc(sizeof(*bank));
 
 	bank->res = res;
diff --git a/drivers/of/reserved-mem.c b/drivers/of/reserved-mem.c
index 34e61dfea343..f50a0bd8374d 100644
--- a/drivers/of/reserved-mem.c
+++ b/drivers/of/reserved-mem.c
@@ -4,16 +4,31 @@
 #include <stdio.h>
 #include <of.h>
 #include <of_address.h>
+#include <memory.h>
+#include <linux/ioport.h>
 
 #define MEMRESERVE_NCELLS	2
-#define MEMRESERVE_FLAGS	(IORESOURCE_MEM | IORESOURCE_EXCLUSIVE)
 
-int of_reserved_mem_walk(int (*handler)(const struct resource *res))
+static void request_region(struct resource *r)
+{
+	struct memory_bank *bank;
+
+	for_each_memory_bank(bank) {
+		if (!resource_contains(bank->res, r))
+			continue;
+
+		if (!reserve_sdram_region(r->name, r->start, resource_size(r)))
+			pr_warn("couldn't request reserved sdram region %pa-%pa\n",
+				&r->start, &r->end);
+		break;
+	}
+}
+
+static int of_reserved_mem_walk(void)
 {
 	struct device_node *node, *child;
 	int ncells = 0;
 	const __be32 *reg;
-	int ret;
 
 	node = of_find_node_by_path("/reserved-memory");
 	if (node) {
@@ -27,11 +42,9 @@ int of_reserved_mem_walk(int (*handler)(const struct resource *res))
 			of_address_to_resource(child, 0, &resource);
 
 			resource.name = child->name;
-			resource.flags = MEMRESERVE_FLAGS;
+			resource.flags = IORESOURCE_MEM;
 
-			ret = handler(&resource);
-			if (ret)
-				return ret;
+			request_region(&resource);
 		}
 	}
 
@@ -48,7 +61,7 @@ int of_reserved_mem_walk(int (*handler)(const struct resource *res))
 
 			snprintf(name, sizeof(name), "fdt-memreserve-%u", n++);
 			resource.name = name;
-			resource.flags = MEMRESERVE_FLAGS;
+			resource.flags = IORESOURCE_MEM;
 
 			resource.start = of_read_number(reg + i, MEMRESERVE_NCELLS);
 			i += MEMRESERVE_NCELLS;
@@ -61,11 +74,10 @@ int of_reserved_mem_walk(int (*handler)(const struct resource *res))
 
 			resource.end = resource.start + size - 1;
 
-			ret = handler(&resource);
-			if (ret)
-				return ret;
+			request_region(&resource);
 		}
 	}
 
 	return 0;
 }
+postmem_initcall(of_reserved_mem_walk);
diff --git a/include/of.h b/include/of.h
index 37320da5ec76..995e9b591399 100644
--- a/include/of.h
+++ b/include/of.h
@@ -328,8 +328,6 @@ int of_autoenable_device_by_path(char *path);
 int of_autoenable_i2c_by_component(char *path);
 int of_prepend_machine_compatible(struct device_node *root, const char *compat);
 
-int of_reserved_mem_walk(int (*handler)(const struct resource *res));
-
 #else
 static inline struct of_reserve_map *of_get_reserve_map(void)
 {
@@ -877,11 +875,6 @@ static inline int of_prepend_machine_compatible(struct device_node *root,
 	return -ENODEV;
 }
 
-static inline int of_reserved_mem_walk(int (*handler)(const struct resource *res))
-{
-	return 0;
-}
-
 #endif
 
 #define for_each_property_of_node(dn, pp) \
-- 
2.30.2

[PATCH v2 06/10] ARM: mmu64: map reserved regions uncached

[Ahmad Fatoum]

Now that we have reserved memory regions specially marked in the SDRAM
bank requests, we can use this information to map these regions
uncached and eXecute Never to avoid speculative access into these
regions from causing hard-to-debug data aborts.

The sequence used here is safe because __mmu_init turns off the MMU if
enabled, so even if we overwrite early MMU uncached entries they are
without effect until the MMU is turned on again, by which time the
for_each_reserved_region should have disabled caching for them again
(e.g. because they were listed in DT as /reserve-memory).

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/cpu/mmu-common.h | 15 +++++++++++++++
 arch/arm/cpu/mmu_64.c     | 10 +++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/mmu-common.h b/arch/arm/cpu/mmu-common.h
index ed7d5bc3168f..c9ea2c122857 100644
--- a/arch/arm/cpu/mmu-common.h
+++ b/arch/arm/cpu/mmu-common.h
@@ -3,6 +3,11 @@
 #ifndef __ARM_MMU_COMMON_H
 #define __ARM_MMU_COMMON_H
 
+#include <linux/types.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/sizes.h>
+
 void dma_inv_range(void *ptr, size_t size);
 void dma_flush_range(void *ptr, size_t size);
 void *dma_alloc_map(size_t size, dma_addr_t *dma_handle, unsigned flags);
@@ -19,4 +24,14 @@ static inline void arm_mmu_not_initialized_error(void)
 	panic("MMU not initialized\n");
 }
 
+static inline size_t resource_first_page(const struct resource *res)
+{
+	return ALIGN_DOWN(res->start, SZ_4K);
+}
+
+static inline size_t resource_count_pages(const struct resource *res)
+{
+	return ALIGN(resource_size(res), SZ_4K);
+}
+
 #endif
diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c
index 06049e000375..f43ac9a12186 100644
--- a/arch/arm/cpu/mmu_64.c
+++ b/arch/arm/cpu/mmu_64.c
@@ -201,9 +201,17 @@ void __mmu_init(bool mmu_on)
 	create_sections(0, 0, 1UL << (BITS_PER_VA - 1), attrs_uncached_mem());
 
 	/* Map sdram cached. */
-	for_each_memory_bank(bank)
+	for_each_memory_bank(bank) {
+		struct resource *rsv;
+
 		create_sections(bank->start, bank->start, bank->size, CACHED_MEM);
 
+		for_each_reserved_region(bank, rsv) {
+			create_sections(resource_first_page(rsv), resource_first_page(rsv),
+					resource_count_pages(rsv), attrs_uncached_mem());
+		}
+	}
+
 	/* Make zero page faulting to catch NULL pointer derefs */
 	zero_page_faulting();
 
-- 
2.30.2

[PATCH v2 07/10] ARM: mmu: define attrs_uncached_mem() helper

[Ahmad Fatoum]

We already have a helper with the same name for ARMv8, so define it here
for reuse in the follow-up commit.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/cpu/mmu.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/cpu/mmu.h b/arch/arm/cpu/mmu.h
index d48522d166f7..1499b70dd649 100644
--- a/arch/arm/cpu/mmu.h
+++ b/arch/arm/cpu/mmu.h
@@ -73,15 +73,20 @@ create_sections(uint32_t *ttb, unsigned long first,
 #define PMD_SECT_DEF_UNCACHED (PMD_SECT_AP_WRITE | PMD_SECT_AP_READ | PMD_TYPE_SECT)
 #define PMD_SECT_DEF_CACHED (PMD_SECT_WB | PMD_SECT_DEF_UNCACHED)
 
-static inline void create_flat_mapping(uint32_t *ttb)
+static inline unsigned long attrs_uncached_mem(void)
 {
 	unsigned int flags = PMD_SECT_DEF_UNCACHED;
 
 	if (cpu_architecture() >= CPU_ARCH_ARMv7)
 		flags |= PMD_SECT_XN;
 
+	return flags;
+}
+
+static inline void create_flat_mapping(uint32_t *ttb)
+{
 	/* create a flat mapping using 1MiB sections */
-	create_sections(ttb, 0, 0xffffffff, flags);
+	create_sections(ttb, 0, 0xffffffff, attrs_uncached_mem());
 }
 
 #endif /* __ARM_MMU_H */
-- 
2.30.2

[PATCH v2 08/10] ARM: mmu: use reserve mem entries to modify maps

[Ahmad Fatoum]

From: Rouven Czerwinski <r.czerwinski@pengutronix.de>

Like done for ARM64, use the reserved memory regions marked in the SDRAM
bank to map these regions uncached and eXecute Never to avoid speculative
access into these regions from causing hard-to-debug data aborts.

Unlike with mmu_64, for CONFIG_MMU_EARLY systems, the MMU isn't turned
off before changing the page table. So what we do instead is allocating
a 16K shadow buffer and modifying that and afterwards overwriting the
active TTB with it.

We could instead use set_ttbr() to move the page table, but in interest
of minimizing chance of breaking older ARM platforms, we keep the online
changing of the entries for now.

Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
[afa: use SDRAM regions instead of FDT reserve entries]
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/cpu/mmu.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/arch/arm/cpu/mmu.c b/arch/arm/cpu/mmu.c
index 6388e1bf14f6..f9c629c0f19d 100644
--- a/arch/arm/cpu/mmu.c
+++ b/arch/arm/cpu/mmu.c
@@ -413,6 +413,7 @@ static void vectors_init(void)
 void __mmu_init(bool mmu_on)
 {
 	struct memory_bank *bank;
+	void *oldttb = NULL;
 
 	arm_set_cache_functions();
 
@@ -430,15 +431,17 @@ void __mmu_init(bool mmu_on)
 		pte_flags_uncached = PTE_FLAGS_UNCACHED_V4;
 	}
 
+	ttb = xmemalign(ARM_TTB_SIZE, ARM_TTB_SIZE);
+
+	/*
+	 * Early MMU code may have already enabled the MMU. We assume a
+	 * flat 1:1 section mapping in this case.
+	 */
 	if (mmu_on) {
-		/*
-		 * Early MMU code has already enabled the MMU. We assume a
-		 * flat 1:1 section mapping in this case.
-		 */
-		/* Clear unpredictable bits [13:0] */
-		ttb = (uint32_t *)(get_ttbr() & ~0x3fff);
+		oldttb = (uint32_t *)(get_ttbr() & ~0x3fff);
+		memcpy(ttb, oldttb, ARM_TTB_SIZE);
 
-		if (!request_sdram_region("ttb", (unsigned long)ttb, SZ_16K))
+		if (!request_sdram_region("ttb", (unsigned long)oldttb, SZ_16K))
 			/*
 			 * This can mean that:
 			 * - the early MMU code has put the ttb into a place
@@ -447,10 +450,8 @@ void __mmu_init(bool mmu_on)
 			 *   the ttb will get corrupted.
 			 */
 			pr_crit("Critical Error: Can't request SDRAM region for ttb at %p\n",
-					ttb);
+				oldttb);
 	} else {
-		ttb = xmemalign(ARM_TTB_SIZE, ARM_TTB_SIZE);
-
 		set_ttbr(ttb);
 
 		/* For the XN bit to take effect, we can't be using DOMAIN_MANAGER. */
@@ -468,11 +469,28 @@ void __mmu_init(bool mmu_on)
 	vectors_init();
 
 	for_each_memory_bank(bank) {
+		struct resource *rsv;
+
 		create_sections(ttb, bank->start, bank->start + bank->size - 1,
 				PMD_SECT_DEF_CACHED);
-		__mmu_cache_flush();
+
+		for_each_reserved_region(bank, rsv) {
+			create_sections(ttb, resource_first_page(rsv),
+					resource_count_pages(rsv),
+					attrs_uncached_mem());
+		}
 	}
 
+	/*
+	 * We could set_ttbr(ttb) here instead and save on the copy, but
+	 * for now we play it safe, so we don't mess with the older ARMs.
+	 */
+	if (oldttb) {
+		memcpy(oldttb, ttb, ARM_TTB_SIZE);
+		free(ttb);
+	}
+
+	__mmu_cache_flush();
 	__mmu_cache_on();
 }
 
-- 
2.30.2

Re: [PATCH v2 08/10] ARM: mmu: use reserve mem entries to modify maps

[Sascha Hauer]

This patch breaks NAND support on my Phytec i.MX6 board. There are some
problems with this patch, so I ended up reverting it for now.

On Wed, Aug 17, 2022 at 01:42:42PM +0200, Ahmad Fatoum wrote:
> @@ -468,11 +469,28 @@ void __mmu_init(bool mmu_on)
>  	vectors_init();
>  
>  	for_each_memory_bank(bank) {
> +		struct resource *rsv;
> +
>  		create_sections(ttb, bank->start, bank->start + bank->size - 1,
>  				PMD_SECT_DEF_CACHED);
> -		__mmu_cache_flush();
> +
> +		for_each_reserved_region(bank, rsv) {
> +			create_sections(ttb, resource_first_page(rsv),
> +					resource_count_pages(rsv),
> +					attrs_uncached_mem());
> +		}

This operates on 1MiB sections, so everything requiring a finer
granularity will fail here. Not sure if we currently need that, but not
even issuing a warning is not good.

>  	}
>  
> +	/*
> +	 * We could set_ttbr(ttb) here instead and save on the copy, but
> +	 * for now we play it safe, so we don't mess with the older ARMs.
> +	 */
> +	if (oldttb) {
> +		memcpy(oldttb, ttb, ARM_TTB_SIZE);
> +		free(ttb);
> +	}

in the early MMU case the MMU still uses 'oldttb' as ttb whereas 'ttb'
now points to invalid memory. Still functions like arm_create_pte()
still operate on 'ttb'. It looks like a ttb = oldttb is missing here.

Also I wonder when we have to map the reserved regions as execute never,
then the early MMU setup seems broken as well, as that creates a flat
mapping without honoring the reserved regions. Shouldn't that be fixed?

Sascha

-- 
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 |

Re: [PATCH v2 08/10] ARM: mmu: use reserve mem entries to modify maps

[Ahmad Fatoum]

Hi,

On 12.09.22 14:01, Sascha Hauer wrote:
> This patch breaks NAND support on my Phytec i.MX6 board. There are some
> problems with this patch, so I ended up reverting it for now.

I wonder why. I see no memory reserves in imx6q-phytec-phycore-som-nand.dts
and the files it includes.

> 
> On Wed, Aug 17, 2022 at 01:42:42PM +0200, Ahmad Fatoum wrote:
>> @@ -468,11 +469,28 @@ void __mmu_init(bool mmu_on)
>>  	vectors_init();
>>  
>>  	for_each_memory_bank(bank) {
>> +		struct resource *rsv;
>> +
>>  		create_sections(ttb, bank->start, bank->start + bank->size - 1,
>>  				PMD_SECT_DEF_CACHED);
>> -		__mmu_cache_flush();
>> +
>> +		for_each_reserved_region(bank, rsv) {
>> +			create_sections(ttb, resource_first_page(rsv),
>> +					resource_count_pages(rsv),
>> +					attrs_uncached_mem());
>> +		}
> 
> This operates on 1MiB sections, so everything requiring a finer
> granularity will fail here. Not sure if we currently need that, but not
> even issuing a warning is not good.

At worst it'd needlessly mark some memory uncached/XN. If users are
affected, they will notice and we can revisit this. I could add a debug
print here.

I need to rework this though, because I see now create_sections differs
between ARM64 and ARM32. On ARM64, it accepts the last address as argument,
while on ARM64, it's the size.. resource_count_pages() is not a nice
name either, because it returns bytes (aligned up to PAGE_SIZE).

> 
>>  	}
>>  
>> +	/*
>> +	 * We could set_ttbr(ttb) here instead and save on the copy, but
>> +	 * for now we play it safe, so we don't mess with the older ARMs.
>> +	 */
>> +	if (oldttb) {
>> +		memcpy(oldttb, ttb, ARM_TTB_SIZE);
>> +		free(ttb);
>> +	}
> 
> in the early MMU case the MMU still uses 'oldttb' as ttb whereas 'ttb'
> now points to invalid memory. Still functions like arm_create_pte()
> still operate on 'ttb'. It looks like a ttb = oldttb is missing here.

Why would ttb point at invalid memory? It's allocated unconditionally
with xmemalign and freed here.

> Also I wonder when we have to map the reserved regions as execute never,
> then the early MMU setup seems broken as well, as that creates a flat
> mapping without honoring the reserved regions. Shouldn't that be fixed?

Yes, see excerpt from cover letter:

"Note that this doesn't yet solve all problems. For example, PPA secure
 monitor installation on Layerscape may happen with CONFIG_MMU_EARLY=y,
 in which case barebox in EL2 may speculate into the secure memory
 before any device tree reserved-memory settings are considered. For this
 reason, both early MMU and normal MMU setup must be aware of the
 reserved memory regions. The original patch set by Rouven used FDT
 parsing in PBL to achieve this, but this is omitted here to limit
 scope of the patch series. Instead we only handle the CONFIG_OPTEE_SIZE
 case out-of-the-box."

My use case is the CONFIG_OPTEE_SIZE one and at least for ARM64, that looks
resolved now IMO.

Cheers,
Ahmad

> 
> Sascha
> 


-- 
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 |

Re: [PATCH v2 08/10] ARM: mmu: use reserve mem entries to modify maps

[Sascha Hauer]

On Mon, Sep 12, 2022 at 05:15:45PM +0200, Ahmad Fatoum wrote:
> Hi,
> 
> On 12.09.22 14:01, Sascha Hauer wrote:
> > This patch breaks NAND support on my Phytec i.MX6 board. There are some
> > problems with this patch, so I ended up reverting it for now.
> 
> I wonder why. I see no memory reserves in imx6q-phytec-phycore-som-nand.dts
> and the files it includes.
> 
> > 
> > On Wed, Aug 17, 2022 at 01:42:42PM +0200, Ahmad Fatoum wrote:
> >> @@ -468,11 +469,28 @@ void __mmu_init(bool mmu_on)
> >>  	vectors_init();
> >>  
> >>  	for_each_memory_bank(bank) {
> >> +		struct resource *rsv;
> >> +
> >>  		create_sections(ttb, bank->start, bank->start + bank->size - 1,
> >>  				PMD_SECT_DEF_CACHED);
> >> -		__mmu_cache_flush();
> >> +
> >> +		for_each_reserved_region(bank, rsv) {
> >> +			create_sections(ttb, resource_first_page(rsv),
> >> +					resource_count_pages(rsv),
> >> +					attrs_uncached_mem());
> >> +		}
> > 
> > This operates on 1MiB sections, so everything requiring a finer
> > granularity will fail here. Not sure if we currently need that, but not
> > even issuing a warning is not good.
> 
> At worst it'd needlessly mark some memory uncached/XN. If users are
> affected, they will notice and we can revisit this. I could add a debug
> print here.
> 
> I need to rework this though, because I see now create_sections differs
> between ARM64 and ARM32. On ARM64, it accepts the last address as argument,
> while on ARM64, it's the size.. resource_count_pages() is not a nice
> name either, because it returns bytes (aligned up to PAGE_SIZE).
> 
> > 
> >>  	}
> >>  
> >> +	/*
> >> +	 * We could set_ttbr(ttb) here instead and save on the copy, but
> >> +	 * for now we play it safe, so we don't mess with the older ARMs.
> >> +	 */
> >> +	if (oldttb) {
> >> +		memcpy(oldttb, ttb, ARM_TTB_SIZE);
> >> +		free(ttb);
> >> +	}
> > 
> > in the early MMU case the MMU still uses 'oldttb' as ttb whereas 'ttb'
> > now points to invalid memory. Still functions like arm_create_pte()
> > still operate on 'ttb'. It looks like a ttb = oldttb is missing here.
> 
> Why would ttb point at invalid memory? It's allocated unconditionally
> with xmemalign and freed here.

It becomes clearer when you look at the scope of the variable.

> 
> > Also I wonder when we have to map the reserved regions as execute never,
> > then the early MMU setup seems broken as well, as that creates a flat
> > mapping without honoring the reserved regions. Shouldn't that be fixed?
> 
> Yes, see excerpt from cover letter:
> 
> "Note that this doesn't yet solve all problems. For example, PPA secure
>  monitor installation on Layerscape may happen with CONFIG_MMU_EARLY=y,
>  in which case barebox in EL2 may speculate into the secure memory
>  before any device tree reserved-memory settings are considered. For this
>  reason, both early MMU and normal MMU setup must be aware of the
>  reserved memory regions. The original patch set by Rouven used FDT
>  parsing in PBL to achieve this, but this is omitted here to limit
>  scope of the patch series. Instead we only handle the CONFIG_OPTEE_SIZE
>  case out-of-the-box."

Ok.

Sascha

-- 
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 |

[PATCH v2 09/10] ARM: early-mmu: don't cache/prefetch OPTEE_SIZE bytes from end of memory

[Ahmad Fatoum]

OPTEE_SIZE is always defined in <asm-generic/memory_layout.h> either as
as CONFIG_OPTEE_SIZE or as 0 if the option is undefined.

There is never a reason to map the last OPTEE_SIZE bytes in the initial
memory as cached and executable:

 - OPTEE_SIZE == 0: no change
 - OPTEE_SIZE != 0 && CONFIG_PBL_OPTEE=y: cache must not be enabled for
   the region to avoid speculation
 - OPTEE_SIZE != 0 && CONFIG_BOOTM_OPTEE=y: we won't use this region
   for anything between MMU early init and normal init, so no harm
   in disabling caches.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/cpu/start.c      | 2 +-
 arch/arm/cpu/uncompress.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/cpu/start.c b/arch/arm/cpu/start.c
index 5861c15d43df..672f26e0063c 100644
--- a/arch/arm/cpu/start.c
+++ b/arch/arm/cpu/start.c
@@ -171,7 +171,7 @@ __noreturn __no_sanitize_address void barebox_non_pbl_start(unsigned long membas
 		} else {
 			pr_debug("enabling MMU, ttb @ 0x%08lx\n", ttb);
 			arm_early_mmu_cache_invalidate();
-			mmu_early_enable(membase, memsize, ttb);
+			mmu_early_enable(membase, memsize - OPTEE_SIZE, ttb);
 		}
 	}
 
diff --git a/arch/arm/cpu/uncompress.c b/arch/arm/cpu/uncompress.c
index 2250b8ccd375..537ee63229d7 100644
--- a/arch/arm/cpu/uncompress.c
+++ b/arch/arm/cpu/uncompress.c
@@ -84,7 +84,7 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
 	if (IS_ENABLED(CONFIG_MMU_EARLY)) {
 		unsigned long ttb = arm_mem_ttb(membase, endmem);
 		pr_debug("enabling MMU, ttb @ 0x%08lx\n", ttb);
-		mmu_early_enable(membase, memsize, ttb);
+		mmu_early_enable(membase, memsize - OPTEE_SIZE, ttb);
 	}
 
 	free_mem_ptr = arm_mem_early_malloc(membase, endmem);
-- 
2.30.2

[PATCH v2 10/10] commands: iomem: point out [R]eserved regions

[Ahmad Fatoum]

Now that we define IORESOURCE_BUSY as meaning that a region possibly
contains secure firmware, it's useful to have this information in the
iomem output as well, so add it.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/iomemport.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/commands/iomemport.c b/commands/iomemport.c
index d0cfc413c262..f2baa0e29397 100644
--- a/commands/iomemport.c
+++ b/commands/iomemport.c
@@ -16,11 +16,14 @@ static void __print_resources(struct resource *res, int indent)
 	for (i = 0; i < indent; i++)
 		printf("  ");
 
-	printf("%pa - %pa (size %pa) %s\n",
-			&res->start, &res->end, &size, res->name);
+	printf("%pa - %pa (size %pa) %s%s\n",
+			&res->start, &res->end, &size,
+			res->flags & IORESOURCE_BUSY ? "[R] " : "",
+			res->name);
 
-	list_for_each_entry(r, &res->children, sibling)
+	list_for_each_entry(r, &res->children, sibling) {
 		__print_resources(r, indent + 1);
+	}
 }
 
 static void print_resources(struct resource *res)
-- 
2.30.2

Re: [PATCH v2 00/10] ARM: mmu: inhibit speculation into secure memory

[Sascha Hauer]

On Wed, Aug 17, 2022 at 01:42:34PM +0200, Ahmad Fatoum wrote:
> v1 -> v2:
>   - fold misplaced hunk changing %u added in [01/10] into 0x%x in
>     [02/10] directly into [01/10] (Ulrich)
>   - Correct typo in commit message (Sascha)
> 
> When setting up page tables, barebox marks all the address space as
> eXecute Never and uncached, except for the memory banks. If we happen to
> have secure memory, this is andequate as speculative execution may read
> from secure memory or even attempt to execute it leading to spurious
> data aborts. The way around this so far was either having OP-TEE in SRAM
> (which normally isn't a barebox memory bank) or having it at the end of
> DRAM, but adjusting size, so it's not covered by a memory bank.
> 
> This adds a generic solution to the issue. We already request the SDRAM
> regions described by the reserved memory entries in the DT. We go a step
> further and mark them as IORESOURCE_BUSY, which we can then evaluat in
> the MMU setup code to map these regions uncached and eXecute Never.
> 
> There has been previous attempts by Rouven to achieve this, the latest
> being:
> 
>   https://lore.barebox.org/barebox/20210803094418.475609-1-r.czerwinski@pengutronix.de/
> 
> While this series tries to achieve the same end goal, it goes about it
> in a different manner: We don't use FDT fixup table to tell us what to
> nstead have both the FDT fixup table and the /reserved-memory child
> nodes feed into the barebox request_sdram_region allocator and then
> use to apply caching attributes.
> 
> Note that this doesn't yet solve all problems. For example, PPA secure
> monitor installation on Layerscape may happen with CONFIG_MMU_EARLY=y,
> in which case barebox in EL2 may speculate into the secure memory
> before any device tree reserved-memory settings are considered. For this
> reason, both early MMU and normal MMU setup must be aware of the
> reserved memory regions. The original patch set by Rouven used FDT
> parsing in PBL to achieve this, but this is omitted here to limit
> scope of the patch series. Instead we only handle the CONFIG_OPTEE_SIZE
> case out-of-the-box.
> 
> Ahmad Fatoum (9):
>   resource: add flags parameter to __request_region
>   common: allow requesting SDRAM regions with custom flags
>   memory: define reserve_sdram_region helper
>   init: define new postmem_initcall()
>   of: reserved-mem: reserve regions prior to mmu_initcall()
>   ARM: mmu64: map reserved regions uncached
>   ARM: mmu: define attrs_uncached_mem() helper
>   ARM: early-mmu: don't cache/prefetch OPTEE_SIZE bytes from end of
>     memory
>   commands: iomem: point out [R]eserved regions
> 
> Rouven Czerwinski (1):
>   ARM: mmu: use reserve mem entries to modify maps

Applied, thanks

Sascha


-- 
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 |