MPU flash write #654

d3zd3z · 2017-06-30T21:16:18Z

This is a rebase of #205. This is needed to even enable the flash driver on NXP without faulting.

mbolivar · 2017-07-03T15:19:26Z

Thanks for picking this back up!

This LGTM; one minor question is about the discussion in #205 on whether MPU_ALLOW_FLASH_WRITE should be 'default y if FLASH', or if the user should need to know about it and turn it on.

You've noted that the changed configuration is required on NXP just to enable the flash driver -- regardless of whether flash_erase() or flash_write() are called. I wonder if a better name for it would be MPU_ALLOW_FLASH_ACCESS, and make it default y if flash.

d3zd3z · 2017-07-05T14:06:45Z

It might be easiest to bring this in without changing the default, and a future patch could do that if we agree. Right now, without this patch, the flash drivers aren't useful, with it as is, it at least can be enabled.

nashif

Instead of adding more configuration files prj_XXX.conf which are platform specific I would prefer to document the options instead or add them to the original config file and disable them and document that they need to be enabled.

nashif · 2017-07-05T14:25:43Z

samples/mpu_test/prj_allow_flash_write_mcux.conf

@@ -0,0 +1,3 @@
+CONFIG_CONSOLE_SHELL=y
+CONFIG_MPU_ALLOW_FLASH_WRITE=y
+CONFIG_FLASH=y


The sample has moved to samples/mpu/mpu_test, so those files need to move...

dbkinder · 2017-07-05T17:24:47Z

samples/mpu/mpu_test/README.rst

@@ -46,6 +46,15 @@ single thread: :file:`prj_single.conf`:

   $ make BOARD=v2m_beetle CONF_FILE=prj_single.conf run

+To build the version that allows flash write, use the supplied configuration
+file: :file:`prj_allow_flash_write_<x>.conf`:


before the trailing colon, add the explanation about <x> instead of after the code block:

file: :file:`prj_allow_flash_write_<x>.conf`, replacing `<x>` with either `mcux` or `stm32`:

dbkinder · 2017-07-05T17:24:59Z

samples/mpu/mpu_test/README.rst

+
+   $ make BOARD=v2m_beetle CONF_FILE=prj_allow_flash_write_<x>.conf run
+
+<x> = mcux or stm32


... and delete this here.

dbkinder

change recommended

MaureenHelm · 2017-07-05T20:54:30Z

You've noted that the changed configuration is required on NXP just to enable the flash driver -- regardless of whether flash_erase() or flash_write() are called. I wonder if a better name for it would be MPU_ALLOW_FLASH_ACCESS, and make it default y if flash.

We need more than just flash write access though, we also need sram execute, and I think enabling sram execute should be explicit. The reason we fault when enabling the nxp flash driver is that the driver init actually tries to execute from ram. The problem code is under the shim, in ext/hal/nxp/mcux/drivers/fsl_flash.c:2695:

/* Workround for some devices which doesn't need this function */
callFlashCommonBitOperation((FTFx_REG32_ACCESS_TYPE)0, 0, 0, 0)

I'm not quite sure yet why the driver does this.

jfischer-no · 2017-07-14T22:38:44Z

I have stumbled upon the problem with MPU and mcux flash driver while playing with #542 and usb dfu class sample.
As Maureen already described, it can be fixed by something like this:

+#ifdef CONFIG_SOC_FLASH_MCUX
+#define MPU_REGION_EXEC_FROM_RAM       MPU_REGION_EXEC
+#else
+#define MPU_REGION_EXEC_FROM_RAM       0
+#endif
+
 /* Super User Attributes */
 #define MPU_REGION_SU   ((SM_SAME_AS_UM << BM0_SM_SHIFT) | \
                          (SM_SAME_AS_UM << BM1_SM_SHIFT) | \
@@ -77,6 +83,7 @@
 /* Some helper defines for common regions */
 #define REGION_RAM_ATTR          (MPU_REGION_READ | \
                           MPU_REGION_WRITE | \
+                          MPU_REGION_EXEC_FROM_RAM | \
                           MPU_REGION_SU)

The driver uses the InRamFunctions when it executes from flash. K64F Reference Manual was not really helpful, I only found a note in Chapter 29.4.7: "The MCU must not read from the flash memory while commands are running...". Anyway, I think permanent permission to execute the code from the RAM is not a good solution.

