MPU Flash write #205

fvincenzo · 2017-05-16T14:13:27Z

This PR depends on #54. Please do not merge directly.

The new patches are the last 3.

Since we now select HAS_DTS from the cortex-m Kconfig we don't need it in 96b_carbon_nrf51_defconfig. Signed-off-by: Kumar Gala <[email protected]>

As we now only support DTS for ARM based SoCs we can remove any associated !HAS_DTS bits in Kconfig. The Nordic NRF5 serial driver and the ARM CMSDK APB serial drivers had Kconfig bits related to !HAS_DTS builds. Signed-off-by: Kumar Gala <[email protected]>

Update Cube version for STM32L4XX family from version: V1.6.0 to version: V1.8.0 Add support for following soc: stm32l496xx stm32l4a6xx Signed-off-by: Neil Armstrong <[email protected]>

Add configuration and dts for the STM32L496 SoC STM32L4 variant. Signed-off-by: Neil Armstrong <[email protected]>

Add configuration, pinmux, dts and documentation for the STM32L496G Discovery board based on the STM32L496AG SoC. Signed-off-by: Neil Armstrong <[email protected]>

Add missing dts for STM32F407. Signed-off-by: Neil Armstrong <[email protected]>

Add configuration, documentation, flash script, pinmux and dts for the STM32F4DISCOVERY board. Signed-off-by: Neil Armstrong <[email protected]>

galak

Can you push to arm branch instead

mbolivar · 2017-05-16T14:55:39Z

arch/arm/core/cortex_m/mpu/Kconfig

+config MPU_ALLOW_FLASH_WRITE
+	bool "Add MPU access to write to flash"
+	depends on NXP_MPU
+	default n


Thanks a lot for submitting this! It will really help us.

What do people think about making this option default y if FLASH or so?

That seems less surprising; by default, asking for both the flash driver and the MPU driver would result in a config with fewer pointy edges. User effort would only be needed to get the flash driver and to allow flash reads, but not writes. @galak pointed out that this is a confusing configuration whose utility is a bit unclear, so perhaps that setup should take more effort to create.

@mbolivar I agree with the approach.

In theory, yes I agree. But I wonder if there are security implications? @agross-linaro

Default options should not make things less secure. I'd rather the user have to jump through 1 more hoop if they want to specifically allow this. That's my two cents.

It seems you're saying that asking for the MPU driver means getting a maximally locked down configuration (that can still run code), and any relaxations should require additional knobs. Is that right?

If so, it's a different point on the security / convenience curve, but that seems reasonable to me.

This sounds fine, my only complaint is the naming. I don't like options where you have DISABLE and say Y. It's negative logic. I'd rather have a ENABLE=y, so FLASH_ENABLE_WRITE_SUPPORT.

Sorry to chime in late here, but it sounds like we're going for a seemingly more secure, and definitely more surprising configuration.

Preventing writes to flash isn't really all that much of a security improvement. In order to actually have secure boot, the bootloader has to be protected by hardware. MPU is insufficient, because it is easy to break before it is set up and then just replace the main code.

Once the bootloader is running, there is no security issue with being able to write to the code space. The images must be signed in order to be bootable.

In addition, apps using the bootloader are going to want to do OTA, which will require flash writing.

I really don't see any scenario where the flash driver would be wanted, but writes not allowed to the flash device.

Also, at least the k6x devices mentioned in the commit text have the ability to write protect flash regions, which is what will be needed to actually provide secure boot in a meaningful way.

One last point, it seems that on NXP, just probing the flash driver requires write access, whether the app tries to write or not. It kind of makes having this selectable a moot point. If you enable the flash driver, with writes disabled, it faults before main is even reached.

fvincenzo · 2017-05-16T15:19:46Z

@galak pushed against arm branch.

dbkinder · 2017-05-16T17:33:49Z

samples/mpu_test/README.rst

