From c829a82ff15719b783756e3d08157f8a4be9146d Mon Sep 17 00:00:00 2001 From: Chip Weinberger Date: Mon, 13 Mar 2023 18:57:22 -0700 Subject: [PATCH] [Stack Mirror][v4.4] make corrupt backtraces not happen nearly as often --- CMakeLists.txt | 4 + Kconfig | 16 +++ .../esp_hw_support/include/soc/spinlock.h | 1 + components/esp_system/CMakeLists.txt | 1 + .../include/esp_private/stack_mirror.h | 10 ++ .../port/arch/xtensa/debug_helpers.c | 13 +- .../esp_system/port/arch/xtensa/panic_arch.c | 7 +- components/esp_system/stack_mirror.c | 128 ++++++++++++++++++ components/freertos/Kconfig | 6 +- .../freertos/include/freertos/FreeRTOS.h | 6 + components/freertos/include/freertos/task.h | 10 ++ components/freertos/port/port_common.c | 6 + .../port/xtensa/include/freertos/portmacro.h | 9 ++ components/freertos/tasks.c | 24 ++++ components/hal/esp32/include/hal/cpu_ll.h | 3 + components/hal/esp32s3/include/hal/cpu_ll.h | 3 + 16 files changed, 243 insertions(+), 4 deletions(-) create mode 100644 components/esp_system/include/esp_private/stack_mirror.h create mode 100644 components/esp_system/stack_mirror.c diff --git a/CMakeLists.txt b/CMakeLists.txt index 15c7125529..4352f1b19e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -28,6 +28,10 @@ if(NOT BOOTLOADER_BUILD) list(APPEND compile_options "-O2") endif() + if(CONFIG_COMPILER_STACK_MIRROR) + list(APPEND compile_options "-finstrument-functions") + endif() + else() # BOOTLOADER_BUILD if(CONFIG_BOOTLOADER_COMPILER_OPTIMIZATION_SIZE) diff --git a/Kconfig b/Kconfig index 928d274106..004ceb6d0d 100644 --- a/Kconfig +++ b/Kconfig @@ -380,6 +380,22 @@ mainmenu "Espressif IoT Development Framework Configuration" help Stack smashing protection. + config COMPILER_STACK_MIRROR + prompt "Enable Stack Mirror" + bool + default "n" + help + Make corrupted backtraces much less likely by creating a dupliate copy of stack return + addresses in heap memory. Requires about 100 bytes of memory per thread. + + config COMPILER_STACK_MIRROR_DEPTH + prompt "Stack Mirror Depth" + int + default 26 + depends on COMPILER_STACK_MIRROR + help + The maximum depth of the stack mirror. 4 bytes are needed per depth. + config COMPILER_WARN_WRITE_STRINGS bool "Enable -Wwrite-strings warning flag" default "n" diff --git a/components/esp_hw_support/include/soc/spinlock.h b/components/esp_hw_support/include/soc/spinlock.h index b2fadfe49e..469dadfd95 100644 --- a/components/esp_hw_support/include/soc/spinlock.h +++ b/components/esp_hw_support/include/soc/spinlock.h @@ -64,6 +64,7 @@ static inline void __attribute__((always_inline)) spinlock_initialize(spinlock_t * @param lock - target spinlock object * @param timeout - cycles to wait, passing SPINLOCK_WAIT_FOREVER blocs indefinitely */ + static inline bool __attribute__((always_inline)) spinlock_acquire(spinlock_t *lock, int32_t timeout) { #if !CONFIG_FREERTOS_UNICORE && !BOOTLOADER_BUILD diff --git a/components/esp_system/CMakeLists.txt b/components/esp_system/CMakeLists.txt index 966fcb121a..3345928d77 100644 --- a/components/esp_system/CMakeLists.txt +++ b/components/esp_system/CMakeLists.txt @@ -20,6 +20,7 @@ else() "startup.c" "system_time.c" "stack_check.c" + "stack_mirror.c" "task_wdt.c" "ubsan.c" "xt_wdt.c" diff --git a/components/esp_system/include/esp_private/stack_mirror.h b/components/esp_system/include/esp_private/stack_mirror.h new file mode 100644 index 0000000000..ec346a99e5 --- /dev/null +++ b/components/esp_system/include/esp_private/stack_mirror.h @@ -0,0 +1,10 @@ +/* + * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#if CONFIG_COMPILER_STACK_MIRROR +void stack_mirror_enable(); +void stack_mirror_print_backtrace(bool panic); +#endif \ No newline at end of file diff --git a/components/esp_system/port/arch/xtensa/debug_helpers.c b/components/esp_system/port/arch/xtensa/debug_helpers.c index 68f0665a13..feb62f5f76 100644 --- a/components/esp_system/port/arch/xtensa/debug_helpers.c +++ b/components/esp_system/port/arch/xtensa/debug_helpers.c @@ -22,6 +22,7 @@ #include "soc/soc_memory_layout.h" #include "soc/cpu.h" #include "esp_private/panic_internal.h" +#include "esp_private/stack_mirror.h" #include "xtensa/xtensa_context.h" @@ -94,6 +95,9 @@ esp_err_t IRAM_ATTR esp_backtrace_print_from_frame(int depth, const esp_backtrac esp_err_t ret = ESP_OK; if (corrupted) { print_str(" |<-CORRUPTED", panic); +#ifndef CONFIG_COMPILER_STACK_MIRROR + print_str("\r\nTurn on Stack Mirroring to avoid corruption", panic); +#endif ret = ESP_FAIL; } else if (stk_frame.next_pc != 0) { //Backtrace continues print_str(" |<-CONTINUES", panic); @@ -107,5 +111,12 @@ esp_err_t IRAM_ATTR esp_backtrace_print(int depth) //Initialize stk_frame with first frame of stack esp_backtrace_frame_t start = { 0 }; esp_backtrace_get_start(&(start.pc), &(start.sp), &(start.next_pc)); - return esp_backtrace_print_from_frame(depth, &start, false); + esp_err_t err = esp_backtrace_print_from_frame(depth, &start, false); +#if CONFIG_COMPILER_STACK_MIRROR + if (err != ESP_OK) { + stack_mirror_print_backtrace(false); + } +#endif + return err; } + diff --git a/components/esp_system/port/arch/xtensa/panic_arch.c b/components/esp_system/port/arch/xtensa/panic_arch.c index c5c5eb5de1..923c5be361 100644 --- a/components/esp_system/port/arch/xtensa/panic_arch.c +++ b/components/esp_system/port/arch/xtensa/panic_arch.c @@ -11,6 +11,7 @@ #include "esp_debug_helpers.h" #include "esp_private/panic_internal.h" +#include "esp_private/stack_mirror.h" #include "esp_private/panic_reason.h" #include "soc/soc.h" @@ -467,5 +468,9 @@ void panic_print_backtrace(const void *f, int core) { XtExcFrame *xt_frame = (XtExcFrame *) f; esp_backtrace_frame_t frame = {.pc = xt_frame->pc, .sp = xt_frame->a1, .next_pc = xt_frame->a0, .exc_frame = xt_frame}; - esp_backtrace_print_from_frame(100, &frame, true); + if (esp_backtrace_print_from_frame(100, &frame, true) != ESP_OK) { +#if CONFIG_COMPILER_STACK_MIRROR + stack_mirror_print_backtrace(true); +#endif + } } diff --git a/components/esp_system/stack_mirror.c b/components/esp_system/stack_mirror.c new file mode 100644 index 0000000000..ab5c9890ba --- /dev/null +++ b/components/esp_system/stack_mirror.c @@ -0,0 +1,128 @@ +/* + * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" +#include "esp_heap_caps.h" +#include "esp_private/panic_internal.h" +#include "soc/soc_memory_types.h" + +#if CONFIG_COMPILER_STACK_MIRROR + +#define STACK_MIRROR_MAGIC 139871234 + +/* __cyg_profile_func_enter - gcc inserts a call to this function at the start of every function + * __cyg_profile_func_exit - gcc inserts a call to this function at the end of every function + * no_instrument_function - This tells GCC to not insert instrumentation calls (would cause an infinite loop) */ +void IRAM_ATTR __cyg_profile_func_enter(void *func, void *caller) __attribute__((no_instrument_function)); +void IRAM_ATTR __cyg_profile_func_exit(void *func, void *caller) __attribute__((no_instrument_function)); +static void IRAM_ATTR print_str(const char* str, bool panic) __attribute__((no_instrument_function)); +static void IRAM_ATTR print_entry(uint32_t pc, bool panic) __attribute__((no_instrument_function)); + +static uint32_t enabled; + +static void IRAM_ATTR print_str(const char* str, bool panic) +{ + if (panic) { + panic_print_str(str); + } else { + esp_rom_printf(str); + } +} + +static void IRAM_ATTR print_entry(uint32_t pc, bool panic) +{ + if (panic) { + panic_print_str(" 0x"); + panic_print_hex(pc); + } else { + esp_rom_printf(" 0x%08X", pc); + } +} + +extern void xt_unhandled_exception(XtExcFrame *frame); + +void IRAM_ATTR __cyg_profile_func_enter(void *func, void *callsite) +{ + if (enabled == STACK_MIRROR_MAGIC) { + esp_stack_mirror_t *m = pvTaskGetStackMirrorPointer(); + if (func == xt_unhandled_exception) { + // dont add to the stack trace after we already hit panic. + m->panicing = true; + } + if (!m->panicing) { + if (m->depth < CONFIG_COMPILER_STACK_MIRROR_DEPTH) { + m->backtrace[m->depth] = (uint32_t) func; + } + m->depth++; + } + } +} + +void IRAM_ATTR __cyg_profile_func_exit(void *func, void *callsite) +{ + if (enabled == STACK_MIRROR_MAGIC) { + esp_stack_mirror_t *m = pvTaskGetStackMirrorPointer(); + if (!m->panicing && m->depth > 0) { + m->depth--; + } + } +} + +void IRAM_ATTR stack_mirror_print_backtrace(bool panic) +{ + esp_stack_mirror_t *m = pvTaskGetStackMirrorPointer(); + + print_str("\r\nBacktrace (Mirror):", panic); + + if (m == NULL) { + + print_str("\r\nNot Set", panic); + + } else if (esp_ptr_internal(m) == false) { + + print_str("\r\nInvalid Ptr", panic); + + } else if (m->depth == 0) { + + print_str("\r\nEmpty", panic); + + } else { + + uint32_t depth = m->depth; + + if (depth >= CONFIG_COMPILER_STACK_MIRROR_DEPTH) { + print_str("\r\nBACKTRACE INCOMPLETE", panic); + depth = CONFIG_COMPILER_STACK_MIRROR_DEPTH; + } + + for (int i = depth - 1; i >= 0; i--) { + + uint32_t pc = m->backtrace[i]; + + print_entry(pc, panic); + + bool corrupted = !esp_ptr_executable((void *)esp_cpu_process_stack_pc(pc)); + + if (corrupted) { + print_str(" |<-CORRUPTED", panic); + break; + } + } + } + + print_str("\r\n\r\n", panic); + + return; +} + +void stack_mirror_enable() +{ + enabled = STACK_MIRROR_MAGIC; +} + +#endif // CONFIG_COMPILER_STACK_MIRROR \ No newline at end of file diff --git a/components/freertos/Kconfig b/components/freertos/Kconfig index 0c66f9af8f..1ccf1e9b78 100644 --- a/components/freertos/Kconfig +++ b/components/freertos/Kconfig @@ -172,8 +172,10 @@ menu "FreeRTOS" FreeRTOS has the ability to store per-thread pointers in the task control block. This controls the number of pointers available. - This value must be at least 1. Index 0 is reserved for use by the pthreads API - thread-local-storage. Other indexes can be used for any desired purpose. + This value must be at least 1. Index 0 is reserved for use by the pthreads API thread-local-storage. + + Other indexes can be used for any desired purpose. + choice FREERTOS_ASSERT prompt "FreeRTOS assertions" diff --git a/components/freertos/include/freertos/FreeRTOS.h b/components/freertos/include/freertos/FreeRTOS.h index 296b287837..9dc8ab7955 100644 --- a/components/freertos/include/freertos/FreeRTOS.h +++ b/components/freertos/include/freertos/FreeRTOS.h @@ -1254,6 +1254,12 @@ typedef struct xSTATIC_TCB #if ( configUSE_POSIX_ERRNO == 1 ) int iDummy22; #endif + #if CONFIG_COMPILER_STACK_MIRROR + uint32_t uDummy23; + uint32_t uDummy24; + uint32_t uDummy25[CONFIG_COMPILER_STACK_MIRROR_DEPTH]; + #endif + } StaticTask_t; /* diff --git a/components/freertos/include/freertos/task.h b/components/freertos/include/freertos/task.h index 89841757d4..cebf14adc2 100644 --- a/components/freertos/include/freertos/task.h +++ b/components/freertos/include/freertos/task.h @@ -1818,6 +1818,16 @@ configSTACK_DEPTH_TYPE uxTaskGetStackHighWaterMark2( TaskHandle_t xTask ) PRIVIL */ uint8_t* pxTaskGetStackStart( TaskHandle_t xTask) PRIVILEGED_FUNCTION; +#if CONFIG_COMPILER_STACK_MIRROR +typedef struct { + uint32_t panicing; + uint32_t depth; + uint32_t backtrace[CONFIG_COMPILER_STACK_MIRROR_DEPTH]; +} esp_stack_mirror_t; + +esp_stack_mirror_t *pvTaskGetStackMirrorPointer(); +#endif // CONFIG_COMPILER_STACK_MIRROR + /* When using trace macros it is sometimes necessary to include task.h before * FreeRTOS.h. When this is done TaskHookFunction_t will not yet have been defined, * so the following two prototypes will cause a compilation error. This can be diff --git a/components/freertos/port/port_common.c b/components/freertos/port/port_common.c index ad0bc8cf71..f66cd91980 100644 --- a/components/freertos/port/port_common.c +++ b/components/freertos/port/port_common.c @@ -15,6 +15,7 @@ #include "esp_task.h" #include "esp_private/crosscore_int.h" #include "esp_private/startup_internal.h" /* Required by g_spiram_ok. [refactor-todo] for g_spiram_ok */ +#include "esp_private/stack_mirror.h" #include "esp_log.h" #include "soc/soc_memory_types.h" #include "soc/dport_access.h" @@ -138,6 +139,11 @@ static void main_task(void* args) } #endif +// Todo: this could be initialized sooner, but not sure where exactly... +#if CONFIG_COMPILER_STACK_MIRROR + stack_mirror_enable(); +#endif + app_main(); vTaskDelete(NULL); } diff --git a/components/freertos/port/xtensa/include/freertos/portmacro.h b/components/freertos/port/xtensa/include/freertos/portmacro.h index bce731d331..0987c3fc1f 100644 --- a/components/freertos/port/xtensa/include/freertos/portmacro.h +++ b/components/freertos/port/xtensa/include/freertos/portmacro.h @@ -558,6 +558,9 @@ static inline void __attribute__((always_inline)) uxPortCompareSetExtram(volatil // --------------------- Interrupts ------------------------ +#if CONFIG_COMPILER_STACK_MIRROR + IRAM_ATTR __attribute__((no_instrument_function)) +#endif static inline UBaseType_t __attribute__((always_inline)) xPortSetInterruptMaskFromISR(void) { UBaseType_t prev_int_level = XTOS_SET_INTLEVEL(XCHAL_EXCM_LEVEL); @@ -565,6 +568,9 @@ static inline UBaseType_t __attribute__((always_inline)) xPortSetInterruptMaskFr return prev_int_level; } +#if CONFIG_COMPILER_STACK_MIRROR + IRAM_ATTR __attribute__((no_instrument_function)) +#endif static inline void __attribute__((always_inline)) vPortClearInterruptMaskFromISR(UBaseType_t prev_level) { portbenchmarkINTERRUPT_RESTORE(prev_level); @@ -629,6 +635,9 @@ static inline bool IRAM_ATTR xPortCanYield(void) // ----------------------- System -------------------------- +#if CONFIG_COMPILER_STACK_MIRROR + __attribute__((no_instrument_function)) +#endif static inline BaseType_t IRAM_ATTR xPortGetCoreID(void) { return (uint32_t) cpu_hal_get_core_id(); diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index deff650528..f85a47b949 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -270,6 +270,7 @@ #define taskEVENT_LIST_ITEM_VALUE_IN_USE 0x80000000UL #endif + /* * Task control block. A task control block (TCB) is allocated for each task, * and stores task state information, including a pointer to the task's context @@ -356,6 +357,11 @@ typedef struct tskTaskControlBlock /* The old naming convention is used to #if ( configUSE_POSIX_ERRNO == 1 ) int iTaskErrno; #endif + + #if CONFIG_COMPILER_STACK_MIRROR + esp_stack_mirror_t stackMirror; + #endif + } tskTCB; /* The old tskTCB name is maintained above then typedefed to the new TCB_t name @@ -991,6 +997,11 @@ static void prvInitialiseNewTask( TaskFunction_t pxTaskCode, } #endif /* portSTACK_GROWTH */ + #if CONFIG_COMPILER_STACK_MIRROR + pxNewTCB->stackMirror.panicing = false; + pxNewTCB->stackMirror.depth = 0; + #endif + /* Store the task name in the TCB. */ if( pcName != NULL ) { @@ -4097,6 +4108,16 @@ static portTASK_FUNCTION( prvIdleTask, pvParameters ) #endif /* configUSE_TICKLESS_IDLE */ /*-----------------------------------------------------------*/ + +#if CONFIG_COMPILER_STACK_MIRROR + IRAM_ATTR __attribute__((no_instrument_function)) + esp_stack_mirror_t * pvTaskGetStackMirrorPointer() + { + TCB_t *pxTCB = xTaskGetCurrentTaskHandle(); + return &pxTCB->stackMirror; + } +#endif + #if ( configNUM_THREAD_LOCAL_STORAGE_POINTERS != 0 ) #if ( configTHREAD_LOCAL_STORAGE_DELETE_CALLBACKS ) @@ -4646,6 +4667,9 @@ static void prvResetNextTaskUnblockTime( void ) #if ( ( INCLUDE_xTaskGetCurrentTaskHandle == 1 ) || ( configUSE_MUTEXES == 1 ) || (configNUM_CORES > 1) ) +#if CONFIG_COMPILER_STACK_MIRROR + IRAM_ATTR __attribute__((no_instrument_function)) +#endif TaskHandle_t xTaskGetCurrentTaskHandle( void ) { TaskHandle_t xReturn; diff --git a/components/hal/esp32/include/hal/cpu_ll.h b/components/hal/esp32/include/hal/cpu_ll.h index 68475e4ddf..5977bed2e3 100644 --- a/components/hal/esp32/include/hal/cpu_ll.h +++ b/components/hal/esp32/include/hal/cpu_ll.h @@ -29,6 +29,9 @@ extern "C" { #endif +#if CONFIG_COMPILER_STACK_MIRROR + IRAM_ATTR __attribute__((no_instrument_function)) +#endif static inline uint32_t IRAM_ATTR cpu_ll_get_core_id(void) { uint32_t id; diff --git a/components/hal/esp32s3/include/hal/cpu_ll.h b/components/hal/esp32s3/include/hal/cpu_ll.h index d22a32f288..84231573d6 100644 --- a/components/hal/esp32s3/include/hal/cpu_ll.h +++ b/components/hal/esp32s3/include/hal/cpu_ll.h @@ -28,6 +28,9 @@ extern "C" { #endif +#if CONFIG_COMPILER_STACK_MIRROR + __attribute__((no_instrument_function)) +#endif static inline uint32_t IRAM_ATTR cpu_ll_get_core_id(void) { uint32_t id;