pkg/xipfs: add MPU memory isolation to file execution #21760
Conversation
GGuche-2XS-Univ-Lille
requested review from
bergzand and
kaspar030
as code owners
github-actions
bot
added
Platform: ARM
crasbe
added
Type: enhancement
|
Where can I look at the code of examples/advanced/xipfs/dumper.fae? |
|
@Teufelchen1 Sorry for the delay. You can find the source of the memory dumper here. It comes with the tools for building : the sources are compiled to an elf file, which is processed in order to build a fae file. For more details on the tools and process, please go to the XIPFS Format repository |
There was a problem hiding this comment.
I don't really like the modifications to the common Cortex-M files to be honest.
This is pretty application specific and does not really belong here.
I acknowledge that currently there are no provisions to change the mem_manage handler or add cases to the SVC dispatcher, but I'm afraid that this is not a good way yet.
|
@crasbe XIPFS is designed for ARM platforms with addressable flash memory and ARMv7-M MPU. As such, we don't target any board specifically but more a family of boards sharing these features. Furthermore, we need to customize the memory fault handler :
We also need to add svc cases :
We would have liked to come with a better proposal, satisfying all of us, but to the best of our knowledge, we need at least these two modifications to make the MPU memory isolation work for the targetted platforms. |
|
@crasbe To be less invasive, would you agree on using callbacks in _svc_dispatch and mem_manage_default ? For example, __svc_dispatch in thread_arch.c : typedef void (*svc_dispatch_handler_t)(unsigned int svc_number, unsigned int *svc_args);
svc_dispatch_handler_t _svc_dispatch_handler = NULL;
...
switch (svc_number) {
case 1: /* SVC number used by cpu_switch_context_exit */
SCB->ICSR = SCB_ICSR_PENDSVSET_Msk;
break;
default:
if (_svc_dispatch_handler != NULL) {
_svc_dispatch_handler(svc_number, svc_args);
} else {
DEBUG("svc: unhandled SVC #%u\n", svc_number);
}
break;
}
...And for mem_manage_default in vector_cortexm.c, typedef int (*mem_manage_handler_t)(void);
mem_manage_handler_t mem_manage_handler = NULL;
...
if (mem_manage_handler != NULL) {
if (mem_manage_handler() == 1) {
return;
}
}
core_panic();The callbacks would be be set to valid handlers before calling xipfs_safe_execv. Is it more compatible with what you have in mind ? |
|
Yes, this looks good. I had something similar in mind, but I wasn't able to articulate a proper proposal. 👍 |
|
Perhaps for the SVC we could also modify it like this: typedef void (*svc_dispatch_handler_t)(unsigned int svc_number, unsigned int *svc_args);
svc_dispatch_handler_t _svc_dispatch_handler = NULL;
...
switch (svc_number) {
case 1: /* SVC number used by cpu_switch_context_exit */
SCB->ICSR = SCB_ICSR_PENDSVSET_Msk;
break;
default:
/* call user defined SVC dispatch handler */
if (_svc_dispatch_handler != NULL) {
if (_svc_dispatch_handler(svc_number, svc_args) > 0) {
break;
}
}
DEBUG("svc: unhandled SVC #%u\n", svc_number);
break;
}
...And |
|
@crasbe Did the modifications. There are some doccheck failing in static tests, but regarding to code not modified by the PR. |
| * @retval < 0 on NULL handler or if a handler has already been set | ||
| * @retval >= 0 otherwise | ||
| */ | ||
| int set_svc_dispatch_handler(svc_dispatch_handler_t handler); |
There was a problem hiding this comment.
Just to be sure I'm not overlooking something:
Why does this need to be set dynamically at run time, when there is only a single callback slot for it anyway?
Why not just provide a weak no-op default handler and register at link time by providing a handler with a strong symbol?
There was a problem hiding this comment.
@maribu That's an interesting question.
In the context of xipfs, we are mostly dealing with short lifecycle execution.
The rationale for the dynamic setting is to scale among usages; other software could use this mechanism without collisions.
Now the weak function is attractive, because of its simplicity, but only one actor could use it, here xipfs.
While we are at it, do you prefer to handle this modification in a separate PR too ?
cpu/cortexm_common/vectors_cortexm.c
Outdated
|
|
||
| #endif | ||
|
|
||
| int set_memory_manage_handler(mem_manage_handler_t handler) { |
There was a problem hiding this comment.
Does it make sense to set this at run-time or would it be sufficient if this was a weak symbol that gets replaced at link-time?
There was a problem hiding this comment.
Hah, I was faster! Quick-draw batch for me 🤣
There was a problem hiding this comment.
@crasbe @maribu @benpicco
To sum it up :
- thread_arch.c
__attribute__((weak)) int svc_dispatch_handler(unsigned int svc_number, unsigned int *svc_args) {
return -ENOTSUP;
}
...
switch (svc_number) {
case 1: /* SVC number used by cpu_switch_context_exit */
SCB->ICSR = SCB_ICSR_PENDSVSET_Msk;
break;
default:
if (_svc_dispatch_handler(svc_number, svc_args) >= 0) {
return;
}
DEBUG("svc: unhandled SVC #%u\n", svc_number);
break;
}
- vector_cortexm.c:
__attribute__((weak)) int mem_manage_handler(void) {
return 0;
}
void mem_manage_default(void)
{
if (_mem_manage_handler() == 1) {
return;
}
core_panic(PANIC_MEM_MANAGE, "MEM MANAGE HANDLER");
}
The all above in a separate PR to keep things clean ?
What would be the PR title then ?
There was a problem hiding this comment.
I wouldn't insist for separating that out, this PR is not too large anyway. IMO: Whatever works best for you.
| #ifdef XIPFS_ENABLE_SAFE_EXEC_SUPPORT | ||
| int ret = (exe_filename_arg_pos == 2) ? | ||
| xipfs_extended_driver_safe_execv(exe_filename, execute_file_handler_args) | ||
| : xipfs_extended_driver_execv(exe_filename, execute_file_handler_args); | ||
| #else | ||
| int ret = xipfs_extended_driver_execv(exe_filename, execute_file_handler_args); | ||
| #endif |
There was a problem hiding this comment.
| #ifdef XIPFS_ENABLE_SAFE_EXEC_SUPPORT | |
| int ret = (exe_filename_arg_pos == 2) ? | |
| xipfs_extended_driver_safe_execv(exe_filename, execute_file_handler_args) | |
| : xipfs_extended_driver_execv(exe_filename, execute_file_handler_args); | |
| #else | |
| int ret = xipfs_extended_driver_execv(exe_filename, execute_file_handler_args); | |
| #endif | |
| if (IS_DEFINED(XIPFS_ENABLE_SAFE_EXEC_SUPPORT) && exe_filename_arg_pos == 2) { | |
| int ret = xipfs_extended_driver_safe_execv(exe_filename, execute_file_handler_args); | |
| } | |
| else { | |
| int ret = xipfs_extended_driver_execv(exe_filename, execute_file_handler_args); | |
| } |
Perhaps a little easier to read.
There was a problem hiding this comment.
I may be wrong but I don't think this would compile successfully.
retwould be out of scope for usage later on.
There was a problem hiding this comment.
Why should it be out of scope? Both code paths have int ret.
There was a problem hiding this comment.
Yes, they both have ìnt ret, but past line 69 or line 72, ret would be out of scope of the selected if branch.
I tried to compile the following snippet :
#include <stdlib.h>
int main(int argc, const char *argv[]) {
if (argc > 1) {
int ret = argc;
} else {
int ret = -argc;
}
return ret;
}
And I got :
gcc -std=c99 -Wall -Werror -o test test.c
test.c: In function ‘main’:
test.c:7:13: error: unused variable ‘ret’ [-Werror=unused-variable]
7 | int ret = argc;
| ^~~
test.c:9:13: error: unused variable ‘ret’ [-Werror=unused-variable]
9 | int ret = -argc;
| ^~~
test.c:12:12: error: ‘ret’ undeclared (first use in this function)
12 | return ret;
| ^~~
test.c:12:12: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
Am I missing something ?
6 participants
Contribution description
The xipfs filesystem is able to perform in-place execution.
The aim of this PR is to add hardware MPU-based memory isolation to the xipfs' execution capability.
With it, an executable fae file is run in non-privileged mode and can only access to its legitimate memory areas, thanks to MPU regions definitions of TEXT, DATA and STACK segments.
TEXT is read-only, but DATA and STACK are read-write.
This generic hardware approach can handle all executables, with almost zero time overhead.
MPU regions identifiers have been chosen with respect to RIOT's MPU safe guard and MPU no exec functionalities.
This feature should be available for all platforms with an ARM cpu and an ARMv7-M MPU, but has been only tested on QORVO DWM1001 for now.
Testing procedure
For this board, a memory dumper is provided in xipfs' example to illustrate simple/safe memory dumps of legit/non-legit ram/rom.
On a sidenote, illegitimate RAM address has been chosen to be equal to 0x20000020, because the first 32 bytes of the RAM are in read-only MPU stack guard region.
In safe execution mode, the dumper would display the 32 bytes starting at 0x20000000 before the MPU detects an illegal access, which could lead to deceptive conclusions.