omap: Invalidate first page to avoid speculation

Hello u-boot list,

Here is a "request for comments" on the best way to solve a little "speculation" issue on recent OMAPs. Any guidance/feedback on the way to go would be greatly appreciated, please.

I am using u-boot on an OMAP5 HS device (with security, that is), and I am experiencing "security violations" due to speculative accesses done by the Cortex-A15 processor to the region near address zero. This region is a secure region, where non-secure accesses are forbidden and reported by the security firmware on an OMAP HS device. On an OMAP GP device, those accesses may very well exist, but are silently ignored by the firmware. Note that the speculative accesses are not actual functional accesses, so their being aborted does not harm the functionality of u-boot as it is.
A quick (and dirty) solution is to mark the region near address zero as being
invalid, which prevents the processor from doing speculative accesses there (see patch).
This patch as it is has a number of issues: it impacts all ARM devices and it
unmaps too large a region. I am not sure how to cleanly rework the patch so that it would be made OMAP-only cleanly. Also, unmapping a smaller region to better fit the hardware characteristics would require using second level descriptors, and I do not know if this is recommended. To make this worse, chips in the OMAP family have differences in their secure rom boundaries.

Does the u-boot community feels this issue needs to be addressed? What would be the best way to solve this?

Best regards,

V.

---
 arch/arm/lib/cache-cp15.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 939de10..57e1974 100644
@@ -72,8 +72,13 @@ static inline void mmu_setup(void)
 	u32 reg;
 
 	arm_init_before_mmu();
+
+	/* First page (starting at 0x0) is made invalid to avoid
+	 * speculative accesses in secure rom. */
+	page_table[0] = 0;
+
 	/* Set up an identity-mapping for all 4GB, rw for everyone */
-	for (i = 0; i < 4096; i++)
+	for (i = 1; i < 4096; i++)
 		page_table[i] = i << 20 | (3 << 10) | 0x12;
 
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {

1.7.9.5

Hi Vincent,

Albert.

Albert Aribaud:

Hi Albert,

Thank you very much for your comments.

Here is "v2" of the patch, reworked to follow your advice. Comments are welcome. I think it is now cleaner to split it in two patches:

[PATCH 1/2] ARM: cache: introduce weak arm_setup_identity_mapping [PATCH 2/2] ARM: OMAP5: redefine arm_setup_identity_mapping

I picked the 'arm_setup_identity_mapping' name more or less arbitrarily, please advise if not.

I verified this patch series on an OMAP5 HS device (with security) and on a GP device (without security), and both work for me.

Best regards,

V.

Separate the MMU identity mapping for ARM in a weak function, to allow redefinition with platform specific function.

This is motivated by the need to unmap the region near address zero on HS OMAP devices, to avoid speculative accesses. Accessing this region causes security violations, which we want to avoid.

---
 arch/arm/lib/cache-cp15.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

index 939de10..886fe5c 100644
@@ -40,6 +40,17 @@ void __arm_init_before_mmu(void)
 void arm_init_before_mmu(void)
 	__attribute__((weak, alias("__arm_init_before_mmu")));
 
+void __arm_setup_identity_mapping(u32 *page_table)
+{
+	int i;
+
+	/* Set up an identity-mapping for all 4GB, rw for everyone */
+	for (i = 0; i < 4096; i++)
+		page_table[i] = i << 20 | (3 << 10) | 0x12;
+}
+void arm_setup_identity_mapping(u32 *page_table)
+	__attribute__((weak, alias("__arm_setup_identity_mapping")));
+
 static void cp_delay (void)
 {
 	volatile int i;

@@ -72,9 +83,10 @@ static inline void mmu_setup(void)
 	u32 reg;
 
 	arm_init_before_mmu();
-	/* Set up an identity-mapping for all 4GB, rw for everyone */
-	for (i = 0; i < 4096; i++)
-		page_table[i] = i << 20 | (3 << 10) | 0x12;
+
+	/* Set up an identity-mapping. Default version maps all 4GB rw for
+	 * everyone */
+	arm_setup_identity_mapping(page_table);
 
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
 		dram_bank_mmu_setup(i);

1.7.9.5

We introduce an OMAP5 specific version of arm_setup_identity_mapping(), which makes the first page of the identity mapping invalid.

We want to unmap the region near address zero on HS OMAP devices, to avoid speculative accesses. Accessing this region causes security violations, which we want to avoid.

---
 arch/arm/cpu/armv7/omap5/Makefile     |    1 +
 arch/arm/cpu/armv7/omap5/cache-cp15.c |   40 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/omap5/cache-cp15.c

index 9b261c4..49c454c 100644
@@ -29,6 +29,7 @@ COBJS	+= hwinit.o
 COBJS	+= clocks.o
 COBJS	+= emif.o
 COBJS	+= sdram.o
+COBJS	+= cache-cp15.o
 
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))

