From 9ce63d8dfa277aa071b765e7b974c809c96ab802 Mon Sep 17 00:00:00 2001 From: Laukik Hase Date: Thu, 25 Jun 2026 19:27:16 +0530 Subject: [PATCH] fix(esp_tee): Add additional input validation checks for TEE service calls --- .../esp_tee/src/esp_secure_service_wrapper.c | 12 ++++++++- .../main/core/esp_secure_services.c | 7 ++++++ .../main/core/esp_secure_services_iram.c | 25 +++++++++++++++++-- .../subproject/main/core/esp_tee_intr.c | 14 +++++------ .../tee_cli_app/sdkconfig.ci.release | 4 +-- 5 files changed, 50 insertions(+), 12 deletions(-) diff --git a/components/esp_tee/src/esp_secure_service_wrapper.c b/components/esp_tee/src/esp_secure_service_wrapper.c index 04f1cc02cf0..e01214439b1 100644 --- a/components/esp_tee/src/esp_secure_service_wrapper.c +++ b/components/esp_tee/src/esp_secure_service_wrapper.c @@ -256,10 +256,20 @@ esp_err_t __wrap_esp_ds_start_sign(const void *message, if (esp_ds_ctx != NULL) { *esp_ds_ctx = malloc(sizeof(esp_ds_context_t)); if (!*esp_ds_ctx) { + esp_crypto_ds_lock_release(); return ESP_ERR_NO_MEM; } } - return esp_tee_service_call(5, SS_ESP_DS_START_SIGN, message, data, key_id, esp_ds_ctx); + + esp_err_t err = esp_tee_service_call(5, SS_ESP_DS_START_SIGN, message, data, key_id, esp_ds_ctx); + if (err != ESP_OK) { + if (esp_ds_ctx != NULL) { + free(*esp_ds_ctx); + *esp_ds_ctx = NULL; + } + esp_crypto_ds_lock_release(); + } + return err; } bool __wrap_esp_ds_is_busy(void) diff --git a/components/esp_tee/subproject/main/core/esp_secure_services.c b/components/esp_tee/subproject/main/core/esp_secure_services.c index ee176d437ab..b56b0c7c28c 100644 --- a/components/esp_tee/subproject/main/core/esp_secure_services.c +++ b/components/esp_tee/subproject/main/core/esp_secure_services.c @@ -583,6 +583,13 @@ int _ss_esp_tee_ota_end(void) /* ---------------------------------------------- Secure Storage ------------------------------------------------- */ +/* NOTE: The key-name pointers here (cfg->id/ctx->key_id) are REE-supplied, NULL-terminated + * NVS key names used read-only for key lookup (NVS compares them with strncmp bounded to + * NVS_KEY_NAME_MAX_SIZE-1) — never written through, never used as a register base. + * Pointing one at TEE memory yields at most a load-fault DoS or a useless presence oracle, + * so they are left unchecked. Argument checks cost code size and add latency to every + * service call, so we keep only the ones that close a real REE->TEE read/write/control-flow gap. + */ esp_err_t _ss_esp_tee_sec_storage_clear_key(const char *key_id) { return esp_tee_sec_storage_clear_key(key_id); diff --git a/components/esp_tee/subproject/main/core/esp_secure_services_iram.c b/components/esp_tee/subproject/main/core/esp_secure_services_iram.c index 7280d074749..b5030720fc4 100644 --- a/components/esp_tee/subproject/main/core/esp_secure_services_iram.c +++ b/components/esp_tee/subproject/main/core/esp_secure_services_iram.c @@ -145,6 +145,15 @@ void _ss_esprv_int_set_vectored(int rv_int_num, bool vectored) /* ---------------------------------------------- RTC_WDT ------------------------------------------------- */ +static bool is_wdt_dev_valid(const void *dev) +{ + return (dev == (const void *)&TIMERG0) +#if TIMG_LL_GET(INST_NUM) >= 2 + || (dev == (const void *)&TIMERG1) +#endif + || (dev == (const void *)RWDT_DEV_GET()); +} + void _ss_wdt_hal_init(wdt_hal_context_t *hal, wdt_inst_t wdt_inst, uint32_t prescaler, bool enable_intr) { bool valid_addr = esp_tee_buf_in_ree(hal, sizeof(wdt_hal_context_t)); @@ -159,7 +168,8 @@ void _ss_wdt_hal_init(wdt_hal_context_t *hal, wdt_inst_t wdt_inst, uint32_t pres void _ss_wdt_hal_deinit(wdt_hal_context_t *hal) { - bool valid_addr = esp_tee_buf_in_ree(hal, sizeof(wdt_hal_context_t)); + bool valid_addr = (esp_tee_buf_in_ree(hal, sizeof(wdt_hal_context_t)) && + is_wdt_dev_valid(hal->mwdt_dev)); if (!valid_addr) { return; @@ -171,6 +181,14 @@ void _ss_wdt_hal_deinit(wdt_hal_context_t *hal) /* ---------------------------------------------- Secure Storage ------------------------------------------------- */ +/* NOTE: The key-name pointers here (cfg->id/ctx->key_id) are REE-supplied, NULL-terminated + * NVS key names used read-only for key lookup (NVS compares them with strncmp bounded to + * NVS_KEY_NAME_MAX_SIZE-1) — never written through, never used as a register base. + * Pointing one at TEE memory yields at most a load-fault DoS or a useless presence oracle, + * so they are left unchecked. Argument checks cost code size and add latency to every + * service call, so we keep only the ones that close a real REE->TEE read/write/control-flow gap. + * The buffers alongside these ARE validated, since the TEE reads/writes them. + */ esp_err_t _ss_esp_tee_sec_storage_ecdsa_sign(const esp_tee_sec_storage_key_cfg_t *cfg, const uint8_t *hash, size_t hlen, esp_tee_sec_storage_ecdsa_sign_t *out_sign) { bool valid_addr = (esp_tee_buf_in_ree(cfg, sizeof(esp_tee_sec_storage_key_cfg_t)) && @@ -337,7 +355,10 @@ static bool is_flash_addr_readable(uint32_t paddr, uint32_t len) static bool is_spi_host_in_ree(spi_flash_host_inst_t *host) { - return esp_tee_buf_in_ree(host, sizeof(spi_flash_hal_context_t)); + const spi_flash_hal_context_t *ctx = (const spi_flash_hal_context_t *)host; + + return (esp_tee_buf_in_ree(host, sizeof(spi_flash_hal_context_t)) && + ctx->spi == spi_flash_ll_get_hw(SPI1_HOST)); } static bool is_spi_trans_valid(spi_flash_host_inst_t *host, spi_flash_trans_t *trans) diff --git a/components/esp_tee/subproject/main/core/esp_tee_intr.c b/components/esp_tee/subproject/main/core/esp_tee_intr.c index db5a8f59154..3cf75328c40 100644 --- a/components/esp_tee/subproject/main/core/esp_tee_intr.c +++ b/components/esp_tee/subproject/main/core/esp_tee_intr.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 */ @@ -12,6 +12,7 @@ #include "soc/periph_defs.h" #include "soc/interrupts.h" #include "soc/interrupt_reg.h" +#include "soc/soc_caps.h" #include "esp_tee.h" #include "esp_tee_intr.h" @@ -34,6 +35,10 @@ static uint32_t protected_sources[INTR_SET_COUNT]; bool esp_tee_is_intr_src_protected(int source) { + if (source < 0 || source >= ETS_MAX_INTR_SOURCE) { + return false; + } + uint32_t base = source / INTR_SET_SIZE; uint32_t offset = source % INTR_SET_SIZE; @@ -57,14 +62,9 @@ void tee_unhandled_interrupt(void *arg) /* Interrupt Matrix configuration API to call from non-secure world */ void esp_tee_route_intr_matrix(int cpu_no, uint32_t model_num, uint32_t intr_num) { - if (esp_tee_is_intr_src_protected(model_num) || intr_num == TEE_SECURE_INUM) { + if (model_num >= ETS_MAX_INTR_SOURCE || esp_tee_is_intr_src_protected(model_num)) { return; } -#if SOC_INT_CLIC_SUPPORTED - if (intr_num == TEE_PASS_INUM) { - return; - } -#endif esp_rom_route_intr_matrix(cpu_no, model_num, intr_num); ESP_LOGV(TAG, "Connected src %d to int %d (cpu %d)", model_num, intr_num, cpu_no); diff --git a/components/esp_tee/test_apps/tee_cli_app/sdkconfig.ci.release b/components/esp_tee/test_apps/tee_cli_app/sdkconfig.ci.release index dacd12be7d7..f4c39acfebb 100644 --- a/components/esp_tee/test_apps/tee_cli_app/sdkconfig.ci.release +++ b/components/esp_tee/test_apps/tee_cli_app/sdkconfig.ci.release @@ -2,8 +2,8 @@ # builds across various configurations - and is not intended for production use. # Reducing TEE IRAM size -# 29.5KB -CONFIG_SECURE_TEE_IRAM_SIZE=0x7600 +# 30KB +CONFIG_SECURE_TEE_IRAM_SIZE=0x7800 # TEE Secure Storage: Release mode CONFIG_SECURE_TEE_SEC_STG_MODE_RELEASE=y