d3zd3z · 2017-07-17T22:34:30Z

I'm not quite sure yet why the driver does this.

The way the NXP controller is implemented the entire contents of flash go away while performing flash operations. This is fairly common among lots of NOR controllers. It generally takes extra hardware to be able to read/execute from FLASH that do allow reads of the parts that aren't being programmed.

Anyway, I think permanent permission to execute the code from the RAM is not a good solution.

Is the concern here with rogue code making it into RAM, and then some kind of pointer fault causing it to execute at this address? Yes this is a concern, but I don't think it is a particularly common exploit. One of the things we're going to have to face here is that the MPU isn't going to be able to protect everything, and we need to figure out what is most important to protect.

We can certainly make this dynamic, but it adds complexity and additional functionality, not really intended.

I fear we are going to end up bikeshedding this patch series to death, and never get it in. As it is, the MPU is completely useless for any code that needs to write to flash. On NXP, it is worse, and the MPU prevents boot entirely if the flash driver is even enabled. An MPU that protects some things is better than having to disable it entirely.

d3zd3z · 2017-07-18T20:44:54Z

Instead of adding more configuration files prj_XXX.conf which are platform specific I would prefer to document the options instead or add them to the original config file and disable them and document that they need to be enabled.

Wouldn't this also mean that they won't get tested, either?

I'm beginning to lean toward the correct solution here being to not have these config options, but just a few settings that get selected according to what other features are enabled:

enabling CONFIG_FLASH causes CONFIG_MPU_ALLOW_FLASH_WRITE to be selected.
enabling both CONFIG_FLASH and the NXP CONFIG_SOC_FLASH_MCUX will enable CONFIG_MPU_ALLOW_RAM_EXECUTE.

I'll give a little time for feedback on this proposal, and then rework the patches to do this.

mbolivar · 2017-07-21T18:48:06Z

I fear we are going to end up bikeshedding this patch series to death, and never get it in