new file mode 100644
index 0000000..506cc00
@@ -0,0 +1,40 @@
+/*
+ * (C) Copyright 2002
+ * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
+ *
+ * (C) Copyright 2012
+ * Vincent Stehl?, Texas Instruments, v-stehle at ti.com.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+
+/* OMAP5 specific function to set up the identity mapping. */
+void arm_setup_identity_mapping(u32 *page_table)
+{
+	/* Perform default mapping, which sets up an identity-mapping for all
+	 * 4GB, rw for everyone */
+	__arm_setup_identity_mapping();
+
+	/* First page (starting at 0x0) is made invalid to avoid speculative
+	 * accesses in secure rom. TODO: use second level descriptors for finer
+	 * grained mapping. */
+	page_table[0] = 0;
+}

1.7.9.5

[snip]

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121119/c80f8eba/attachment.pgp>

Tom Rini:

Hi Tom,

Thank you very much for your review.

Here is v3 of the patch series with reworks for __weak and comments style:

[PATCH 1/2] ARM: cache: introduce weak arm_setup_identity_mapping [PATCH 2/2] ARM: OMAP5: redefine arm_setup_identity_mapping

This boots ok on OMAP5 HS device.

Best regards,

V.

Separate the MMU identity mapping for ARM in a weak function, to allow redefinition with platform specific function.

This is motivated by the need to unmap the region near address zero on HS OMAP devices, to avoid speculative accesses. Accessing this region causes security violations, which we want to avoid.

---
 arch/arm/lib/cache-cp15.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

index 939de10..0b87d93 100644
@@ -23,6 +23,7 @@
 
 #include <common.h>
 #include <asm/system.h>
+#include <linux/compiler.h>
 
 #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
 

@@ -40,6 +41,17 @@ void __arm_init_before_mmu(void)
 void arm_init_before_mmu(void)
 	__attribute__((weak, alias("__arm_init_before_mmu")));
 
+void __arm_setup_identity_mapping(u32 *page_table)
+{
+	int i;
+
+	/* Set up an identity-mapping for all 4GB, rw for everyone */
+	for (i = 0; i < 4096; i++)
+		page_table[i] = i << 20 | (3 << 10) | 0x12;
+}
+__weak void arm_setup_identity_mapping(u32 *page_table)
+	__attribute__((alias("__arm_setup_identity_mapping")));
+
 static void cp_delay (void)
 {
 	volatile int i;

@@ -72,9 +84,10 @@ static inline void mmu_setup(void)
 	u32 reg;
 
 	arm_init_before_mmu();
-	/* Set up an identity-mapping for all 4GB, rw for everyone */
-	for (i = 0; i < 4096; i++)
-		page_table[i] = i << 20 | (3 << 10) | 0x12;
+
+	/* Set up an identity-mapping. Default version maps all 4GB rw for
+	 * everyone */
+	arm_setup_identity_mapping(page_table);
 
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
 		dram_bank_mmu_setup(i);

1.7.9.5

We introduce an OMAP5 specific version of arm_setup_identity_mapping(), which makes the first page of the identity mapping invalid.

We want to unmap the region near address zero on HS OMAP devices, to avoid speculative accesses. Accessing this region causes security violations, which we want to avoid.

---
 arch/arm/cpu/armv7/omap5/Makefile     |    1 +
 arch/arm/cpu/armv7/omap5/cache-cp15.c |   44 +++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/omap5/cache-cp15.c

index 9b261c4..49c454c 100644
@@ -29,6 +29,7 @@ COBJS	+= hwinit.o
 COBJS	+= clocks.o
 COBJS	+= emif.o
 COBJS	+= sdram.o
+COBJS	+= cache-cp15.o
 
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))

1.7.9.5

We introduce an OMAP5 specific version of arm_setup_identity_mapping(), which makes the first page of the identity mapping invalid.

We want to unmap the region near address zero on HS OMAP devices, to avoid speculative accesses. Accessing this region causes security violations, which we want to avoid.

---
Changes for v2:
- Fix missing page_table argument
- Add extern definition to fix compilation warning

 arch/arm/cpu/armv7/omap5/Makefile     |    1 +
 arch/arm/cpu/armv7/omap5/cache-cp15.c |   46 +++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/omap5/cache-cp15.c

index 9b261c4..49c454c 100644
@@ -29,6 +29,7 @@ COBJS	+= hwinit.o
 COBJS	+= clocks.o
 COBJS	+= emif.o
 COBJS	+= sdram.o
+COBJS	+= cache-cp15.o
 
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))

1.7.9.5

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Lets put the extern in arch/arm/include/asm/cache.h and make both files #include <asm/cache.h>. Thanks!

