Protect inactive EC image from code execution

This change configures MPU to prevent instruction fetch from the flash image
that is not running at the time system_disable_jump is called. Violating
the protection causes instruction access violation, then the EC reboots.

RO image protection is tested as follows:
  ...
  [6.255696 MPU type: 00000800]
  [6.255874 RAM locked. Exclusion 20005680-200056a0]
  [6.256168 RO image locked]
  ...

  > sysjump 0
  Jumping to 0x00000000

  === PROCESS EXCEPTION: 03 ====== xPSR: 60000000 ===
  r0 :00000000 r1 :2000541c r2 :00001388 r3 :20007fe8
  r4 :200032f0 r5 :00000000 r6 :20002b70 r7 :20002df4
  r8 :0002d308 r9 :20002df4 r10:00000000 r11:00000000
  r12:00000002 sp :20002358 lr :0002a1a7 pc :00000000
  Instruction access violation, Forced hard fault
  mmfs = 1, shcsr = 70000, hfsr = 40000000, dfsr = 0

  =========== Process Stack Contents ===========
  200023c0: 00000098 00000000 00000000 0002a785
  200023d0: 00000002 20002dfd 00000007 20002b70
  200023e0: 00000002 00025777 00000000 20002dfd
  200023f0: 20002df4 20002dfc 00000000 00000000

  Rebooting...

Memory management fault status register has bit0 set, indicating there was an
instruction fetch volation. FYI, RAM protection is still working:

  > sysjump 0x20000000
  Jumping to 0x20000000

  === PROCESS EXCEPTION: 03 ====== xPSR: 60000000 ===
  r0 :00000000 r1 :2000541c r2 :00001388 r3 :20007fe8
  r4 :200032f0 r5 :20000000 r6 :20002b70 r7 :20002df4
  r8 :0002d308 r9 :20002df4 r10:00000000 r11:00000000
  r12:00000002 sp :20002358 lr :0002a1a7 pc :20000000
  Instruction access violation, Forced hard fault
  mmfs = 1, shcsr = 70000, hfsr = 40000000, dfsr = 0

  =========== Process Stack Contents ===========
  200023c0: 00000098 00000000 20000000 0002a785
  200023d0: 00000002 20002e06 00000007 20002b70
  200023e0: 00000002 00025777 00000000 20002e06
  200023f0: 20002df4 20002dfc 00000000 00000000

  Rebooting...

TEST=Booted Peppy. Tested lid close & open. Ran Flashrom from userspace to
update main firmware then software-synched an EC image.
BUG=chrome-os-partner:16904
BRANCH=none
Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
Change-Id: Id4f84d24325566a9f648194166bde0d94d1124dc
Reviewed-on: https://chromium-review.googlesource.com/169050
Reviewed-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Commit-Queue: Daisuke Nojiri <dnojiri@google.com>
Tested-by: Daisuke Nojiri <dnojiri@google.com>

3 files changed, 130 insertions, 70 deletions

diff --git a/common/system.c b/common/system.c
index 5a3e9f9e03..346496c087 100644
--- a/common/system.c
+++ b/common/system.c
@@ -225,32 +225,53 @@ void system_disable_jump(void)
 	disable_jump = 1;
 
 #ifdef CONFIG_MPU
-	/*
-	 * Lock down memory to prevent code execution from data areas.
-	 *
-	 * TODO(crosbug.com/p/16904): Also lock down the image which isn't
-	 * running (RO if RW, or vice versa), so a bad or malicious jump can't
-	 * execute code from that image.
-	 */
 	if (system_is_locked()) {
+		int ret;
+		int enable_mpu = 0;
+		enum system_image_copy_t copy;
+
+		CPRINTF("[%T MPU type: %08x]\n", mpu_get_type());
 		/*
-		 * Protect memory from code execution
+		 * Protect RAM from code execution
 		 */
-		int mpu_error = mpu_protect_ram();
-		if (mpu_error == EC_SUCCESS) {
-			mpu_enable();
+		ret = mpu_protect_ram();
+		if (ret == EC_SUCCESS) {
+			enable_mpu = 1;
 			CPRINTF("[%T RAM locked. Exclusion %08x-%08x]\n",
 				&__iram_text_start, &__iram_text_end);
 		} else {
-			CPRINTF("[%T Failed to lock RAM (%d). mpu_type:%08x]\n",
-				mpu_error, mpu_get_type());
+			CPRINTF("[%T Failed to lock RAM (%d)]\n", ret);
 		}
+
 		/*
-		 * Protect the other image from code execution
-		 * TODO: https://chromium-review.googlesource.com/#/c/169050/
+		 * Protect inactive image (ie. RO if running RW, vice versa)
+		 * from code execution.
 		 */
+		switch (system_get_image_copy()) {
+		case SYSTEM_IMAGE_RO:
+			ret =  mpu_lock_rw_flash();
+			copy = SYSTEM_IMAGE_RW;
+			break;
+		case SYSTEM_IMAGE_RW:
+			ret =  mpu_lock_ro_flash();
+			copy = SYSTEM_IMAGE_RO;
+			break;
+		default:
+			copy = SYSTEM_IMAGE_UNKNOWN;
+			ret = !EC_SUCCESS;
+		}
+		if (ret == EC_SUCCESS) {
+			enable_mpu = 1;
+			CPRINTF("[%T %s image locked]\n", image_names[copy]);
+		} else {
+			CPRINTF("[%T Failed to lock %s image (%d)]\n",
+				image_names[copy], ret);
+		}
+
+		if (enable_mpu)
+			mpu_enable();
 	} else {
-		CPRINTF("[%T RAM not locked]\n");
+		CPRINTF("[%T System is unlocked. Skip MPU configuration\n");
 	}
 #endif
 }
