MPU API #8335

c1728p9 · 2018-10-06T01:31:15Z

Description

Add a minimalistic HAL MPU API with the ability to prevent execution in ram. Enable this by default.

~~This is just a preliminary patchset intended for testing and feedback.~~

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Breaking change

c1728p9 · 2018-10-06T01:32:05Z

CC: @deepikabhavnani @SenRamakri @dreemkiller @bulislaw @donatieng

TESTS/mbed_hal/mpu/main.cpp

hal/mbed_mpu_api.c

SenRamakri · 2018-10-08T20:48:37Z

platform/mbed_mpu_mgmt.c

+    core_util_critical_section_enter();
+    if (mem_xn_lock == USHRT_MAX) {
+        core_util_critical_section_exit();
+        MBED_ERROR1(MBED_MAKE_ERROR(MBED_MODULE_HAL, MBED_ERROR_CODE_OVERFLOW), "Memory execute never lock overflow (> USHRT_MAX)", mem_xn_lock);


Should the module be MBED_MODULE_PLATFORM or MBED_MODULE_HAL? Since the code is under platform should that be MBED_MODULE_PLATFORM.

I changed this to MBED_MODULE_PLATFORM since it is under the platform directory.

c1728p9 · 2018-10-10T21:34:11Z

Made the following changes:

Rebased to master to pull in fixes
Added cache settings which match the default for all MPU regions
Hardcoded hardfault IRQn since this differs on some targets
Turned off the MPU for devices not part of the ARMv6m and ARMv7m architecture

c1728p9 · 2018-10-18T00:40:36Z

Added a design doc and title of this PR. I also removed Need work since this can be merged once everyone is happy with it.

kjbracey · 2018-10-18T09:38:45Z

At the minute, because mbed OS doesn't touch the MPU, targets can and do use it to configure particular requirements about, for example, marking SRAM regions for Ethernet or similar buffers Shareable/Non-cacheable.

I think you may need some sort of hook to allow that, or maybe just reserve some region numbers for target use - eg don't touch regions 4 and up.

c1728p9 · 2018-10-19T00:44:32Z

@kjbracey-arm the default implementation here is for the ARMv7m MPU which I don't see being used in any of the targets. I know some Kinetis devices turn off their MPU for certain operations but Kinetis devices have a custom MPU, so this change won't effect them. Which targets did you see changing MPU settings? Are any targets with an Arm MPU using it?

cmonr · 2018-10-19T19:28:26Z

@SenRamakri @ARMmbed/mbed-os-hal @bulislaw @flit @dreemkiller @donatieng @deepikabhavnani @TacoGrandeTX
A reminder that this still in need of a review

c1728p9 · 2018-10-20T00:25:24Z

I rebased this to master and added a commit to disable the MPU when doing flash programming.

TESTS/mbed_hal/mpu/main.cpp

drivers/FlashIAP.cpp

bulislaw

+1 for the design docs. Please make sure docs are in place both for the platform and porting guide sides.

bulislaw · 2018-10-29T12:17:28Z

docs/design-documents/hal/mpu_design.md

+ - System integration so the execution from RAM is disabled by default
+ - Initial porting for the Arm MPU
+
+The goal of this is to increase security for a large number of devices by disabling execution from ram it is not required.


disabling execution from ram it is not required. what's not required?

Good catch, this should be disabling execution from ram when it is not required

bulislaw · 2018-10-29T12:28:38Z

hal/mbed_mpu_api.c

+    }
+
+    /*
+     * ARMv6m and ARMv7m memory map:


Would it make sense to allow platforms override default memory map on hal level?

This may be required if there is a target with non-standard memory, but for now there isn't a good reason to.

deepikabhavnani

We discussed one fix https://github.com/ARMmbed/mbed-os/pull/8335/files#diff-891ef2631c17d62d627a2598292b2ebeR81, which is needed, Apart from that looks good. Thanks 👍

c1728p9 · 2018-10-29T22:16:35Z

Added fixes, an RAII XN API and rebased on master.

donatieng

Great stuff @c1728p9
@ARMmbed/mbed-os-maintainers please hold CI for now, v8M support is coming
@dreemkiller would you be able to check this?

cmonr · 2018-11-26T15:21:11Z

Restarting CI

0xc0170 · 2018-11-26T15:54:52Z

I restarted travis (already second time, having networking issues, not related)

donatieng · 2018-11-26T16:13:22Z

Third time's the charm 🤔 - restarted again

donatieng · 2018-11-26T16:17:00Z

😭

cmonr · 2018-11-26T16:19:05Z

@donatieng Somehow, it looks like Travis is searching for the head branch in ARMmbed's repo instead of @c1728p9' fork. Looking into what we can do for now.

Might end up either reopening the PR or cloning the contents of this PR into ARMmbed:mpu to allow the PR to progress.

donatieng · 2018-11-26T16:20:38Z

@cmonr Yes just noticed the same:

git clone --depth=50 --branch=mpu https://github.com/ARMmbed/mbed-os.git ARMmbed/mbed-os

cmonr · 2018-11-26T16:28:46Z

Trying a thing. Standby.

cmonr · 2018-11-26T17:57:25Z

Trying another thing.

mbed-ci · 2018-11-26T18:12:12Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 21
Build artifacts
Build logs

mattbrown015 · 2018-12-06T09:30:21Z

Hi,

I've just tried to update to the latest master and my code size has increased considerably because of this PR. I was somewhat surprised!

| Module                                                 |        .text |    .data |      .bss |
|--------------------------------------------------------|--------------|----------|-----------|
...
| mbed-os\hal\mpu\mbed_mpu_v7m.o                         |    315(+315) |    0(+0) |     0(+0) |
...
| mbed-os\platform\mbed_mpu_mgmt.o                       |    446(+446) |    0(+0) |     4(+4) |
| Subtotals                                              | 215926(+824) | 3296(+0) | 28488(+0) |
...
Total Static RAM memory (data + bss): 31784(+0) bytes
Total Flash memory (text + data): 219222(+824) bytes

An increase of 824 bytes feels like a lot, to me, for something I didn't ask for! :-;

I can mitigate this slightly by defining MBED_MPU_CUSTOM and removing the MPU device from my custom target:

| Module                                                 |        .text |    .data |      .bss |
|--------------------------------------------------------|--------------|----------|-----------|
...
| mbed-os\hal\mpu\mbed_mpu_v7m.o                         |      0(-315) |    0(+0) |     0(+0) |    ...
...
| mbed-os\platform\mbed_mpu_mgmt.o                       |     406(-40) |    0(+0) |     4(+0) |
...
| Subtotals                                              | 215590(-336) | 3296(+0) | 28488(+0) |
Total Static RAM memory (data + bss): 31784(+0) bytes
Total Flash memory (text + data): 218886(-336) bytes

But now it feels like I've got 406 byte of completely pointless code in mbed_mpu_mgmt.c.

I'm using the develop profile and errors are enabled. Some space could/would be saved by removing the error messages.

My target is a STM32L442 and luckily I've got some spare code flash at the moment. I've got more code to write though.

Obviously security is a really important subject but shouldn't it be possible to disable this feature so that it has zero impact??

Our device has propriety LoRaWAN and NFC interfaces (there's no network or Internet connection). I'm wondering how exposed we are to exploits such as buffer overrun attacks. (If we'd finished our software on time I never would have noticed this PR or realised that the ARM M4 had an MPU.) Perhaps security professionals would suggest this is exactly the attitude that has caused all the great security breaches! :-}

0xc0170 · 2018-12-06T11:18:50Z

Thanks @mattbrown015 for the feedback, we will review

cmonr · 2018-12-06T16:28:02Z

@mattbrown015 Would you mind migrating this to an issue, so that we can track and assign it properly?

bulislaw · 2018-12-10T09:29:07Z

@c1728p9 this memory increase is unacceptable, I'm considering actually switching this feature off by default for now. Please can you look at it ASAP.

kjbracey · 2018-12-10T09:52:10Z

That mbed_mpu_mgmt code should only be activated if someone trying to deactivate protection at runtime. The only in-tree user is FlashIAP, so I hope @mattbrown015's application is using nvstore or FlashIAPBlockDevice or something else depending on that. If not, something weird is happening.

I can knock up a quick PR addressing #9007 and also cutting down the memory use when it's enabled.

mattbrown015 · 2018-12-10T10:09:54Z

I hope @mattbrown015's application is using nvstore or FlashIAPBlockDevice or something else depending on that

We are using FlashIAP directly.

Up until #8335 I didn't even realise the M4 had a MPU so we're not explicitly doing anything about memory protection. Obviously that implies some issues on our part but we're trying to get this project finished so I think it is a bit late for us to make many changes.

c1728p9 added the do not merge label Oct 6, 2018

c1728p9 requested a review from TacoGrandeTX October 7, 2018 21:53

cmonr added the needs: review label Oct 8, 2018

cmonr requested review from SenRamakri, deepikabhavnani, dreemkiller, donatieng, bulislaw and a team October 8, 2018 16:09

SenRamakri reviewed Oct 8, 2018

View reviewed changes

c1728p9 requested a review from flit October 10, 2018 20:43

c1728p9 force-pushed the mpu branch from d321060 to 5b2a88a Compare October 10, 2018 21:35

c1728p9 changed the title ~~[RFC] MPU API~~ MPU API and design doc Oct 18, 2018

c1728p9 removed the do not merge label Oct 18, 2018

c1728p9 force-pushed the mpu branch from 4552631 to 33eb4d9 Compare October 20, 2018 00:24

deepikabhavnani reviewed Oct 23, 2018

View reviewed changes

TESTS/mbed_hal/mpu/main.cpp Show resolved Hide resolved

deepikabhavnani reviewed Oct 26, 2018

View reviewed changes

drivers/FlashIAP.cpp Outdated Show resolved Hide resolved

deepikabhavnani reviewed Oct 26, 2018

View reviewed changes

drivers/FlashIAP.cpp Outdated Show resolved Hide resolved

bulislaw approved these changes Oct 29, 2018

View reviewed changes

deepikabhavnani approved these changes Oct 29, 2018

View reviewed changes

c1728p9 force-pushed the mpu branch from 33eb4d9 to a3236f7 Compare October 29, 2018 22:15

donatieng approved these changes Oct 31, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Nov 26, 2018

cmonr closed this Nov 26, 2018

cmonr removed the needs: CI label Nov 26, 2018

cmonr reopened this Nov 26, 2018

cmonr added the needs: CI label Nov 26, 2018

cmonr closed this Nov 26, 2018

cmonr removed the needs: CI label Nov 26, 2018

cmonr mentioned this pull request Nov 26, 2018

MPU API (Reopened) #8871

Merged

cmonr removed priority: high risk: G release-version: 5.11.0-rc1 labels Nov 27, 2018

This was referenced Dec 7, 2018

MPU API Increases Code Size and Cannot be Completely Removed #9007

Closed

Build No Longer Fails if 'target.restrict_size' Exceeded #9012

Closed

cmonr mentioned this pull request Dec 12, 2018

Disable MPU on RT1050 due to memory map #9036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPU API #8335

MPU API #8335

c1728p9 commented Oct 6, 2018 •

edited

c1728p9 commented Oct 6, 2018

SenRamakri Oct 8, 2018

c1728p9 Oct 10, 2018

c1728p9 commented Oct 10, 2018

c1728p9 commented Oct 18, 2018

kjbracey commented Oct 18, 2018

c1728p9 commented Oct 19, 2018

cmonr commented Oct 19, 2018

c1728p9 commented Oct 20, 2018

bulislaw left a comment

bulislaw Oct 29, 2018

c1728p9 Oct 29, 2018

bulislaw Oct 29, 2018

c1728p9 Nov 2, 2018

deepikabhavnani left a comment

c1728p9 commented Oct 29, 2018

donatieng left a comment •

edited

cmonr commented Nov 26, 2018

0xc0170 commented Nov 26, 2018

donatieng commented Nov 26, 2018

donatieng commented Nov 26, 2018

cmonr commented Nov 26, 2018

donatieng commented Nov 26, 2018

cmonr commented Nov 26, 2018

cmonr commented Nov 26, 2018

mbed-ci commented Nov 26, 2018

mattbrown015 commented Dec 6, 2018

0xc0170 commented Dec 6, 2018

cmonr commented Dec 6, 2018

bulislaw commented Dec 10, 2018

kjbracey commented Dec 10, 2018

mattbrown015 commented Dec 10, 2018

MPU API #8335

MPU API #8335

Conversation

c1728p9 commented Oct 6, 2018 • edited

Description

Pull request type

c1728p9 commented Oct 6, 2018

SenRamakri Oct 8, 2018

Choose a reason for hiding this comment

c1728p9 Oct 10, 2018

Choose a reason for hiding this comment

c1728p9 commented Oct 10, 2018

c1728p9 commented Oct 18, 2018

kjbracey commented Oct 18, 2018

c1728p9 commented Oct 19, 2018

cmonr commented Oct 19, 2018

c1728p9 commented Oct 20, 2018

bulislaw left a comment

Choose a reason for hiding this comment

bulislaw Oct 29, 2018

Choose a reason for hiding this comment

c1728p9 Oct 29, 2018

Choose a reason for hiding this comment

bulislaw Oct 29, 2018

Choose a reason for hiding this comment

c1728p9 Nov 2, 2018

Choose a reason for hiding this comment

deepikabhavnani left a comment

Choose a reason for hiding this comment

c1728p9 commented Oct 29, 2018

donatieng left a comment • edited

Choose a reason for hiding this comment

cmonr commented Nov 26, 2018

0xc0170 commented Nov 26, 2018

donatieng commented Nov 26, 2018

donatieng commented Nov 26, 2018

cmonr commented Nov 26, 2018

donatieng commented Nov 26, 2018

cmonr commented Nov 26, 2018

cmonr commented Nov 26, 2018

mbed-ci commented Nov 26, 2018

Test run: SUCCESS

mattbrown015 commented Dec 6, 2018

0xc0170 commented Dec 6, 2018

cmonr commented Dec 6, 2018

bulislaw commented Dec 10, 2018

kjbracey commented Dec 10, 2018

mattbrown015 commented Dec 10, 2018

c1728p9 commented Oct 6, 2018 •

edited

donatieng left a comment •

edited