From dabd37fae8d2a7b1cc71f48fce824b0b9d909a07 Mon Sep 17 00:00:00 2001 From: Laukik Hase Date: Thu, 9 Jul 2026 11:50:42 +0530 Subject: [PATCH] fix(esp_tee): Validate the stack pointer at the privilege switch boundary --- .../main/arch/riscv/esp_tee_asm_utils.inc | 32 +++++++++ .../main/arch/riscv/esp_tee_vectors_clic.S | 72 ++++++++++++------- .../main/arch/riscv/esp_tee_vectors_plic.S | 70 +++++++++++------- .../test_apps/tee_test_fw/main/CMakeLists.txt | 2 +- .../tee_test_fw/main/test_esp_tee_panic.c | 20 ++++++ .../tee_test_fw/tee_exception_test_map.py | 1 + 6 files changed, 142 insertions(+), 55 deletions(-) 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 7f470bd4ef0..781ac2f6ff4 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,34 +57,33 @@ _ns_sp_min: _ns_sp_max: .word 0 + .global _ns_int_rtn +_ns_int_rtn: + .word 0 + .section .exception_vectors.text, "ax" /* Exception handler. */ .global _tee_panic_handler .type _tee_panic_handler, @function _tee_panic_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 _actual_panic: /* Not an ecall, proceed to the panic handler */ @@ -146,6 +147,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 _actual_panic + .size _tee_panic_handler, .-_tee_panic_handler /* ECALL handler. */ @@ -204,14 +211,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 */ @@ -259,6 +267,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 @@ -392,8 +404,9 @@ _4: lw sp, 0(t1) /* 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) /* Place magic bytes in all the general registers */ store_magic_general_regs @@ -408,6 +421,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 a4dd850f018..be7e042e242 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 and Cache error interrupts trigger the panic * handler before reset, so they don’t need to be delegated. */ .equ TEE_INTR_DELEG_MASK, ~((1U << TEE_SECURE_INUM) | (1U << ETS_INT_WDT_INUM) | (1U << ETS_CACHEERR_INUM)) @@ -56,34 +57,32 @@ _ns_sp_min: _ns_sp_max: .word 0 + .global _ns_int_rtn +_ns_int_rtn: + .word 0 + .section .exception_vectors.text, "ax" /* Exception handler. */ .global _tee_panic_handler .type _tee_panic_handler, @function _tee_panic_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 _actual_panic: /* Not an ecall, proceed to the panic handler */ @@ -138,6 +137,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 _actual_panic + .size _tee_panic_handler, .-_tee_panic_handler /* ECALL handler. */ @@ -192,14 +197,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 */ @@ -244,6 +250,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 @@ -315,8 +325,9 @@ _tee_ns_intr_handler: lw sp, 0(t1) /* 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 @@ -335,6 +346,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 fa10a28e267..5aa23099823 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 json 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 612c0d475bd..4c7a666b7f1 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 @@ -19,6 +19,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)) @@ -212,3 +213,22 @@ TEST_CASE("Test REE-TEE isolation: DROM-W1", "[exception]") *(uint32_t *)(test_addr - 0x04) = 0xbadc0de; 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"); +} 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 db2d00c3b05..9eccea39adb 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 @@ -24,6 +24,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',