diff --git a/core/cortex-m/include/mpu.h b/core/cortex-m/include/mpu.h
index c3a632aff7..c0c595a367 100644
--- a/core/cortex-m/include/mpu.h
+++ b/core/cortex-m/include/mpu.h
@@ -20,39 +20,27 @@
 #define MPU_CTRL_PRIVDEFEN	(1 << 2)
 #define MPU_CTRL_HFNMIENA	(1 << 1)
 #define MPU_CTRL_ENABLE		(1 << 0)
-#define MPU_ATTR_NX		(1 << 12)
-#define MPU_ATTR_NOACCESS	(0 << 8)
-#define MPU_ATTR_FULLACCESS	(3 << 8)
-/* Suggested in table 3-6 of Stellaris LM4F232H5QC Datasheet and table 38 of
- * STM32F10xxx Cortex-M3 programming manual for internal sram. */
-#define MPU_ATTR_INTERNALSRAM	6
 
-/**
- * Configure a region
- *
- * region: Number of the region to update
- * addr: Base address of the region
- * size: Size of the region in bytes
- * attr: Attribute of the region. Current value will be overwritten if enable
- * is set.
- * enable: Enables the region if non zero. Otherwise, disables the region.
- *
- * Returns EC_SUCCESS on success or EC_ERROR_INVAL if a parameter is invalid.
+/*
+ * XN (execute never) bit. It's bit 12 if accessed by halfword.
+ *   0: XN off
+ *   1: XN on
  */
-int mpu_config_region(uint8_t region, uint32_t addr, uint32_t size,
-		      uint16_t attr, uint8_t enable);
+#define MPU_ATTR_XN		(1 << 12)
 
