Add optional support for executable space protections

[nmeum]

Contribution description

Executable space protections are a protection mechanism for
operating system security which mitigate exploitation of buffer
overflows. The exploit payload, used for exploiting buffer overflows, is
often placed in the buffer itself. As an example, consider the exploit
for sock_dns from #10739.

Considering a stack-based buffer overflow, the attacker can simply
overwrite the function return address with the address of the buffer
thereby causing the code contained in it to be executed. I've worked on
techniques for preventing the overwrite of the function return address
(#13119, #13175) but these can be circumvented. As such, conventional
operating system also mark the text and data segment non-executable to
prevent attackers from placing exploit payloads there (executable space
protections). Enforcing this efficiently requires a memory protection or
memory management unit.

I noticed recently that RIOT already supports the Cortex-M4 MPU. Using
this MPU on the nucleo-f401re I came up with an implementation of
executable space protections for RIOT. The implementation is entirely
optional and must be activated explicitly by using the mpu_noexec_ram
pseudomodule. When activated, the entire SRAM section is marked as non
executable, reads- and writes are still allowed.

Kudos to @pyropeter for helping out with this.

Testing procedure

The changes proposed here also include a test for this pseudo module.
The test is further described in tests/mpu_noexec_ram/README.md. The
test successfully passed with BOARD=nucleo-f401re.

Issues/PRs references

Executable space protections can be circumvented through code-reuse
attacks (e.g. return-to-libc or ROP in the general case). These attacks
can be mitigated through address randomization techniques such as ASLR.
While ASLR is not deemed employable on constrained devices, a different
address randomization technique, often referred to as link time reordering is.

This technique has been implemented by myself for RIOT in #13176. I
would highly recommended that this is merged as well though this PR does
not technically depend on it.

[nmeum]

Opps, didn't intended to request a review from so many people, sorry.

[benpicco]

Opps, didn't intended to request a review from so many people, sorry.

Not your fault, the CODEOWNERS file is responsible for that.
If your PR touches a file or directory with an 'owner' attached, they will be notified.
Currently we have many owners for the entire tests/ directory - we'll see if this is useful or not (by seeing if those owners submit PRs for more fine grained ownership).

[kaspar030]

Looks good. Note to myself: need to document the use of MPU regions somewhere.

Can you provide a test script that catches the panic (and a possible "I HAVE NOT CRASHED" which would be reached if the MPU did not work as intended)?

[nmeum]

Can you provide a test script that catches the panic (and a possible "I HAVE NOT CRASHED" which would be reached if the MPU did not work as intended)?

This is technically possible, though the latter will be difficult to do sanely since the code in the buffer on the stack would than need to return control graceful back to the C code, jump to a specific C function or something like that. Also the former would require installing a custom memory management interrupt handler.

Maybe that's an improvement which can later be made in a separate PR. Possibly with also updating the tests/mpu_stack_guard accordingly? Would that work for you?

[kaspar030]

Maybe that's an improvement which can later be made in a separate PR.

Thing is, we don't merge test applications anymore which don't come with a test script.
It would be fine to wait for the correct (expected) panic and timeout otherwise.

[nmeum]

Thing is, we don't merge test applications anymore which don't come with a test script.

Alright, converted the mpu_noexec_ram test to an automated test. The test script checks whether the memory management interrupt handler is invoked correctly after jumping to the stack-based buffer.

[benpicco]

unsigned char is uint8_t though.

[nmeum]

unsigned char is uint8_t though.

Let's just get rid of the memset it's cleaner without anyhow.

[nmeum]

Hm? Why was the label removed? The cppcheck warning has been fixed successfully.

[benpicco]

Until #13456 is merged all builds will fail now.

[nmeum]

Rebased this against #13391. All tests pass on the CI now. Merge?

[benpicco]

Since this adds a new test to CI, we should let CI run the test first 😉
But looks good, if everything is green we should merge this!

[benpicco]

Did you test this with / without the mpu_stack_guard module?

[benpicco]

run_test/examples/suit_update/nrf52dk:gnu keeps failing, but that's unrelated to this PR.

Nice addition, Murdock is happy too.

[nmeum]

Did you test this with / without the mpu_stack_guard module?

Yes, I did. See 59676a1