From fd73878204d728d7e660d7053a7f9094c478c58b Mon Sep 17 00:00:00 2001 From: Laukik Hase Date: Thu, 30 Jul 2026 14:29:46 +0800 Subject: [PATCH] fix(esp_tee): Reject re-entrant secure service calls from the REE --- .../main/arch/riscv/esp_tee_asm_utils.inc | 19 ++++++ .../main/arch/riscv/esp_tee_vectors_clic.S | 23 +++++++ .../main/arch/riscv/esp_tee_vectors_plic.S | 23 +++++++ .../tee_test_fw/main/test_esp_tee_interrupt.c | 60 +++++++++++++++++-- 4 files changed, 121 insertions(+), 4 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 7e638cf55af..626dfdc0bf6 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 @@ -22,6 +22,7 @@ .equ SAVE_REGS, 32 .equ CONTEXT_SIZE, (SAVE_REGS * 4) .equ MAGIC, 0x1f +.equ NS_INT_RTN_MAGIC, (MAGIC << 12) /* Macro which first allocates space on the stack to save general * purpose registers, and then save them. GP register is excluded. @@ -222,6 +223,24 @@ #endif .endm +/** + * SVC_LOCK_ACQUIRE / SVC_LOCK_RELEASE + * Hold _s_svc_lock for the duration of a secure service call, so that a call issued + * while another one is active branches to \fail instead of clobbering it. + * + * Clobbers: \tx + */ +.macro SVC_LOCK_ACQUIRE tx, fail + la \tx, _s_svc_lock + amoswap.w.aq \tx, \tx, (\tx) + bnez \tx, \fail +.endm + +.macro SVC_LOCK_RELEASE tx + la \tx, _s_svc_lock + amoswap.w.rl zero, zero, (\tx) +.endm + /** * VALIDATE_REE_SP * Validate an REE-supplied sp before the TEE stores through it. The TEE region is 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 debc4f77a3d..7c56c5f0bc6 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 @@ -61,6 +61,10 @@ _ns_sp_max: _ns_int_rtn: .word 0 + .global _s_svc_lock +_s_svc_lock: + .word 0 + .section .exception_vectors.text, "ax" /* Exception handler. */ @@ -178,6 +182,9 @@ _1: lui t0, ESP_TEE_M2U_SWITCH_MAGIC beq a1, t0, _skip_ctx_restore + /* The secure service has returned - the REE may issue the next one */ + SVC_LOCK_RELEASE t0 + /* Check if we need to restore the MINTTHRESH register */ la t0, _s_intr_thresh lw t1, 0(t0) @@ -218,10 +225,13 @@ _skip_ctx_restore: /* U-mode ecall handler */ _user_ecall: /* Check whether we are returning after servicing an U-mode interrupt */ + li t0, NS_INT_RTN_MAGIC + bne a0, t0, _svc_call_enter la t0, _ns_int_rtn lw t0, 0(t0) bnez t0, _rtn_from_ns_int +_svc_call_enter: /* Reject an sp whose frame would be out-of-bounds */ VALIDATE_REE_SP CONTEXT_SIZE, t0, 0 @@ -233,6 +243,9 @@ _user_ecall: save_general_regs save_mepc + /* Claim the TEE before touching any of its state */ + SVC_LOCK_ACQUIRE t0, _svc_call_reject + # Check if REE is in a critical section csrr t0, CSR_UINTTHRESH # t0 = current UINTTHRESH beqz t0, _process_ecall # if threshold == 0 -> continue @@ -314,6 +327,16 @@ _3: mret + /* Discard a service call that arrived while another one was active */ +_svc_call_reject: + addi sp, sp, CONTEXT_SIZE /* t0 is the only register clobbered past the context save */ + csrr t0, mepc + addi t0, t0, 4 /* resume the REE after its ecall */ + csrw mepc, t0 + csrr t0, mscratch + li a0, -1 + mret + .size _ecall_handler, .-_ecall_handler /* This is the interrupt handler for the U-mode interrupts. 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 28437683e35..9615594ba0d 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 @@ -65,6 +65,10 @@ _ns_sp_max: _ns_int_rtn: .word 0 + .global _s_svc_lock +_s_svc_lock: + .word 0 + .section .exception_vectors.text, "ax" /* Exception handler. */ @@ -167,6 +171,9 @@ _machine_ecall: lui t0, ESP_TEE_M2U_SWITCH_MAGIC beq a1, t0, _skip_ctx_restore + /* The secure service has returned - the REE may issue the next one */ + SVC_LOCK_RELEASE t0 + /* Check if we need to restore the MXINT threshold register */ la t0, _s_intr_thresh lw t1, 0(t0) @@ -208,10 +215,13 @@ _skip_ctx_restore: /* U-mode ecall handler */ _user_ecall: /* Check whether we are returning after servicing an U-mode interrupt */ + li t0, NS_INT_RTN_MAGIC + bne a0, t0, _svc_call_enter la t0, _ns_int_rtn lw t0, 0(t0) bnez t0, _rtn_from_ns_int +_svc_call_enter: /* Reject an sp whose frame would be out-of-bounds */ VALIDATE_REE_SP CONTEXT_SIZE, t0, 0 @@ -223,6 +233,9 @@ _user_ecall: save_general_regs save_mepc + /* Claim the TEE before touching any of its state */ + SVC_LOCK_ACQUIRE t0, _svc_call_reject + # Check if REE is in a critical section li t0, PLIC_UXINT_THRESH_REG lw t1, 0(t0) # t1 = current UXINT threshold @@ -293,6 +306,16 @@ _rtn_from_ns_int: mret + /* Discard a service call that arrived while another one was active */ +_svc_call_reject: + addi sp, sp, CONTEXT_SIZE /* t0 is the only register clobbered past the context save */ + csrr t0, mepc + addi t0, t0, 4 /* resume the REE after its ecall */ + csrw mepc, t0 + csrr t0, mscratch + li a0, -1 + mret + .size _ecall_handler, .-_ecall_handler /* This is the interrupt handler for the U-mode interrupts. diff --git a/components/esp_tee/test_apps/tee_test_fw/main/test_esp_tee_interrupt.c b/components/esp_tee/test_apps/tee_test_fw/main/test_esp_tee_interrupt.c index d8da5c75ba8..ba15ed54781 100644 --- a/components/esp_tee/test_apps/tee_test_fw/main/test_esp_tee_interrupt.c +++ b/components/esp_tee/test_apps/tee_test_fw/main/test_esp_tee_interrupt.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2024-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2024-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -34,7 +34,7 @@ static bool IRAM_ATTR test_timer_on_alarm_cb(gptimer_handle_t timer, const gptim return true; } -static void test_timer_init(volatile uint32_t *arg) +static void test_timer_init_with_cb(gptimer_alarm_cb_t on_alarm, void *arg) { /* Select and initialize basic parameters of the timer */ gptimer_config_t timer_config = { @@ -45,9 +45,9 @@ static void test_timer_init(volatile uint32_t *arg) ESP_ERROR_CHECK(gptimer_new_timer(&timer_config, &gptimer)); gptimer_event_callbacks_t cbs = { - .on_alarm = test_timer_on_alarm_cb, + .on_alarm = on_alarm, }; - ESP_ERROR_CHECK(gptimer_register_event_callbacks(gptimer, &cbs, (void *)arg)); + ESP_ERROR_CHECK(gptimer_register_event_callbacks(gptimer, &cbs, arg)); ESP_ERROR_CHECK(gptimer_enable(gptimer)); @@ -60,6 +60,11 @@ static void test_timer_init(volatile uint32_t *arg) ESP_ERROR_CHECK(gptimer_start(gptimer)); } +static void test_timer_init(volatile uint32_t *arg) +{ + test_timer_init_with_cb(test_timer_on_alarm_cb, (void *)arg); +} + static void test_timer_deinit(void) { ESP_ERROR_CHECK(gptimer_stop(gptimer)); @@ -113,6 +118,53 @@ TEST_CASE("Test REE interrupt in TEE", "[basic]") TEST_ASSERT_MESSAGE((mode == ESP_CPU_NS_MODE), "Incorrect privilege mode!"); } +typedef struct { + volatile uint32_t intr_count; + volatile uint32_t accepted_count; +} test_nested_svc_call_ctx_t; + +static bool IRAM_ATTR test_nested_svc_call_cb(gptimer_handle_t timer, const gptimer_alarm_event_data_t *edata, void *user_data) +{ + test_nested_svc_call_ctx_t *ctx = (test_nested_svc_call_ctx_t *)user_data; + + /* Issued while the preempted service call sits parked inside the TEE */ + uint32_t ret = esp_tee_service_call(3, SS_ESP_TEE_TEST_SERVICE_ADD, 200, 100); + if (ret != UINT32_MAX) { + ctx->accepted_count = ctx->accepted_count + 1; + } + ctx->intr_count = ctx->intr_count + 1; + + esp_rom_printf("[mode: %d] Nested service call from ISR (%d) returned 0x%x\n", + esp_cpu_get_curr_privilege_level(), ctx->intr_count, ret); + return true; +} + +TEST_CASE("Test nested secure service call from an REE interrupt", "[basic]") +{ + TEST_ASSERT_EQUAL(ESP_CPU_NS_MODE, esp_cpu_get_curr_privilege_level()); + + static test_nested_svc_call_ctx_t ctx; + ctx.intr_count = 0; + ctx.accepted_count = 0; + + test_timer_init_with_cb(test_nested_svc_call_cb, &ctx); + + /* Runs in the TEE until the ISR above has fired ESP_TEE_TEST_INTR_ITER times */ + uint32_t val = esp_tee_service_call(2, SS_ESP_TEE_TEST_REE_INTR_IN_TEE, &ctx.intr_count); + TEST_ASSERT_EQUAL_UINT32(0, val); + + test_timer_deinit(); + + /* Every call made from the ISR should have been rejected by the TEE */ + TEST_ASSERT_EQUAL_UINT32(0, ctx.accepted_count); + + /* The parked call resumed and completed, so the TEE takes calls again */ + val = esp_tee_service_call(3, SS_ESP_TEE_TEST_SERVICE_ADD, 200, 100); + TEST_ASSERT_EQUAL_UINT32(300, val); + + TEST_ASSERT_EQUAL(ESP_CPU_NS_MODE, esp_cpu_get_curr_privilege_level()); +} + TEST_CASE("Test TEE interrupt in REE", "[basic]") { esp_cpu_priv_mode_t mode = esp_cpu_get_curr_privilege_level();