-/**
- * Set a region non-executable.
- *
- * region: number of the region
- * addr: base address of the region
- * size: size of the region in bytes
- */
-int mpu_nx_region(uint8_t region, uint32_t addr, uint32_t size);
+/* AP bit. See table 3-5 of Stellaris LM4F232H5QC datasheet for details */
+#define MPU_ATTR_NO_NO (0 << 8)  /* previleged no access, unprev no access */
+#define MPU_ATTR_RW_RW (3 << 8)  /* previleged ReadWrite, unprev ReadWrite */
+#define MPU_ATTR_RO_NO (5 << 8)  /* previleged Read-only, unprev no access */
+
+/* Suggested value for TEX S/C/B bit. See table 3-6 of Stellaris LM4F232H5QC
+ * datasheet and table 38 of STM32F10xxx Cortex-M3 programming manual. */
+#define MPU_ATTR_INTERNAL_SRAM  6  /* for Internal SRAM */
+#define MPU_ATTR_FLASH_MEMORY   2  /* for flash memory */
 
 /**
- * Enable MPU */
+ * Enable MPU
+ */
 void mpu_enable(void);
 
 /**
@@ -70,11 +58,17 @@ extern char __iram_text_start;
 extern char __iram_text_end;
 
 /**
- * Lock down RAM
+ * Protect RAM from code execution
  */
 int mpu_protect_ram(void);
 
 /**
+ * Protect flash memory from code execution
+ */
+int mpu_lock_ro_flash(void);
+int mpu_lock_rw_flash(void);
+
+/**
  * Initialize MPU.
  * It disables all regions if MPU is implemented. Otherwise, returns
  * EC_ERROR_UNIMPLEMENTED.
diff --git a/core/cortex-m/mpu.c b/core/cortex-m/mpu.c
index 23d434d7e6..b8f2ca8def 100644
--- a/core/cortex-m/mpu.c
+++ b/core/cortex-m/mpu.c
@@ -11,20 +11,28 @@
 #include "task.h"
 #include "util.h"
 
+/* Region assignment. 7 as the highest, a higher index has a higher priority.
+ * For example, using 7 for .iram.text allows us to mark entire RAM XN except
+ * .iram.text, which is used for hibernation. */
+enum mpu_region {
+	REGION_IRAM = 0,          /* For internal RAM */
+	REGION_FLASH_MEMORY = 1,  /* For flash memory */
+	REGION_IRAM_TEXT = 7      /* For *.(iram.text) */
+};
+
 /**
  * Update a memory region.
  *
- * region: number of the region to update
+ * region: index of the region to update
  * addr: base address of the region
  * size_bit: size of the region in power of two.
- * attr: attribute of the region. Current value will be overwritten if enable
- * is set.
+ * attr: attribute bits. Current value will be overwritten if enable is true.
  * enable: enables the region if non zero. Otherwise, disables the region.
  *
  * Based on 3.1.4.1 'Updating an MPU Region' of Stellaris LM4F232H5QC Datasheet
  */
 static void mpu_update_region(uint8_t region, uint32_t addr, uint8_t size_bit,
-		       uint16_t attr, uint8_t enable)
+			      uint16_t attr, uint8_t enable)
 {
 	asm volatile("isb; dsb;");
 
@@ -39,8 +47,19 @@ static void mpu_update_region(uint8_t region, uint32_t addr, uint8_t size_bit,
 	asm volatile("isb; dsb;");
 }
 
-int mpu_config_region(uint8_t region, uint32_t addr, uint32_t size,
-		      uint16_t attr, uint8_t enable)
+/**
+ * Configure a region
+ *
+ * region: index of the region to update
+ * addr: Base address of the region
+ * size: Size of the region in bytes
+ * attr: Attribute bits. Current value will be overwritten if enable is set.
+ * enable: Enables the region if non zero. Otherwise, disables the region.
+ *
+ * Returns EC_SUCCESS on success or EC_ERROR_INVAL if a parameter is invalid.
+ */
+static int mpu_config_region(uint8_t region, uint32_t addr, uint32_t size,
+			     uint16_t attr, uint8_t enable)
 {
 	int size_bit = 0;
 
@@ -60,11 +79,34 @@ int mpu_config_region(uint8_t region, uint32_t addr, uint32_t size,
 	return EC_SUCCESS;
 }
 
-int mpu_nx_region(uint8_t region, uint32_t addr, uint32_t size)
+/**
+ * Set a region non-executable and read-write.
+ *
+ * region: index of the region
+ * addr: base address of the region
+ * size: size of the region in bytes
+ * texscb: TEX and SCB bit field
+ */
+static int mpu_lock_region(uint8_t region, uint32_t addr, uint32_t size,
+			   uint8_t texscb)
 {
-	return mpu_config_region(
-		region, addr, size,
-		MPU_ATTR_NX | MPU_ATTR_FULLACCESS | MPU_ATTR_INTERNALSRAM, 1);
+	return mpu_config_region(region, addr, size,
+				 MPU_ATTR_XN | MPU_ATTR_RW_RW | texscb, 1);
+}
+
+/**
+ * Set a region executable and read-write.
+ *
+ * region: index of the region
+ * addr: base address of the region
+ * size: size of the region in bytes
+ * texscb: TEX and SCB bit field
+ */
+static int mpu_unlock_region(uint8_t region, uint32_t addr, uint32_t size,
+			     uint8_t texscb)
+{
+	return mpu_config_region(region, addr, size,
+				 MPU_ATTR_RW_RW | texscb, 1);
 }
 
 void mpu_enable(void)
@@ -82,27 +124,32 @@ uint32_t mpu_get_type(void)
 	return MPU_TYPE;
 }
 
-/**
- * Prevent code from running on RAM.
- * We need to allow execution from iram.text. Using higher region# allows us to
- * do 'whitelisting' (lock down all then create exceptions).
- */
 int mpu_protect_ram(void)
 {
 	int ret;
-
-	ret = mpu_nx_region(0, CONFIG_RAM_BASE, CONFIG_RAM_SIZE);
+	ret = mpu_lock_region(REGION_IRAM, CONFIG_RAM_BASE,
+			      CONFIG_RAM_SIZE, MPU_ATTR_INTERNAL_SRAM);
 	if (ret != EC_SUCCESS)
 		return ret;
-
-	ret = mpu_config_region(
-		7, (uint32_t)&__iram_text_start,
+	ret = mpu_unlock_region(
+		REGION_IRAM_TEXT, (uint32_t)&__iram_text_start,
 		(uint32_t)(&__iram_text_end - &__iram_text_start),
-		MPU_ATTR_FULLACCESS | MPU_ATTR_INTERNALSRAM, 1);
-
+		MPU_ATTR_INTERNAL_SRAM);
 	return ret;
 }
 
+int mpu_lock_ro_flash(void)
+{
+	return mpu_lock_region(REGION_FLASH_MEMORY, CONFIG_FW_RO_OFF,
+			       CONFIG_FW_IMAGE_SIZE, MPU_ATTR_FLASH_MEMORY);
+}
+
+int mpu_lock_rw_flash(void)
+{
+	return mpu_lock_region(REGION_FLASH_MEMORY, CONFIG_FW_RW_OFF,
+			       CONFIG_FW_IMAGE_SIZE, MPU_ATTR_FLASH_MEMORY);
+}
+
 int mpu_pre_init(void)
 {
 	int i;
@@ -112,9 +159,7 @@ int mpu_pre_init(void)
 
 	mpu_disable();
 	for (i = 0; i < 8; ++i) {
-		mpu_config_region(
-			i, CONFIG_RAM_BASE, CONFIG_RAM_SIZE,
-			MPU_ATTR_FULLACCESS | MPU_ATTR_INTERNALSRAM, 0);
+		mpu_config_region(i, CONFIG_RAM_BASE, CONFIG_RAM_SIZE, 0, 0);
 	}
 
 	return EC_SUCCESS;