diff --git a/components/esp_tee/subproject/main/arch/riscv/esp_tee_asm_utils.inc b/components/esp_tee/subproject/main/arch/riscv/esp_tee_asm_utils.inc index 6478f02e13a..7e638cf55af 100644 --- a/components/esp_tee/subproject/main/arch/riscv/esp_tee_asm_utils.inc +++ b/components/esp_tee/subproject/main/arch/riscv/esp_tee_asm_utils.inc @@ -221,3 +221,35 @@ 1: #endif .endm + +/** + * VALIDATE_REE_SP + * Validate an REE-supplied sp before the TEE stores through it. The TEE region is + * at the bottom of SRAM, so a valid REE frame [sp - framesz, sp) must lie in the + * band above it and below the peripheral window: [SOC_S_DRAM_END, SOC_PERIPHERAL_LOW]. + * An out-of-bound sp will lead to a fault. + * + * With chk_priv (default) the check is skipped unless the trap came from U-mode; + * pass chk_priv=0 where the U-mode origin is already guaranteed (ecall-from-U). + * + * TODO: Revisit these bounds for high-performance RISC-V SoCs (e.g. ESP32-P4, + * ESP32-S31) with different memory maps than current ESP-TEE targets. + * + * Clobbers: \tx + */ +.macro VALIDATE_REE_SP framesz, tx, chk_priv=1 +.if \chk_priv + /* Skip validation unless the previous privilege (mstatus.MPP) was U-mode */ + csrr \tx, mstatus + srli \tx, \tx, MSTATUS_MPP_SHIFT + andi \tx, \tx, (MSTATUS_MPP >> MSTATUS_MPP_SHIFT) + bnez \tx, 1f +.endif + li \tx, (SOC_S_DRAM_END + \framesz) + bltu sp, \tx, _tee_sp_reject /* frame would dip into the TEE (or sub-TEE) region */ + li \tx, SOC_PERIPHERAL_LOW + bltu \tx, sp, _tee_sp_reject /* sp at/above the peripheral window */ +.if \chk_priv +1: +.endif +.endm diff --git a/components/esp_tee/subproject/main/arch/riscv/esp_tee_vectors_clic.S b/components/esp_tee/subproject/main/arch/riscv/esp_tee_vectors_clic.S index bf64ea045d5..debc4f77a3d 100644 --- a/components/esp_tee/subproject/main/arch/riscv/esp_tee_vectors_clic.S +++ b/components/esp_tee/subproject/main/arch/riscv/esp_tee_vectors_clic.S @@ -25,6 +25,8 @@ .equ ECALL_M_MODE, 0xb .equ CSR_UINTTHRESH, 0x047 .equ CSR_MINTTHRESH, 0x347 + .equ MCAUSE_EXCCODE_SHIFT, 20 + .equ MSTATUS_MPP_SHIFT, 11 .global esp_tee_global_interrupt_handler .global esp_tee_service_dispatcher @@ -55,6 +57,10 @@ _ns_sp_min: _ns_sp_max: .word 0 + .global _ns_int_rtn +_ns_int_rtn: + .word 0 + .section .exception_vectors.text, "ax" /* Exception handler. */ @@ -62,28 +68,23 @@ _ns_sp_max: .global _tee_panic_handler .type _tee_panic_handler, @function _tee_exception_handler: - /* Backup t0, t1 on the stack before using it */ - addi sp, sp, -16 - sw t0, 0(sp) - sw t1, 4(sp) + /* Backup t0 before using it */ + csrw mscratch, t0 - /* Read mcause */ + /* Check whether the exception is an M-mode/U-mode ecall */ csrr t0, mcause - li t1, VECTORS_MCAUSE_REASON_MASK - and t0, t0, t1 + slli t0, t0, MCAUSE_EXCCODE_SHIFT + srli t0, t0, MCAUSE_EXCCODE_SHIFT + addi t0, t0, -ECALL_M_MODE + beqz t0, _machine_ecall /* M-mode ecall */ + addi t0, t0, (ECALL_M_MODE - ECALL_U_MODE) + beqz t0, _user_ecall /* U-mode ecall */ - /* Check whether the exception is an M-mode ecall */ - li t1, ECALL_M_MODE - beq t0, t1, _machine_ecall + /* Validate a U-mode-origin sp before the handler stores/dumps through it */ + VALIDATE_REE_SP RV_STK_FRMSZ, t0 - /* Check whether the exception is an U-mode ecall */ - li t1, ECALL_U_MODE - beq t0, t1, _user_ecall - - /* Restore t0, t1 from the stack */ - lw t0, 0(sp) - lw t1, 4(sp) - addi sp, sp, 16 + /* Restore t0 */ + csrr t0, mscratch _tee_panic_handler: /* Not an ecall, proceed to the panic handler */ @@ -147,6 +148,12 @@ _return_from_exception: restore_general_regs RV_STK_FRMSZ mret + /* Fault if the sp given by the REE is found to be out-of-bounds */ +_tee_sp_reject: + csrr t0, mscratch + la sp, _tee_stack + j _tee_panic_handler + .size _tee_exception_handler, .-_tee_exception_handler /* ECALL handler. */ @@ -211,14 +218,15 @@ _skip_ctx_restore: /* U-mode ecall handler */ _user_ecall: /* Check whether we are returning after servicing an U-mode interrupt */ - lui t0, RTNVAL - csrrw t1, mscratch, zero - beq t0, t1, _rtn_from_ns_int + la t0, _ns_int_rtn + lw t0, 0(t0) + bnez t0, _rtn_from_ns_int - /* Restore t0, t1 from the stack */ - lw t0, 0(sp) - lw t1, 4(sp) - addi sp, sp, 16 + /* Reject an sp whose frame would be out-of-bounds */ + VALIDATE_REE_SP CONTEXT_SIZE, t0, 0 + + /* Restore t0 */ + csrr t0, mscratch /* This point is reached when a secure service call is issued from the REE */ /* Save register context and mepc */ @@ -272,6 +280,10 @@ _2: /* This point is reached after servicing a U-mode interrupt occurred * while executing a secure service */ _rtn_from_ns_int: + /* Consume the U-mode-interrupt-return sentinel (checked in _user_ecall). */ + la t0, _ns_int_rtn + sw zero, 0(t0) + /* Disable the U-mode interrupt delegation */ li t0, INTMTX_SIG_IDX_ASSERT_IN_SEC_REG li t1, TEE_PASS_INUM + CLIC_EXT_INTR_NUM_OFFSET @@ -415,8 +427,9 @@ _4: fence /* Set a flag to identify the next U2M switch would be after handling a U-mode interrupt */ - lui t0, RTNVAL - csrw mscratch, t0 + la t0, _ns_int_rtn + li t1, RTNVAL + sw t1, 0(t0) STACK_GUARD_POST_SWITCH t3 1 _ns_sp_min _ns_sp_max @@ -435,6 +448,11 @@ _4: .global _tee_s_intr_handler .type _tee_s_intr_handler, @function _tee_s_intr_handler: + /* Check sp if trapped from U-mode */ + csrw mscratch, t0 + VALIDATE_REE_SP RV_STK_FRMSZ, t0 + csrr t0, mscratch /* restore the preempted t0 */ + /* Start by saving the general purpose registers and the PC value before * the interrupt happened. */ save_general_regs RV_STK_FRMSZ diff --git a/components/esp_tee/subproject/main/arch/riscv/esp_tee_vectors_plic.S b/components/esp_tee/subproject/main/arch/riscv/esp_tee_vectors_plic.S index 1274e5df3d6..28437683e35 100644 --- a/components/esp_tee/subproject/main/arch/riscv/esp_tee_vectors_plic.S +++ b/components/esp_tee/subproject/main/arch/riscv/esp_tee_vectors_plic.S @@ -23,6 +23,7 @@ .equ RTNVAL, 0xc0de .equ ECALL_U_MODE, 0x8 .equ ECALL_M_MODE, 0xb + .equ MSTATUS_MPP_SHIFT, 11 /* NOTE: INTWDT timeout, Cache error and Stack protection interrupts * trigger the panic handler before reset, so they don't need to be delegated. */ .equ TEE_INTR_DELEG_BASE, ((1U << TEE_SECURE_INUM) | (1U << ETS_INT_WDT_INUM) | (1U << ETS_CACHEERR_INUM)) @@ -60,6 +61,10 @@ _ns_sp_min: _ns_sp_max: .word 0 + .global _ns_int_rtn +_ns_int_rtn: + .word 0 + .section .exception_vectors.text, "ax" /* Exception handler. */ @@ -67,28 +72,22 @@ _ns_sp_max: .global _tee_panic_handler .type _tee_exception_handler, @function _tee_exception_handler: - /* Backup t0, t1 on the stack before using it */ - addi sp, sp, -16 - sw t0, 0(sp) - sw t1, 4(sp) + /* Backup t0 before using it */ + csrw mscratch, t0 - /* Read mcause */ + /* Check whether the exception is an M-mode/U-mode ecall */ csrr t0, mcause - li t1, VECTORS_MCAUSE_REASON_MASK - and t0, t0, t1 + andi t0, t0, VECTORS_MCAUSE_REASON_MASK + addi t0, t0, -ECALL_M_MODE + beqz t0, _machine_ecall /* M-mode ecall */ + addi t0, t0, (ECALL_M_MODE - ECALL_U_MODE) + beqz t0, _user_ecall /* U-mode ecall */ - /* Check whether the exception is an M-mode ecall */ - li t1, ECALL_M_MODE - beq t0, t1, _machine_ecall + /* Validate a U-mode-origin sp before the handler stores/dumps through it */ + VALIDATE_REE_SP RV_STK_FRMSZ, t0 - /* Check whether the exception is an U-mode ecall */ - li t1, ECALL_U_MODE - beq t0, t1, _user_ecall - - /* Restore t0, t1 from the stack */ - lw t0, 0(sp) - lw t1, 4(sp) - addi sp, sp, 16 + /* Restore t0 */ + csrr t0, mscratch _tee_panic_handler: /* Not an ecall, proceed to the panic handler */ @@ -143,6 +142,12 @@ _return_from_exception: restore_general_regs RV_STK_FRMSZ mret + /* Fault if the sp given by the REE is found to be out-of-bounds */ +_tee_sp_reject: + csrr t0, mscratch + la sp, _tee_stack + j _tee_panic_handler + .size _tee_exception_handler, .-_tee_exception_handler /* ECALL handler. */ @@ -203,14 +208,15 @@ _skip_ctx_restore: /* U-mode ecall handler */ _user_ecall: /* Check whether we are returning after servicing an U-mode interrupt */ - lui t0, RTNVAL - csrrw t1, mscratch, zero - beq t0, t1, _rtn_from_ns_int + la t0, _ns_int_rtn + lw t0, 0(t0) + bnez t0, _rtn_from_ns_int - /* Restore t0, t1 from the stack */ - lw t0, 0(sp) - lw t1, 4(sp) - addi sp, sp, 16 + /* Reject an sp whose frame would be out-of-bounds */ + VALIDATE_REE_SP CONTEXT_SIZE, t0, 0 + + /* Restore t0 */ + csrr t0, mscratch /* This point is reached when a secure service call is issued from the REE */ /* Save register context and mepc */ @@ -260,6 +266,10 @@ _process_ecall: /* This point is reached after servicing a U-mode interrupt occurred * while executing a secure service */ _rtn_from_ns_int: + /* Consume the U-mode-interrupt-return sentinel (checked in _user_ecall). */ + la t0, _ns_int_rtn + sw zero, 0(t0) + /* Disable the U-mode interrupt delegation */ csrwi mideleg, 0 @@ -341,8 +351,9 @@ _tee_ns_intr_handler: fence /* Set a flag to identify the next U2M switch would be after handling a U-mode interrupt */ - lui t0, RTNVAL - csrw mscratch, t0 + la t0, _ns_int_rtn + li t1, RTNVAL + sw t1, 0(t0) /* Enable the U-mode interrupt delegation (except for the TEE secure interrupt) */ li t0, TEE_INTR_DELEG_MASK @@ -365,6 +376,11 @@ _tee_ns_intr_handler: .global _tee_s_intr_handler .type _tee_s_intr_handler, @function _tee_s_intr_handler: + /* Check sp if trapped from U-mode */ + csrw mscratch, t0 + VALIDATE_REE_SP RV_STK_FRMSZ, t0 + csrr t0, mscratch /* restore the preempted t0 */ + /* Start by saving the general purpose registers and the PC value before * the interrupt happened. */ save_general_regs RV_STK_FRMSZ diff --git a/components/esp_tee/test_apps/tee_test_fw/main/CMakeLists.txt b/components/esp_tee/test_apps/tee_test_fw/main/CMakeLists.txt index af0c5563825..9c8e4c6e968 100644 --- a/components/esp_tee/test_apps/tee_test_fw/main/CMakeLists.txt +++ b/components/esp_tee/test_apps/tee_test_fw/main/CMakeLists.txt @@ -1,6 +1,6 @@ idf_build_get_property(idf_path IDF_PATH) -set(priv_requires bootloader_support esp_driver_gptimer esp_tee esp_timer mbedtls spi_flash) +set(priv_requires bootloader_support esp_driver_gptimer esp_system esp_tee esp_timer mbedtls spi_flash) # Test FW related list(APPEND priv_requires nvs_flash test_utils unity) # TEE related diff --git a/components/esp_tee/test_apps/tee_test_fw/main/test_esp_tee_panic.c b/components/esp_tee/test_apps/tee_test_fw/main/test_esp_tee_panic.c index 79382076166..63f004d011d 100644 --- a/components/esp_tee/test_apps/tee_test_fw/main/test_esp_tee_panic.c +++ b/components/esp_tee/test_apps/tee_test_fw/main/test_esp_tee_panic.c @@ -29,6 +29,7 @@ #include "unity.h" #include "esp_tee.h" +#include "esp_private/hw_stack_guard.h" #include "secure_service_num.h" #define ALIGN_DOWN_TO_MMU_PAGE_SIZE(addr) ((addr) & ~((SOC_MMU_PAGE_SIZE) - 1)) @@ -229,6 +230,25 @@ TEST_CASE("Test REE-TEE isolation: DROM-W1", "[exception]") TEST_FAIL_MESSAGE("Exception should have been generated"); } +TEST_CASE("Test REE-TEE isolation: Corrupted SP", "[exception]") +{ + uintptr_t atk_sp = (uintptr_t)&_iram_start - 0x100; + + /* Disable U-mode interrupts so the tick cannot preempt before the ecall */ + __asm__ volatile("csrci ustatus, 0x1\n\t" : : : "memory"); + + /* Stop the REE-owned HW stack guard, as a malicious REE could */ +#if CONFIG_ESP_SYSTEM_HW_STACK_GUARD + esp_hw_stack_guard_monitor_stop(); +#endif + + /* Cross into the TEE with the doctored sp; the handler rejects it and panics */ + __asm__ volatile("mv sp, %0\n\t" + "ecall\n\t" : : "r"(atk_sp) : "memory"); + + TEST_FAIL_MESSAGE("Exception should have been generated"); +} + static void do_stack_overflow(int depth, volatile uint8_t *sink) { if (depth == -1) { diff --git a/components/esp_tee/test_apps/tee_test_fw/tee_exception_test_map.py b/components/esp_tee/test_apps/tee_test_fw/tee_exception_test_map.py index 7590bae9469..b53d7d5df9c 100644 --- a/components/esp_tee/test_apps/tee_test_fw/tee_exception_test_map.py +++ b/components/esp_tee/test_apps/tee_test_fw/tee_exception_test_map.py @@ -23,6 +23,7 @@ _BASE_CONFIG = { 'DROM-R1': 'Load access fault', 'DROM-W1': 'Store access fault', 'MMU-spillover': 'Illegal instruction', + 'Corrupted SP': 'Environment call from U-mode', }, 'apm_violation': { 'eFuse': 'APM - Space exception',