Cortex-M MPU driver and pseudo-module for guarding against stack overflows by hexluthor · Pull Request #5564 · RIOT-OS/RIOT

hexluthor · 2016-06-21T16:47:00Z

A 32-byte region at the bottom of the running stack is made read-only by the MPU. If a stack overflow occurs, a memory management exception is triggered before any data below the stack is overwritten. Depending on the alignment of the individual stacks, this comes at a cost of 32-63 bytes per thread. For this reason, the feature is an optional pseudo-module and is not enabled by default.

As a future PR, I'd like to define an attribute for all statically-allocated stacks that instructs the linker to align them to a 32-byte boundary if this module is used, and would otherwise be just a placeholder or "no-op".

kaspar030 · 2016-06-21T17:04:54Z

nice!

kaspar030 · 2016-06-21T17:05:12Z

Makefile.pseudomodules

 PSEUDOMODULES += lwip_tcp
 PSEUDOMODULES += lwip_udp
 PSEUDOMODULES += lwip_udplite
+PSEUDOMODULES += mpu_stack_guard


I considered that, but wanted to leave room for other MPUs to be used in the same way.

jnohlgard · 2016-06-21T23:51:35Z

Cool! We need a test though, do you have an application for testing? Otherwise we can write a recursive function which will run out of stack and put it under tests/stack_overflow or something.
The same test would be interesting for the isr stack since the cortexm isr stack relocation pr was merged.

kaspar030 · 2016-06-22T08:03:46Z

cpu/cortexm_common/mpu.c

+    /* Enable the memory fault exception */
+    SCB->SHCSR |= SCB_SHCSR_MEMFAULTENA_Msk;
+
+    return 0;


This returns 0 on success, 1 on "not available", right?
I find the opposite more intuitive.

hexluthor · 2016-06-22T15:27:55Z

@gebart I had a feeling someone would ask for a test app. I don't have one yet but will think about it.

jnohlgard · 2016-06-22T17:57:31Z

@kaspar030 @hexluthor
wouldn't it be more consistent to use negative return values for errors, like in the periph drivers do it?

jnohlgard · 2016-06-23T07:32:02Z

Suggestion for a follow up PR: Mark the running stack non-executable.

hexluthor · 2016-06-23T15:20:35Z

@gebart RIOT seems pretty inconsistent about this but what you describe is also the Linux coding style so I'm going with it.

hexluthor · 2016-06-28T17:53:12Z

Here's a test app too. Tested on cc2538dk.

kYc0o · 2016-07-22T12:56:13Z

Great! tested on arduino-due and it works like a charm, so ACK. Please squash and set the flag for Murdock if possible.

OlegHahm · 2016-07-22T13:09:28Z

@gebart RIOT seems pretty inconsistent about this

Can you elaborate on where you found these inconsistencies?

kYc0o · 2016-07-25T12:25:49Z

We can wait for the next release on this one...

hexluthor · 2016-07-25T16:27:19Z

@OlegHahm I did something like ack-grep 'return.*(success|fail)'. In most cases functions return 0 on success, but there are some that return 1. Didn't really investigate beyond the grep.

OlegHahm · 2016-07-26T19:29:42Z

I would guess that in some cases (maybe not all) this is okay, according to https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions#return-values

miri64 · 2016-08-31T14:36:29Z

Is there anything to discuss in this PR (it was ACKed in #5564 (comment))

kaspar030 · 2016-08-31T14:57:00Z

core/sched.c

+    mpu_configure(
+        1,                                                /* MPU region 1 */
+        (uintptr_t)sched_active_thread->stack_start + 31, /* Base Address (rounded up) */
+        MPU_ATTR(1, AP_RO_RO, 0, 1, 0, 1, MPU_SIZE_32B)   /* Attributes and Size */


This prevents overwriting the lowest byte of the stack?

please ignore the noise, just read the PR description again. ;)

hexluthor · 2016-09-27T18:43:41Z

I see no reason to not merge it (but I'm also the author).

kaspar030 · 2016-09-27T18:45:51Z

cpu/cortexm_common/include/mpu.h

+ *
+ * @return combined region attribute word
+ */
+#define MPU_ATTR(xn, ap, tex, c, b, s, size) ( \


Conventions say multiline macros should be static inline functions. Would that optimize to the same code in this case?

I prefer the macro but yield to convention. They are binary equivalent for tests/mpu_stack_guard.

kYc0o · 2016-10-19T13:53:10Z

There's just the question about the returns, if 0 or negative values in case of failure.

Rebase if possible and maybe we should let Murdock look into it again, because this PR is kind of old and several things were merged in the meantime.

miri64 · 2016-10-19T13:57:28Z

@kYc0o

Rebase if possible and maybe we should let Murdock look into it again, because this PR is kind of old and several things were merged in the meantime.

Murdock seemed to be happy last time I tried it and there are no conflicts so why rebase?

kYc0o · 2016-10-19T14:04:18Z

Oh, I didn't know you tried afterwards. It should be OK then. Yeah for the conflicts I agree too. So just this "inconsistency" about the return on failure but the arguments are "sometimes 0 is valid, sometimes not", so that's maybe another discussion. I propose to merge this as is and when an agreement about what are the "right" return errors we can change them all, including this one.

miri64 · 2016-10-19T14:09:47Z

Oh, I didn't know you tried afterwards.

I'm not sure this is clear now: I did not test this PR, I just observed Murdock's results ;-).

kYc0o · 2016-10-19T14:13:31Z

Yes, it was me who tested successfully ;)