Guilty :(. Let me get out of your way...

andrewboie · 2017-07-21T19:09:13Z

Need to address @dbkinder doc comments.
Need @nashif to re-visit his review.
Substance of patch LGTM I think we should proceed, I think we all have an understanding that the memory protection features are going to be subject to iterative refinement over the next 6 months or so, we can tinker with this down the line if we need to.

This patch adds the allow flash write CONFIG option to the NXP MPU configuration in privileged mode. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: David Brown <[email protected]>

This patch adds the allow flash write CONFIG option to the ARM MPU configuration in privileged mode. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Michael Scott <[email protected]> Signed-off-by: David Brown <[email protected]>

d3zd3z · 2017-07-24T15:24:56Z

New patches. Addressed @nashif comments by eliminating new config files, and adding commented out (and described) lines in the main prj.conf file. This also addresses @dbkinder comments by simplifying what is written in the docs.

dbkinder · 2017-07-24T23:38:05Z

samples/mpu/mpu_test/README.rst

@@ -46,6 +46,10 @@ single thread: :file:`prj_single.conf`:

   $ make BOARD=v2m_beetle CONF_FILE=prj_single.conf run

+To build a version that allows writes to the flash device, edit
+``prj.conf``, and following the directions in the comments to enable


s/following/follow/

dbkinder

small edit requested

This patch adds a configuration file that tests Flash writes with MPU enabled. [david.brown: Put options in prj.conf with comments as per review feedback] Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: David Brown <[email protected]>

d3zd3z · 2017-07-27T15:23:43Z

Ping @dbkinder, I believe I've addressed your concern.

dbkinder

+1

MaureenHelm

I'm not quite sure yet why the driver does this.

The way the NXP controller is implemented the entire contents of flash go away while performing flash operations. This is fairly common among lots of NOR controllers. It generally takes extra hardware to be able to read/execute from FLASH that do allow reads of the parts that aren't being programmed.

My concern was why the mcux flash driver tries to execute anything from ram during init, before anything tries to erase or program flash. In other words, why we can't even boot Zephyr with the flash driver enabled. This will get fixed in the next release of mcux.

MaureenHelm · 2017-07-31T20:11:12Z

include/arch/arm/cortex_m/mpu/arm_mpu.h

+#if defined(CONFIG_MPU_ALLOW_FLASH_WRITE)
+#define REGION_FLASH_ATTR(size) \
+		(NORMAL_OUTER_INNER_NON_CACHEABLE_NON_SHAREABLE | size | \
+		 P_RW_U_RO)


Does REGION_RAM_ATTR need execute access as in the NXP MPU?

…ject-rtos#654) This bug was hit if you tried to pass the fourth encoding parameter to the function; it would get an error even if you passed 'utf8'. Also, cleaned up indentation issue in I2C.js. Signed-off-by: Geoff Gustafson <[email protected]>

d3zd3z mentioned this pull request Jun 30, 2017

MPU Flash write #205

Closed

galak force-pushed the arm branch from 4c01e1a to 395daba Compare July 1, 2017 16:08

andrewboie previously approved these changes Jul 5, 2017

View reviewed changes

andrewboie requested a review from nashif July 5, 2017 14:11

nashif requested changes Jul 5, 2017

View reviewed changes

dbkinder reviewed Jul 5, 2017

View reviewed changes

dbkinder suggested changes Jul 5, 2017

View reviewed changes

MaureenHelm requested review from galak and agross-oss July 5, 2017 20:55

galak force-pushed the arm branch 2 times, most recently from 0f36cad to 1bc2fdc Compare July 7, 2017 15:32

galak force-pushed the arm branch from bfa5cf5 to 86d8e44 Compare July 14, 2017 16:08

galak force-pushed the arm branch 3 times, most recently from 0723c92 to 38db4ed Compare July 19, 2017 16:37

mbolivar mentioned this pull request Jul 24, 2017

RFC: Memory Domain APIs #814

Merged

d3zd3z dismissed andrewboie’s stale review via 1a4fbfb July 24, 2017 15:20

d3zd3z force-pushed the pr/mpu-flash branch from 98a88ea to 1a4fbfb Compare July 24, 2017 15:20

Vincenzo Frascino added 2 commits July 24, 2017 09:23

arm: soc: nxp k6x: Add Allow write on Flash

5751e14

This patch adds the allow flash write CONFIG option to the NXP MPU configuration in privileged mode. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: David Brown <[email protected]>

d3zd3z force-pushed the pr/mpu-flash branch from 1a4fbfb to c5abde7 Compare July 24, 2017 15:23

dbkinder reviewed Jul 24, 2017

View reviewed changes

dbkinder suggested changes Jul 24, 2017

View reviewed changes

d3zd3z force-pushed the pr/mpu-flash branch from c5abde7 to d9c23b7 Compare July 25, 2017 17:14

nashif approved these changes Jul 26, 2017

View reviewed changes

andrewboie approved these changes Jul 27, 2017

View reviewed changes

dbkinder approved these changes Jul 27, 2017

View reviewed changes

MaureenHelm force-pushed the arm branch from 38db4ed to b4598c1 Compare July 28, 2017 19:55

MaureenHelm reviewed Jul 31, 2017

View reviewed changes

galak approved these changes Aug 1, 2017

View reviewed changes

galak merged commit fce2cc1 into zephyrproject-rtos:arm Aug 1, 2017


		$ make BOARD=v2m_beetle CONF_FILE=prj_allow_flash_write_<x>.conf run

		<x> = mcux or stm32

MPU flash write #654

MPU flash write #654

Uh oh!

Conversation

d3zd3z commented Jun 30, 2017

Uh oh!

mbolivar commented Jul 3, 2017

Uh oh!

d3zd3z commented Jul 5, 2017

Uh oh!

nashif left a comment

Choose a reason for hiding this comment

Uh oh!

nashif Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

dbkinder Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

dbkinder Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

MaureenHelm commented Jul 5, 2017

Uh oh!

jfischer-no commented Jul 14, 2017

Uh oh!

d3zd3z commented Jul 17, 2017

Uh oh!

d3zd3z commented Jul 18, 2017

Uh oh!

mbolivar commented Jul 21, 2017

Uh oh!

andrewboie commented Jul 21, 2017

Uh oh!

d3zd3z commented Jul 24, 2017

Uh oh!

dbkinder Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

d3zd3z commented Jul 27, 2017

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

MaureenHelm left a comment

Choose a reason for hiding this comment

Uh oh!

MaureenHelm Jul 31, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!