- --
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQx1SdAAoJENk4IS6UOR1WmRkQAIfaVp9YOcBPiGmKNDNTBpXL ol4psfAEEg0OGOZdzX4lIW2+NjzLtzzcwx8s8luEt4YMtaSkPlJ+Sf5eQ1Fd9KpG xBteU5PLWqE9mtTRBHylKrwqjysyuypFHfWpU4tiwLWygGI+eybRb0hrRKEplop6 BYjAOhJ8i4J5NYxQucFkLFeCTR7WAFQsxQ58rfDg/7KkVYcK71j+tZs6SmPijhIw oJoGsWnpsljC/4mbTs189Y391CKmYcSNxgRtGc6fU9NJQFJ2Vx8Ajazhasvtl8sU 7yOrxR8ssFhXSSD2/PdcKUi/VHrX/mVXTV9uk4B9ImsQt5e5Jr3c8XTnw+hLdBzz WMBpQNNPxEMibhYn3UZpskWfxR+1T1kCNbq+lVB6KVCboy98/3Bhu8OWGCLUWRk5 IB/LUfrf4uWjO5pA9hHlmzM0ckjLbQd27zGtof1qwB6EsOefL+x83XR4azqHW6Pq 6Iw+RdX7G+KKvJEqHux/fHvxrSqUzSUQJH4Bk6GYVeSNno+XfIAzNSfMfGv94zvN HMrL9AQIhxfl+2+D1siEXG5YHw4COgNhcz55DYMZK9/1HvQ6QWW5WmT/+Ty1+DfB WKX9ZLksdVl9dPgHtxlfaqWIVc2GrbwUgEzdZHVbdzsDfecNFDxxypBXU7U693KW AC/XcVMEiN1rGRL0cRFt
=giVG
-----END PGP SIGNATURE-----

Tom Rini:

Sure, here is an updated patches pair:

[PATCH v3 1/2] ARM: cache: introduce weak arm_setup_identity_mapping [PATCH v3 2/2] ARM: OMAP5: redefine arm_setup_identity_mapping

Thank you for your reviews.

Best regards,

V.

Separate the MMU identity mapping for ARM in a weak function, to allow redefinition with platform specific function.

This is motivated by the need to unmap the region near address zero on HS OMAP devices, to avoid speculative accesses. Accessing this region causes security violations, which we want to avoid.

---
Changes for v3:
- Add definition of __arm_setup_identity_mapping() into asm/cache.h
- Fix comments style

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

index eef6a5a..328377b 100644
@@ -41,6 +41,7 @@ static inline void invalidate_l2_cache(void)
 
 void l2_cache_enable(void);
 void l2_cache_disable(void);
+void __arm_setup_identity_mapping(u32 *page_table);
 
 /*
  * The current upper bound for ARM L1 data cache line sizes is 64 bytes.  We

index 939de10..f785ff5 100644
@@ -23,6 +23,8 @@
 
 #include <common.h>
 #include <asm/system.h>
+#include <asm/cache.h>
+#include <linux/compiler.h>
 
 #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
 

@@ -40,6 +42,17 @@ void __arm_init_before_mmu(void)
 void arm_init_before_mmu(void)
 	__attribute__((weak, alias("__arm_init_before_mmu")));
 
+void __arm_setup_identity_mapping(u32 *page_table)
+{
+	int i;
+
+	/* Set up an identity-mapping for all 4GB, rw for everyone */
+	for (i = 0; i < 4096; i++)
+		page_table[i] = i << 20 | (3 << 10) | 0x12;
+}
+__weak void arm_setup_identity_mapping(u32 *page_table)
+	__attribute__((alias("__arm_setup_identity_mapping")));
+
 static void cp_delay (void)
 {
 	volatile int i;

@@ -72,9 +85,12 @@ static inline void mmu_setup(void)
 	u32 reg;
 
 	arm_init_before_mmu();
-	/* Set up an identity-mapping for all 4GB, rw for everyone */
-	for (i = 0; i < 4096; i++)
-		page_table[i] = i << 20 | (3 << 10) | 0x12;
+
+	/*
+	 * Set up an identity-mapping. Default version maps all 4GB rw for
+	 * everyone
+	 */
+	arm_setup_identity_mapping(page_table);
 
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
 		dram_bank_mmu_setup(i);

1.7.9.5

We introduce an OMAP5 specific version of arm_setup_identity_mapping(), which makes the first page of the identity mapping invalid.

We want to unmap the region near address zero on HS OMAP devices, to avoid speculative accesses. Accessing this region causes security violations, which we want to avoid.

---
Changes for v3:
- Use definition of __arm_setup_identity_mapping() from asm/cache.h

Changes for v2:
- Fix missing page_table argument
- Add extern definition to fix compilation warning

 arch/arm/cpu/armv7/omap5/Makefile     |    1 +
 arch/arm/cpu/armv7/omap5/cache-cp15.c |   45 +++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/omap5/cache-cp15.c

index 9b261c4..49c454c 100644
@@ -29,6 +29,7 @@ COBJS	+= hwinit.o
 COBJS	+= clocks.o
 COBJS	+= emif.o
 COBJS	+= sdram.o
+COBJS	+= cache-cp15.o
 
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))

1.7.9.5