@@ -46,6 +46,13 @@ 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.conf`:


no such file (there are prj_allow_flash_write_mcux.conf and prj_allow_flash_write_stm32.conf). Maybe use the same notation you use in the example below prj_allow_flash_write_.conf and tell them what the means.

Ok I will change it. Thanks

dbkinder

recommend fixing :file: reference to prj_allow_flash_write.conf

MaureenHelm · 2017-05-16T17:44:23Z

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

+#if defined(CONFIG_MPU_ALLOW_FLASH_WRITE)
+/* Required to initialize the flash driver */
+#define REGION_RAM_ATTR (MPU_REGION_READ | MPU_REGION_WRITE | \
+			 MPU_REGION_EXEC |  MPU_REGION_SU)


I think we should bring more attention to the fact that not only are we enabling flash write, but also execution from ram. Can you mention this explicitly in the commit message? Is there a better config name to indicate this?

Ok I will make it more clear. Thanks for pointing it out.

I agree with Maureen. I'd think about a more descriptive config name if that is easy to deal with. Otherwise just deal with it in the commit msg.

MaureenHelm · 2017-05-16T17:45:55Z

arch/arm/core/cortex_m/mpu/Kconfig

+config MPU_ALLOW_FLASH_WRITE
+	bool "Add MPU access to write to flash"
+	depends on NXP_MPU
+	default n


In theory, yes I agree. But I wonder if there are security implications? @agross-linaro

The stm32f4discovery was incorrectly named and should have been stm32f4_disco. Also added 96b_carbon_nrf51 that was missing from the list. Signed-off-by: Kumar Gala <[email protected]>

This patch adds the allow flash write CONFIG option to the NXP MPU configuration in privileged mode. Signed-off-by: Vincenzo Frascino <[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]>

This patch adds a configuration file that tests Flash writes with MPU enabled. Signed-off-by: Vincenzo Frascino <[email protected]>

galak · 2017-05-17T16:37:20Z

Do we have an example that can be added to this PR for the case in which we want to allow flash_read() to work but not flash_write()?

I feel like towards Andy's comment, that if we do allow for such an option, it would be DISABLE_WRITE_SUPPORT, and I'd think we'd make the API (flash_write) go away as part of it.

d3zd3z · 2017-06-30T21:17:00Z

I've rebased these changes and put them in #654. We should close this PR and continue discussion there.

mbolivar · 2017-07-26T18:03:30Z

Now that #654 has been approved, this should be closed.

[build] Added feature to analyze.sh to generate prj.conf options

galak and others added 7 commits May 16, 2017 09:35

arm: dts: cleanup HAS_DTS from defconfig

065d06d

Since we now select HAS_DTS from the cortex-m Kconfig we don't need it in 96b_carbon_nrf51_defconfig. Signed-off-by: Kumar Gala <[email protected]>

ext: stm32cube: update stm32l4xx cube version

33a99ba

Update Cube version for STM32L4XX family from version: V1.6.0 to version: V1.8.0 Add support for following soc: stm32l496xx stm32l4a6xx Signed-off-by: Neil Armstrong <[email protected]>

arm: stm32l4: Add configuration and dts for STM32L496

5bfab9f

Add configuration and dts for the STM32L496 SoC STM32L4 variant. Signed-off-by: Neil Armstrong <[email protected]>

boards: arm: Add support for STM32L496G Discovery board

87b0228

Add configuration, pinmux, dts and documentation for the STM32L496G Discovery board based on the STM32L496AG SoC. Signed-off-by: Neil Armstrong <[email protected]>

arm: Add dts for STM32F407

15b2bce

Add missing dts for STM32F407. Signed-off-by: Neil Armstrong <[email protected]>

arm: Add support for STM32F4DISCOVERY Board

71157c7

Add configuration, documentation, flash script, pinmux and dts for the STM32F4DISCOVERY board. Signed-off-by: Neil Armstrong <[email protected]>

galak changed the title ~~[Depends on: https://github.com/zephyrproject-rtos/zephyr/pull/54] Flash write~~ MPU Flash write May 16, 2017

galak self-assigned this May 16, 2017

galak self-requested a review May 16, 2017 14:37

galak requested changes May 16, 2017

View reviewed changes

galak requested review from mbolivar and MaureenHelm May 16, 2017 14:38

mbolivar reviewed May 16, 2017

View reviewed changes

fvincenzo changed the base branch from master to arm May 16, 2017 15:13

dbkinder reviewed May 16, 2017

View reviewed changes

dbkinder suggested changes May 16, 2017

View reviewed changes

MaureenHelm requested changes May 16, 2017

View reviewed changes

MaureenHelm requested a review from agross-oss May 16, 2017 17:51

galak and others added 4 commits May 16, 2017 16:58

arm: Fixup sanity check list of boards

c530b40

The stm32f4discovery was incorrectly named and should have been stm32f4_disco. Also added 96b_carbon_nrf51 that was missing from the list. Signed-off-by: Kumar Gala <[email protected]>

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

f0fabb0

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

arm: core: mpu: Add Allow write on Flash

e70f1ac

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

samples: mpu_test: Add Allow Flash Write config

81b55c9

This patch adds a configuration file that tests Flash writes with MPU enabled. Signed-off-by: Vincenzo Frascino <[email protected]>

galak added this to the v1.9 milestone May 17, 2017

galak force-pushed the arm branch from 42dad4a to ed187eb Compare May 19, 2017 17:40

galak force-pushed the arm branch 2 times, most recently from 1d365cc to 0a8ad73 Compare June 9, 2017 14:38

galak force-pushed the arm branch from adce86e to 8b5c982 Compare June 12, 2017 16:45

mbolivar mentioned this pull request Jun 14, 2017

Flash map updates, redux mcu-tools/mcuboot#66

Merged

galak force-pushed the arm branch 5 times, most recently from c52d348 to 41c85d8 Compare June 22, 2017 14:55

galak force-pushed the arm branch 4 times, most recently from f3f7aad to a87804d Compare June 29, 2017 21:23

galak force-pushed the arm branch from f95c1d6 to 4c01e1a Compare June 30, 2017 20:53

d3zd3z mentioned this pull request Jun 30, 2017

MPU flash write #654

Merged

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

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

galak closed this Jul 26, 2017

nashif modified the milestones: v1.9, v1.9.0 Oct 3, 2017

nagineni pushed a commit to nagineni/zephyr that referenced this pull request Nov 20, 2017

Merge pull request zephyrproject-rtos#205 from jprestwo/analyze-prj

a4e2b41

[build] Added feature to analyze.sh to generate prj.conf options

MPU Flash write #205

MPU Flash write #205

Uh oh!

Conversation

fvincenzo commented May 16, 2017

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fvincenzo commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galak commented May 17, 2017

Uh oh!

d3zd3z commented Jun 30, 2017

Uh oh!

mbolivar commented Jul 26, 2017

Uh oh!

Uh oh!