From de3a510a18a4db4c193848e42c2762f67b1873c1 Mon Sep 17 00:00:00 2001 From: "harshal.patil" Date: Thu, 3 Sep 2026 19:08:48 +0530 Subject: [PATCH] fix(esp_security): cover the crypto reset coupling in the driver locks A peripheral's reset also resets the ones it occupies, so a lock has to cover both. Gate the ECDSA MPI lock on SOC_ECDSA_USES_MPI rather than the runtime ecdsa_ll_is_mpi_required() and set that capability on C5, lock the Key Manager path in esp_key_mgr.c, clean HMAC after its reset, and enable DS before the primitives its reset covers. --- .../esp32c5/include/hal/ecdsa_ll.h | 10 ++- components/esp_security/src/esp_crypto_lock.c | 78 +++++++++++-------- .../esp_security/src/esp_crypto_periph_clk.c | 1 + components/esp_security/src/esp_ds.c | 7 +- components/esp_security/src/esp_key_mgr.c | 46 +++++++---- .../esp32c5/include/soc/Kconfig.soc_caps.in | 4 + components/soc/esp32c5/include/soc/soc_caps.h | 1 + components/soc/esp32h2/include/soc/soc_caps.h | 2 +- .../soc/esp32h21/include/soc/soc_caps.h | 1 + components/soc/esp32p4/include/soc/soc_caps.h | 2 +- 10 files changed, 100 insertions(+), 52 deletions(-) diff --git a/components/esp_hal_security/esp32c5/include/hal/ecdsa_ll.h b/components/esp_hal_security/esp32c5/include/hal/ecdsa_ll.h index 031df896ddb..5ff24638c3e 100644 --- a/components/esp_hal_security/esp32c5/include/hal/ecdsa_ll.h +++ b/components/esp_hal_security/esp32c5/include/hal/ecdsa_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -445,6 +445,14 @@ __attribute__((always_inline)) static inline void ecdsa_ll_set_ecdsa_key_blk(ecd } } +/** + * @brief Check if the ECDSA peripheral uses MPI module's memory + */ +static inline bool ecdsa_ll_is_mpi_required(void) +{ + return false; +} + /** * @brief Check if the ECDSA peripheral is supported on this chip revision * For ESP32-C5, ECDSA is always supported diff --git a/components/esp_security/src/esp_crypto_lock.c b/components/esp_security/src/esp_crypto_lock.c index 7dd20eff564..f98b267d03a 100644 --- a/components/esp_security/src/esp_crypto_lock.c +++ b/components/esp_security/src/esp_crypto_lock.c @@ -8,16 +8,41 @@ #include "esp_crypto_lock.h" -/* Lock overview: -SHA: peripheral independent, but DMA is shared with AES -AES: peripheral independent, but DMA is shared with SHA -MPI/RSA: independent -ECC: independent -HMAC: needs SHA -DS: needs HMAC (which needs SHA), AES and MPI -ECDSA: needs ECC and MPI, and its reset pulse holds SHA (and thus the SHA/AES DMA) in reset -Key Manager: shared key-usage selectors (ECDSA/HMAC/DS/XTS-AES flash); - esp_crypto_key_mgr_enable_periph_clk(true) resets it +/* Lock overview. + + Two separate relations decide what a lock must cover: + + 1. Functional dependency - which peripherals an operation drives: + SHA: independent, but DMA is shared with AES + AES: independent, but DMA is shared with SHA + MPI/RSA: independent + ECC: independent + HMAC: needs SHA + DS: needs HMAC (which needs SHA), AES and MPI + ECDSA: needs ECC, SHA where the K value is derived deterministically or + the Z value is taken from SHA rather than supplied, and MPI on + some targets + + 2. Reset coupling - which peripherals are also reset when this one's RST_EN is + pulsed, because the hardware reset tree is shared: + AES/SHA/MPI/ECC: itself only + HMAC: HMAC, SHA + DS: DS, AES, SHA, MPI + ECDSA: ECDSA, SHA, ECC, and MPI where SOC_ECDSA_USES_MPI + KM: KM, AES, ECC + + A lock must cover the union of both. The reset coupling is why the ECDSA lock + takes the SHA/AES and MPI locks even though an ECDSA operation does not + necessarily use those engines. + + The Key Manager holds key usage selectors shared by ECDSA, HMAC, DS and the + XTS-AES engines. The accelerator paths take the Key Manager lock around the + clock enable that lets those selectors be written; only the Key Manager's own + driver resets the peripheral, because that reset is one of the couplings above. + + + Acquisition order, which every path must follow to stay deadlock-free: + DS -> ECDSA -> HMAC -> ECC -> SHA/AES -> MPI -> Key Manager */ #if !NON_OS_BUILD @@ -49,9 +74,6 @@ static _lock_t s_crypto_ecc_lock; #ifdef SOC_ECDSA_SUPPORTED /* Lock for ECDSA peripheral */ static _lock_t s_crypto_ecdsa_lock; -#if SOC_ECDSA_USES_MPI -#include "hal/ecdsa_ll.h" -#endif /* SOC_ECDSA_USES_MPI */ #endif /* SOC_ECDSA_SUPPORTED */ #if SOC_KEY_MANAGER_SUPPORT_KEY_DEPLOYMENT @@ -143,32 +165,22 @@ void esp_crypto_ecdsa_lock_acquire(void) _lock_acquire(&s_crypto_ecdsa_lock); esp_crypto_ecc_lock_acquire(); #if defined(SOC_SHA_SUPPORTED) || defined(SOC_AES_SUPPORTED) - /* Enabling the ECDSA peripheral pulses the ECDSA reset - (esp_crypto_ecdsa_enable_periph_clk() -> ecdsa_ll_reset_register()), and on every - target that has an ECDSA peripheral that reset also holds SHA in reset: see the - "otherwise SHA is held in reset" note in sha_ll_reset_register(). SHA shares its - (G)DMA channel with AES, and the SHA/AES lock is what serializes both of them, so - it has to be held across the pulse. Without it, a hardware ECDSA operation on one - core lands in the middle of an unrelated SHA or AES transfer on the other core, - which completes without an error but yields wrong output. - Taken before the MPI lock to keep the acquisition order of - esp_crypto_ds_lock_acquire() (SHA/AES before MPI) and avoid a lock cycle. */ + /* The ECDSA reset holds SHA, which shares its DMA with AES. Taken before MPI + to keep esp_crypto_ds_lock_acquire()'s order. */ esp_crypto_sha_aes_lock_acquire(); #endif /* defined(SOC_SHA_SUPPORTED) || defined(SOC_AES_SUPPORTED) */ -#ifdef SOC_ECDSA_USES_MPI - if (ecdsa_ll_is_mpi_required()) { - esp_crypto_mpi_lock_acquire(); - } -#endif /* SOC_ECDSA_USES_MPI */ + /* Unconditional under the cap: the reset coupling is present whether or not + this revision needs the MPI engine. */ +#if (SOC_MPI_SUPPORTED && SOC_ECDSA_USES_MPI) + esp_crypto_mpi_lock_acquire(); +#endif /* (SOC_MPI_SUPPORTED && SOC_ECDSA_USES_MPI) */ } void esp_crypto_ecdsa_lock_release(void) { -#ifdef SOC_ECDSA_USES_MPI - if (ecdsa_ll_is_mpi_required()) { - esp_crypto_mpi_lock_release(); - } -#endif /* SOC_ECDSA_USES_MPI */ +#if (SOC_MPI_SUPPORTED && SOC_ECDSA_USES_MPI) + esp_crypto_mpi_lock_release(); +#endif /* (SOC_MPI_SUPPORTED && SOC_ECDSA_USES_MPI) */ #if defined(SOC_SHA_SUPPORTED) || defined(SOC_AES_SUPPORTED) esp_crypto_sha_aes_lock_release(); #endif /* defined(SOC_SHA_SUPPORTED) || defined(SOC_AES_SUPPORTED) */ diff --git a/components/esp_security/src/esp_crypto_periph_clk.c b/components/esp_security/src/esp_crypto_periph_clk.c index f07a3307abb..d8ed0ebb6cb 100644 --- a/components/esp_security/src/esp_crypto_periph_clk.c +++ b/components/esp_security/src/esp_crypto_periph_clk.c @@ -123,6 +123,7 @@ void esp_crypto_hmac_enable_periph_clk(bool enable) hmac_ll_enable_bus_clock(enable); if (enable) { hmac_ll_reset_register(); + hmac_ll_clean(); } } } diff --git a/components/esp_security/src/esp_ds.c b/components/esp_security/src/esp_ds.c index b713ff9558c..368162349a8 100644 --- a/components/esp_security/src/esp_ds.c +++ b/components/esp_security/src/esp_ds.c @@ -259,11 +259,12 @@ static void ds_acquire_enable(void) { esp_crypto_ds_lock_acquire(); - // We also enable SHA and HMAC here. SHA is used by HMAC, HMAC is used by DS. + /* DS first: its reset also resets AES, SHA and MPI, so anything enabled + before it would be reset again here. */ + esp_crypto_ds_enable_periph_clk(true); esp_crypto_hmac_enable_periph_clk(true); esp_crypto_sha_enable_periph_clk(true); esp_crypto_mpi_enable_periph_clk(true); - esp_crypto_ds_enable_periph_clk(true); #if SOC_KEY_MANAGER_DS_KEY_DEPLOY /* Key Manager holds the key usage selector register(efuse vs own key). @@ -286,10 +287,10 @@ static void ds_disable_release(void) esp_crypto_key_manager_lock_release(); #endif /* SOC_KEY_MANAGER_DS_KEY_DEPLOY */ - esp_crypto_ds_enable_periph_clk(false); esp_crypto_mpi_enable_periph_clk(false); esp_crypto_sha_enable_periph_clk(false); esp_crypto_hmac_enable_periph_clk(false); + esp_crypto_ds_enable_periph_clk(false); esp_crypto_ds_lock_release(); } diff --git a/components/esp_security/src/esp_key_mgr.c b/components/esp_security/src/esp_key_mgr.c index 2540573f457..8dcf083f37f 100644 --- a/components/esp_security/src/esp_key_mgr.c +++ b/components/esp_security/src/esp_key_mgr.c @@ -118,13 +118,27 @@ static void esp_key_mgr_release_key_lock(esp_key_mgr_key_type_t key_type) } #endif /* NON_OS_BUILD */ +/* The Key Manager reset also resets AES and ECC, and the sequences guarded here + drive the state machine and write the shared key usage selector. Callers of + esp_key_mgr_acquire_hardware()/release_hardware() hold all three locks. */ +static void key_mgr_crypto_lock_acquire(void) +{ + esp_crypto_ecc_lock_acquire(); + esp_crypto_sha_aes_lock_acquire(); + esp_crypto_key_manager_lock_acquire(); +} + +static void key_mgr_crypto_lock_release(void) +{ + esp_crypto_key_manager_lock_release(); + esp_crypto_sha_aes_lock_release(); + esp_crypto_ecc_lock_release(); +} + static void esp_key_mgr_acquire_hardware(bool deployment_mode) { if (deployment_mode) { - // We only need explicit locks in the deployment mode - esp_crypto_ecc_lock_acquire(); - esp_crypto_sha_aes_lock_acquire(); - esp_crypto_key_manager_lock_acquire(); + key_mgr_crypto_lock_acquire(); // The KM peripheral uses the external ECC block for the ECDH0/ECDH1 // scalar multiplications; its bus clock must be on, otherwise the KM // deploys an incorrect key. @@ -132,7 +146,6 @@ static void esp_key_mgr_acquire_hardware(bool deployment_mode) esp_crypto_ecc_enable_periph_clk(true); #endif } - // Reset the Key Manager Clock esp_crypto_key_mgr_enable_periph_clk(true); } @@ -142,13 +155,12 @@ static void esp_key_mgr_release_hardware(bool deployment_mode) #if SOC_ECC_SUPPORTED esp_crypto_ecc_enable_periph_clk(false); #endif - esp_crypto_key_manager_lock_release(); - esp_crypto_sha_aes_lock_release(); - esp_crypto_ecc_lock_release(); } - - // Reset the Key Manager Clock esp_crypto_key_mgr_enable_periph_clk(false); + + if (deployment_mode) { + key_mgr_crypto_lock_release(); + } } /** @@ -604,12 +616,12 @@ esp_err_t esp_key_mgr_activate_key(esp_key_mgr_key_recovery_info_t *key_recovery esp_key_mgr_acquire_key_lock(key_type); + key_mgr_crypto_lock_acquire(); esp_key_mgr_acquire_hardware(false); esp_err_t esp_ret = key_mgr_recover_key(&key_recovery_config); if (esp_ret != ESP_OK) { ESP_LOGE(TAG, "Failed to recover key"); - esp_key_mgr_release_key_lock(key_type); goto cleanup; } @@ -619,7 +631,6 @@ esp_err_t esp_key_mgr_activate_key(esp_key_mgr_key_recovery_info_t *key_recovery esp_ret = key_mgr_recover_key(&key_recovery_config); if (esp_ret != ESP_OK) { ESP_LOGE(TAG, "Failed to recover key"); - esp_key_mgr_release_key_lock(key_type); goto cleanup; } } @@ -627,19 +638,28 @@ esp_err_t esp_key_mgr_activate_key(esp_key_mgr_key_recovery_info_t *key_recovery // Set the Key Manager Static Register to use own key for the respective key type key_mgr_hal_set_key_usage(key_type, ESP_KEY_MGR_USE_OWN_KEY); + /* Released here: nothing after this point drives the peripheral. */ + key_mgr_crypto_lock_release(); + ESP_LOGD(TAG, "Key activation for type %d successful", key_type); return ESP_OK; cleanup: ESP_LOGE(TAG, "Key activation failed"); esp_key_mgr_release_hardware(false); + key_mgr_crypto_lock_release(); + esp_key_mgr_release_key_lock(key_type); return esp_ret; } esp_err_t esp_key_mgr_deactivate_key(esp_key_mgr_key_type_t key_type) { - esp_key_mgr_release_key_lock(key_type); + key_mgr_crypto_lock_acquire(); esp_key_mgr_release_hardware(false); + key_mgr_crypto_lock_release(); + + esp_key_mgr_release_key_lock(key_type); + ESP_LOGD(TAG, "Key deactivation successful for type %d", key_type); return ESP_OK; } diff --git a/components/soc/esp32c5/include/soc/Kconfig.soc_caps.in b/components/soc/esp32c5/include/soc/Kconfig.soc_caps.in index 2741a627a3c..47f8781ca0a 100644 --- a/components/soc/esp32c5/include/soc/Kconfig.soc_caps.in +++ b/components/soc/esp32c5/include/soc/Kconfig.soc_caps.in @@ -931,6 +931,10 @@ config SOC_ECDSA_SUPPORT_DETERMINISTIC_MODE bool default y +config SOC_ECDSA_USES_MPI + bool + default y + config SOC_ECDSA_SUPPORT_HW_DETERMINISTIC_LOOP bool default y diff --git a/components/soc/esp32c5/include/soc/soc_caps.h b/components/soc/esp32c5/include/soc/soc_caps.h index 4f28358f4b2..eda95a4ca22 100644 --- a/components/soc/esp32c5/include/soc/soc_caps.h +++ b/components/soc/esp32c5/include/soc/soc_caps.h @@ -384,6 +384,7 @@ /*--------------------------- ECDSA CAPS ---------------------------------------*/ #define SOC_ECDSA_SUPPORT_EXPORT_PUBKEY (1) #define SOC_ECDSA_SUPPORT_DETERMINISTIC_MODE (1) +#define SOC_ECDSA_USES_MPI (1) /*!< ECDSA shares MPI's reset domain: v1.0 dropped ECDSA's use of RSA but kept the clkrst coupling, so the MPI lock is still required */ #define SOC_ECDSA_SUPPORT_HW_DETERMINISTIC_LOOP (1) #define SOC_ECDSA_SUPPORT_CURVE_P384 (1) #define SOC_ECDSA_SUPPORT_CURVE_SPECIFIC_KEY_PURPOSES (1) /*!< Support individual key purposes for different ECDSA curves (P192, P256, P384) */ diff --git a/components/soc/esp32h2/include/soc/soc_caps.h b/components/soc/esp32h2/include/soc/soc_caps.h index 8e08c1b4c6f..af51f2553d3 100644 --- a/components/soc/esp32h2/include/soc/soc_caps.h +++ b/components/soc/esp32h2/include/soc/soc_caps.h @@ -429,7 +429,7 @@ #define SOC_ECC_CONSTANT_TIME_POINT_MUL 1 /*------------------------- ECDSA CAPS -------------------------*/ -#define SOC_ECDSA_USES_MPI (1) +#define SOC_ECDSA_USES_MPI (1) /*!< ECDSA reuses the MPI operand memory below rev v1.2, and shares MPI's reset domain on every revision */ #define SOC_ECDSA_SUPPORT_DETERMINISTIC_MODE (1) #define SOC_ECDSA_SUPPORT_HW_DETERMINISTIC_LOOP (1) #define SOC_ECDSA_P192_CURVE_DEFAULT_DISABLED (1) diff --git a/components/soc/esp32h21/include/soc/soc_caps.h b/components/soc/esp32h21/include/soc/soc_caps.h index 49f329fb98b..ac809a885da 100644 --- a/components/soc/esp32h21/include/soc/soc_caps.h +++ b/components/soc/esp32h21/include/soc/soc_caps.h @@ -408,6 +408,7 @@ #define SOC_ECDSA_SUPPORT_DETERMINISTIC_MODE (1) #define SOC_ECDSA_SUPPORT_HW_DETERMINISTIC_LOOP (1) #define SOC_ECDSA_P192_CURVE_DEFAULT_DISABLED (1) +// #define SOC_ECDSA_USES_MPI 1 // TODO: [ESP32H21] IDF-16142 /*-------------------------- UART CAPS ---------------------------------------*/ // ESP32-H21 has 2 UARTs diff --git a/components/soc/esp32p4/include/soc/soc_caps.h b/components/soc/esp32p4/include/soc/soc_caps.h index c5340114575..9d94437b4fc 100644 --- a/components/soc/esp32p4/include/soc/soc_caps.h +++ b/components/soc/esp32p4/include/soc/soc_caps.h @@ -496,7 +496,7 @@ #define SOC_ECDSA_SUPPORT_EXPORT_PUBKEY (1) #define SOC_ECDSA_SUPPORT_DETERMINISTIC_MODE (1) #define SOC_ECDSA_SUPPORT_HW_DETERMINISTIC_LOOP (1) -#define SOC_ECDSA_USES_MPI (1) +#define SOC_ECDSA_USES_MPI (1) /*!< ECDSA shares MPI's reset domain, so the MPI lock is required even though ECDSA uses neither the MPI engine nor its memory */ #define SOC_ECDSA_SUPPORT_CURVE_P384 (1) #define SOC_ECDSA_SUPPORT_CURVE_SPECIFIC_KEY_PURPOSES (1) /*!< Support individual key purposes for different ECDSA curves (P192, P256, P384) */