hexluthor · 2016-10-19T15:18:46Z

Ok I

Rebased onto master.
Whitelisted new Cortex-M boards arduino-duemilanove, arduino-uno, arduino-zero, nucleo-f207, nucleo-f446, sodaq-autonomo in tests/mpu_stack_guard.
Corrected a doxygen typo about return values. Besides mpu_enabled(), all functions return 0 on success, -1 on failure.

kYc0o · 2016-10-24T15:40:02Z

It seems OK for me now, I'd re-ACK but I'd like the second commit divided into several for the files you're changing, because now it changes several files and the commit message it's not prefixed.

hexluthor · 2016-10-24T16:22:25Z

@kYc0o ok I committed the test app separately. Not sure what else could be divided.

kYc0o · 2016-10-24T16:29:51Z

Maybe the commits for core and cpu... I can also see some commit on another test but I don't know if split it too... Since this PR is touching "delicate" files I'd like to have them in a separated commit.

triggers an exception during stack overflow, but at a cost of 32-63 bytes of RAM per thread.

kYc0o

ACK

kYc0o · 2016-10-25T09:56:34Z

and go!

hexluthor added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers labels Jun 21, 2016

hexluthor added this to the Release 2016.07 milestone Jun 21, 2016

hexluthor mentioned this pull request Jun 21, 2016

mpu: Use memory protection unit (MPU) for protecting memory accesses #3284

Closed

kaspar030 reviewed Jun 21, 2016
View reviewed changes

kaspar030 reviewed Jun 22, 2016
View reviewed changes

hexluthor added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jun 29, 2016

kYc0o self-assigned this Jul 25, 2016

kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 25, 2016

hexluthor force-pushed the mpu-rebase2 branch from 3b07f15 to 524d1f9 Compare July 25, 2016 16:07

kaspar030 reviewed Aug 31, 2016
View reviewed changes

kaspar030 reviewed Sep 27, 2016

View reviewed changes

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 30, 2016

miri64 mentioned this pull request Oct 3, 2016

dist: tools: jlink.sh: use printf for JLINK_*_FLASH to allow use of backslash #5900

Merged

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 4, 2016

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 19, 2016

hexluthor force-pushed the mpu-rebase2 branch from b7723b9 to 9926991 Compare October 19, 2016 15:09

cpu/cortexm_common: add Memory Protection Unit (MPU) driver

b7bd6b3

hexluthor force-pushed the mpu-rebase2 branch from 9926991 to 684c291 Compare October 19, 2016 15:13

hexluthor force-pushed the mpu-rebase2 branch from 684c291 to 9698ddd Compare October 24, 2016 16:20

hexluthor added 3 commits October 24, 2016 12:36

core: save the stack_start in the thread context for mpu_stack_guard

28a7ddc

add mpu_stack_guard pseudomodule (just Cortex-M for now)

22b5de8

triggers an exception during stack overflow, but at a cost of 32-63 bytes of RAM per thread.

add tests/mpu_stack_guard for testing psuedomodule

0c588e7

hexluthor force-pushed the mpu-rebase2 branch from 9698ddd to 0c588e7 Compare October 24, 2016 16:42

kYc0o approved these changes Oct 25, 2016

View reviewed changes

kYc0o merged commit b2bed29 into RIOT-OS:master Oct 25, 2016

kaspar030 mentioned this pull request Oct 27, 2016

cpu: cortexm_common: fix compile warning #6009

Merged

Conversation

hexluthor commented Jun 21, 2016

Uh oh!

kaspar030 commented Jun 21, 2016

Uh oh!

kaspar030 Jun 21, 2016

Choose a reason for hiding this comment

Uh oh!

hexluthor Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Jun 21, 2016

Uh oh!

kaspar030 Jun 22, 2016

Choose a reason for hiding this comment

Uh oh!

hexluthor commented Jun 22, 2016

Uh oh!

jnohlgard commented Jun 22, 2016

Uh oh!

jnohlgard commented Jun 23, 2016

Uh oh!

hexluthor commented Jun 23, 2016

Uh oh!

hexluthor commented Jun 28, 2016

Uh oh!

kYc0o commented Jul 22, 2016

Uh oh!

OlegHahm commented Jul 22, 2016

Uh oh!

kYc0o commented Jul 25, 2016

Uh oh!

hexluthor commented Jul 25, 2016

Uh oh!

OlegHahm commented Jul 26, 2016

Uh oh!

miri64 commented Aug 31, 2016

Uh oh!

kaspar030 Aug 31, 2016

Choose a reason for hiding this comment

Uh oh!

kaspar030 Aug 31, 2016

Choose a reason for hiding this comment

Uh oh!

hexluthor commented Sep 27, 2016

Uh oh!

kaspar030 Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

hexluthor Sep 29, 2016

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Oct 19, 2016

Uh oh!

miri64 commented Oct 19, 2016

Uh oh!

kYc0o commented Oct 19, 2016

Uh oh!

miri64 commented Oct 19, 2016

Uh oh!

kYc0o commented Oct 19, 2016

Uh oh!

hexluthor commented Oct 19, 2016

Uh oh!

kYc0o commented Oct 24, 2016

Uh oh!

hexluthor commented Oct 24, 2016

Uh oh!

kYc0o commented Oct 24, 2016

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Oct 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone