From 6cf92fd748f7de7f13454ec78329085d08b7f6f7 Mon Sep 17 00:00:00 2001 From: tychovrahe Date: Tue, 9 Aug 2022 19:16:54 +0200 Subject: [PATCH] feat(core): Introduce stack overflow detection by moving stack to the start of RAM --- core/.changelog.d/2427.added | 1 + core/embed/bootloader/.changelog.d/2427.added | 1 + core/embed/bootloader/memory.ld | 9 +++++---- core/embed/firmware/main.c | 8 ++++++-- core/embed/firmware/memory_1.ld | 13 +++++++------ core/embed/firmware/memory_1_min.ld | 13 +++++++------ core/embed/firmware/memory_T.ld | 13 +++++++------ core/embed/firmware/startup.S | 14 +++++++++++++- core/embed/trezorhal/supervise.h | 16 +++++++++++----- 9 files changed, 58 insertions(+), 30 deletions(-) create mode 100644 core/.changelog.d/2427.added create mode 100644 core/embed/bootloader/.changelog.d/2427.added diff --git a/core/.changelog.d/2427.added b/core/.changelog.d/2427.added new file mode 100644 index 000000000..505358bb6 --- /dev/null +++ b/core/.changelog.d/2427.added @@ -0,0 +1 @@ +Add stack overflow detection diff --git a/core/embed/bootloader/.changelog.d/2427.added b/core/embed/bootloader/.changelog.d/2427.added new file mode 100644 index 000000000..505358bb6 --- /dev/null +++ b/core/embed/bootloader/.changelog.d/2427.added @@ -0,0 +1 @@ +Add stack overflow detection diff --git a/core/embed/bootloader/memory.ld b/core/embed/bootloader/memory.ld index bc3dc77c8..cf280882f 100644 --- a/core/embed/bootloader/memory.ld +++ b/core/embed/bootloader/memory.ld @@ -8,7 +8,7 @@ MEMORY { SRAM (wal) : ORIGIN = 0x20000000, LENGTH = 192K } -main_stack_base = ORIGIN(CCMRAM) + LENGTH(CCMRAM); /* 8-byte aligned full descending stack */ +main_stack_base = ORIGIN(CCMRAM) + SIZEOF(.stack) ; /* 8-byte aligned full descending stack */ /* used by the startup code to populate variables used by the C code */ data_lma = LOADADDR(.data); @@ -39,6 +39,10 @@ SECTIONS { . = ALIGN(512); } >FLASH AT>FLASH + .stack : ALIGN(8) { + . = 16K; /* Exactly 16K allocated for stack. Overflow causes MemManage fault (when using MPU). */ + } >CCMRAM + .data : ALIGN(4) { *(.data*); . = ALIGN(512); @@ -54,7 +58,4 @@ SECTIONS { . = ALIGN(4); } >SRAM - .stack : ALIGN(8) { - . = 4K; /* this acts as a build time assertion that at least this much memory is available for stack use */ - } >CCMRAM } diff --git a/core/embed/firmware/main.c b/core/embed/firmware/main.c index d3069837f..147441c0b 100644 --- a/core/embed/firmware/main.c +++ b/core/embed/firmware/main.c @@ -129,7 +129,7 @@ int main(void) { // Stack limit should be less than real stack size, so we have a chance // to recover from limit hit. mp_stack_set_top(&_estack); - mp_stack_set_limit((char *)&_estack - (char *)&_heap_end - 1024); + mp_stack_set_limit((char *)&_estack - (char *)&_sstack - 1024); #if MICROPY_ENABLE_PYSTACK static mp_obj_t pystack[1024]; @@ -179,10 +179,14 @@ void HardFault_Handler(void) { error_shutdown("Internal error", "(HF)", NULL, NULL); } -void MemManage_Handler(void) { +void MemManage_Handler_MM(void) { error_shutdown("Internal error", "(MM)", NULL, NULL); } +void MemManage_Handler_SO(void) { + error_shutdown("Internal error", "(SO)", NULL, NULL); +} + void BusFault_Handler(void) { error_shutdown("Internal error", "(BF)", NULL, NULL); } diff --git a/core/embed/firmware/memory_1.ld b/core/embed/firmware/memory_1.ld index b4bb47f01..625c2c641 100644 --- a/core/embed/firmware/memory_1.ld +++ b/core/embed/firmware/memory_1.ld @@ -7,7 +7,8 @@ MEMORY { SRAM (wal) : ORIGIN = 0x20000000, LENGTH = 128K } -main_stack_base = ORIGIN(SRAM) + LENGTH(SRAM); /* 8-byte aligned full descending stack */ +main_stack_base = ORIGIN(SRAM) + SIZEOF(.stack); /* 8-byte aligned full descending stack */ +_sstack = ORIGIN(SRAM); _estack = main_stack_base; /* used by the startup code to populate variables used by the C code */ @@ -52,6 +53,10 @@ SECTIONS { . = ALIGN(4); } >FLASH AT>FLASH + .stack : ALIGN(8) { + . = 16K; /* Exactly 16K allocated for stack. Overflow causes MemManage fault (when using MPU). */ + } >SRAM + .data : ALIGN(4) { *(.data*); . = ALIGN(512); @@ -64,10 +69,6 @@ SECTIONS { .heap : ALIGN(4) { . = 37K; /* this acts as a build time assertion that at least this much memory is available for heap use */ - . = ABSOLUTE(sram_end - 16K); /* this explicitly sets the end of the heap effectively giving the stack at most 16K */ - } >SRAM - - .stack : ALIGN(8) { - . = 4K; /* this acts as a build time assertion that at least this much memory is available for stack use */ + . = ABSOLUTE(sram_end - 8); /* this explicitly sets the end of the heap, T1 bootloader had 8 bytes reserved at end */ } >SRAM } diff --git a/core/embed/firmware/memory_1_min.ld b/core/embed/firmware/memory_1_min.ld index b4bb47f01..625c2c641 100644 --- a/core/embed/firmware/memory_1_min.ld +++ b/core/embed/firmware/memory_1_min.ld @@ -7,7 +7,8 @@ MEMORY { SRAM (wal) : ORIGIN = 0x20000000, LENGTH = 128K } -main_stack_base = ORIGIN(SRAM) + LENGTH(SRAM); /* 8-byte aligned full descending stack */ +main_stack_base = ORIGIN(SRAM) + SIZEOF(.stack); /* 8-byte aligned full descending stack */ +_sstack = ORIGIN(SRAM); _estack = main_stack_base; /* used by the startup code to populate variables used by the C code */ @@ -52,6 +53,10 @@ SECTIONS { . = ALIGN(4); } >FLASH AT>FLASH + .stack : ALIGN(8) { + . = 16K; /* Exactly 16K allocated for stack. Overflow causes MemManage fault (when using MPU). */ + } >SRAM + .data : ALIGN(4) { *(.data*); . = ALIGN(512); @@ -64,10 +69,6 @@ SECTIONS { .heap : ALIGN(4) { . = 37K; /* this acts as a build time assertion that at least this much memory is available for heap use */ - . = ABSOLUTE(sram_end - 16K); /* this explicitly sets the end of the heap effectively giving the stack at most 16K */ - } >SRAM - - .stack : ALIGN(8) { - . = 4K; /* this acts as a build time assertion that at least this much memory is available for stack use */ + . = ABSOLUTE(sram_end - 8); /* this explicitly sets the end of the heap, T1 bootloader had 8 bytes reserved at end */ } >SRAM } diff --git a/core/embed/firmware/memory_T.ld b/core/embed/firmware/memory_T.ld index 4adb25246..e50df36cd 100644 --- a/core/embed/firmware/memory_T.ld +++ b/core/embed/firmware/memory_T.ld @@ -9,7 +9,8 @@ MEMORY { SRAM (wal) : ORIGIN = 0x20000000, LENGTH = 192K } -main_stack_base = ORIGIN(SRAM) + LENGTH(SRAM); /* 8-byte aligned full descending stack */ +main_stack_base = ORIGIN(SRAM) + SIZEOF(.stack); /* 8-byte aligned full descending stack */ +_sstack = ORIGIN(SRAM); _estack = main_stack_base; /* used by the startup code to populate variables used by the C code */ @@ -62,6 +63,10 @@ SECTIONS { . = ALIGN(512); } >FLASH AT>FLASH + .stack : ALIGN(8) { + . = 16K; /* Exactly 16K allocated for stack. Overflow causes MemManage fault (when using MPU). */ + } >SRAM + .data : ALIGN(4) { *(.data*); . = ALIGN(512); @@ -74,10 +79,6 @@ SECTIONS { .heap : ALIGN(4) { . = 37K; /* this acts as a build time assertion that at least this much memory is available for heap use */ - . = ABSOLUTE(sram_end - 16K); /* this explicitly sets the end of the heap effectively giving the stack at most 16K */ - } >SRAM - - .stack : ALIGN(8) { - . = 4K; /* this acts as a build time assertion that at least this much memory is available for stack use */ + . = ABSOLUTE(sram_end); /* this explicitly sets the end of the heap */ } >SRAM } diff --git a/core/embed/firmware/startup.S b/core/embed/firmware/startup.S index 34c2a57a8..a82ffa5e4 100644 --- a/core/embed/firmware/startup.S +++ b/core/embed/firmware/startup.S @@ -16,7 +16,7 @@ reset_handler: ldr r1, =0x08010400 // r1 = FLASH_APP_START str r1, [r0] // assign - ldr r0, =_estack - 8 // r0 = stack pointer, T1 bootloader had 8 bytes reserved at end + ldr r0, =_estack // r0 = stack pointer msr msp, r0 // set stack pointer dsb isb @@ -59,4 +59,16 @@ reset_handler: b shutdown_privileged + .global MemManage_Handler + .type MemManage_Handler, STT_FUNC +MemManage_Handler: + ldr r2, =_sstack + mrs r1, msp + ldr r0, =_estack + msr msp, r0 + cmp r1, r2 + IT lt + bllt MemManage_Handler_SO + bl MemManage_Handler_MM + .end diff --git a/core/embed/trezorhal/supervise.h b/core/embed/trezorhal/supervise.h index 851c61aaa..93769b012 100644 --- a/core/embed/trezorhal/supervise.h +++ b/core/embed/trezorhal/supervise.h @@ -16,8 +16,14 @@ static inline uint32_t is_mode_unprivileged(void) { return r0 & 1; } +static inline uint32_t is_mode_handler(void) { + uint32_t r0; + __asm__ volatile("mrs %0, ipsr" : "=r"(r0)); + return (r0 & 0x1FF) != 0; +} + static inline void svc_enableIRQ(uint32_t IRQn) { - if (is_mode_unprivileged()) { + if (is_mode_unprivileged() && !is_mode_handler()) { register uint32_t r0 __asm__("r0") = IRQn; __asm__ __volatile__("svc %0" ::"i"(SVC_ENABLE_IRQ), "r"(r0) : "memory"); } else { @@ -26,7 +32,7 @@ static inline void svc_enableIRQ(uint32_t IRQn) { } static inline void svc_disableIRQ(uint32_t IRQn) { - if (is_mode_unprivileged()) { + if (is_mode_unprivileged() && !is_mode_handler()) { register uint32_t r0 __asm__("r0") = IRQn; __asm__ __volatile__("svc %0" ::"i"(SVC_DISABLE_IRQ), "r"(r0) : "memory"); } else { @@ -35,7 +41,7 @@ static inline void svc_disableIRQ(uint32_t IRQn) { } static inline void svc_setpriority(uint32_t IRQn, uint32_t priority) { - if (is_mode_unprivileged()) { + if (is_mode_unprivileged() && !is_mode_handler()) { register uint32_t r0 __asm__("r0") = IRQn; register uint32_t r1 __asm__("r1") = priority; __asm__ __volatile__("svc %0" ::"i"(SVC_SET_PRIORITY), "r"(r0), "r"(r1) @@ -46,14 +52,14 @@ static inline void svc_setpriority(uint32_t IRQn, uint32_t priority) { } static inline void svc_shutdown(void) { - if (is_mode_unprivileged()) { + if (is_mode_unprivileged() && !is_mode_handler()) { __asm__ __volatile__("svc %0" ::"i"(SVC_SHUTDOWN) : "memory"); } else { shutdown_privileged(); } } static inline void svc_reboot_to_bootloader(void) { - if (is_mode_unprivileged()) { + if (is_mode_unprivileged() && !is_mode_handler()) { __asm__ __volatile__("svc %0" ::"i"(SVC_REBOOT_TO_BOOTLOADER) : "memory"); } else { reboot_to_bootloader();