From f3277cf2dc2d8a69b04be55fcf7be6d31782b68f Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Fri, 21 Dec 2018 15:37:57 +1100 Subject: [PATCH] hwcrypto sha: Allow SHA contexts to be shared between tasks Previously, hardware SHA engine "locks" were mutex semaphores. This meant that the task which started a particular SHA session (in hardware) needed to finalise that session, or an invalid FreeRTOS state was created. Replace with binary semaphore which can be shared between tasks. Includes a unit test, but unit test doesn't crash even without this fix (some other unknown condition is required). --- components/esp32/hwcrypto/sha.c | 125 +++++++++++++-------- components/mbedtls/test/test_mbedtls_sha.c | 44 ++++++++ 2 files changed, 123 insertions(+), 46 deletions(-) diff --git a/components/esp32/hwcrypto/sha.c b/components/esp32/hwcrypto/sha.c index ca3badf3d9..ff26fdc942 100644 --- a/components/esp32/hwcrypto/sha.c +++ b/components/esp32/hwcrypto/sha.c @@ -31,6 +31,9 @@ #include #include +#include "freertos/FreeRTOS.h" +#include "freertos/semphr.h" + #include "hwcrypto/sha.h" #include "rom/ets_sys.h" #include "soc/dport_reg.h" @@ -57,21 +60,26 @@ inline static uint32_t SHA_CONTINUE_REG(esp_sha_type sha_type) { */ static _lock_t memory_block_lock; -typedef struct { - _lock_t lock; - bool in_use; -} sha_engine_state; -/* Pointer to state of each concurrent SHA engine. +/* Binary semaphore managing the state of each concurrent SHA engine. + + Available = noone is using this SHA engine + Taken = a SHA session is running on this SHA engine Indexes: 0 = SHA1 1 = SHA2_256 2 = SHA2_384 or SHA2_512 */ -static sha_engine_state engine_states[3]; +static SemaphoreHandle_t engine_states[3]; -/* Index into the sha_engine_state array */ +static uint8_t engines_in_use; + +/* Lock for engines_in_use counter +*/ +static _lock_t engines_in_use_lock; + +/* Index into the engine_states array */ inline static size_t sha_engine_index(esp_sha_type type) { switch(type) { case SHA1: @@ -123,74 +131,89 @@ void esp_sha_unlock_memory_block(void) _lock_release(&memory_block_lock); } -/* Lock to hold when changing SHA engine state, - allows checking of sha_engines_all_idle() -*/ -static _lock_t state_change_lock; +static SemaphoreHandle_t sha_get_engine_state(esp_sha_type sha_type) +{ + unsigned idx = sha_engine_index(sha_type); + volatile SemaphoreHandle_t *engine = &engine_states[idx]; + SemaphoreHandle_t result = *engine; -inline static bool sha_engines_all_idle() { - return !engine_states[0].in_use - && !engine_states[1].in_use - && !engine_states[2].in_use; + if (result == NULL) { + // Create a new semaphore for 'in use' flag + SemaphoreHandle_t new_engine = xSemaphoreCreateBinary(); + assert(new_engine != NULL); + xSemaphoreGive(new_engine); // start available + + // try to atomically set the previously NULL *engine to new_engine + uint32_t set_engine = (uint32_t)new_engine; + uxPortCompareSet((volatile uint32_t *)engine, 0, &set_engine); + + if (set_engine != 0) { // we lost a race setting *engine + vSemaphoreDelete(new_engine); + } + result = *engine; + } + return result; } -static void esp_sha_lock_engine_inner(sha_engine_state *engine); +static bool esp_sha_lock_engine_common(esp_sha_type sha_type, TickType_t ticks_to_wait); bool esp_sha_try_lock_engine(esp_sha_type sha_type) { - sha_engine_state *engine = &engine_states[sha_engine_index(sha_type)]; - if(_lock_try_acquire(&engine->lock) != 0) { - /* This SHA engine is already in use */ - return false; - } else { - esp_sha_lock_engine_inner(engine); - return true; - } + return esp_sha_lock_engine_common(sha_type, 0); } void esp_sha_lock_engine(esp_sha_type sha_type) { - sha_engine_state *engine = &engine_states[sha_engine_index(sha_type)]; - _lock_acquire(&engine->lock); - esp_sha_lock_engine_inner(engine); + esp_sha_lock_engine_common(sha_type, portMAX_DELAY); } -static void esp_sha_lock_engine_inner(sha_engine_state *engine) +static bool esp_sha_lock_engine_common(esp_sha_type sha_type, TickType_t ticks_to_wait) { - _lock_acquire(&state_change_lock); + SemaphoreHandle_t engine_state = sha_get_engine_state(sha_type); + BaseType_t result = xSemaphoreTake(engine_state, ticks_to_wait); - if (sha_engines_all_idle()) { - /* Enable SHA hardware */ + if (result == pdFALSE) { + // failed to take semaphore + return false; + } + + _lock_acquire(&engines_in_use_lock); + + if (engines_in_use == 0) { + /* Just locked first engine, + so enable SHA hardware */ periph_module_enable(PERIPH_SHA_MODULE); DPORT_STALL_OTHER_CPU_START(); ets_sha_enable(); DPORT_STALL_OTHER_CPU_END(); } - assert( !engine->in_use && "in_use flag should be cleared" ); - engine->in_use = true; + engines_in_use++; + assert(engines_in_use <= 3); - _lock_release(&state_change_lock); + _lock_release(&engines_in_use_lock); + + return true; } void esp_sha_unlock_engine(esp_sha_type sha_type) { - sha_engine_state *engine = &engine_states[sha_engine_index(sha_type)]; + SemaphoreHandle_t *engine_state = sha_get_engine_state(sha_type); - _lock_acquire(&state_change_lock); + _lock_acquire(&engines_in_use_lock); - assert( engine->in_use && "in_use flag should be set" ); - engine->in_use = false; + engines_in_use--; - if (sha_engines_all_idle()) { - /* Disable SHA hardware */ + if (engines_in_use == 0) { + /* About to release last engine, so + disable SHA hardware */ periph_module_disable(PERIPH_SHA_MODULE); } - _lock_release(&state_change_lock); + _lock_release(&engines_in_use_lock); - _lock_release(&engine->lock); + xSemaphoreGive(engine_state); } void esp_sha_wait_idle(void) @@ -207,8 +230,13 @@ void esp_sha_wait_idle(void) void esp_sha_read_digest_state(esp_sha_type sha_type, void *digest_state) { - sha_engine_state *engine = &engine_states[sha_engine_index(sha_type)]; - assert(engine->in_use && "SHA engine should be locked" ); +#ifndef NDEBUG + { + SemaphoreHandle_t *engine_state = sha_get_engine_state(sha_type); + assert(uxSemaphoreGetCount(engine_state) == 0 && + "SHA engine should be locked" ); + } +#endif esp_sha_lock_memory_block(); @@ -234,8 +262,13 @@ void esp_sha_read_digest_state(esp_sha_type sha_type, void *digest_state) void esp_sha_block(esp_sha_type sha_type, const void *data_block, bool is_first_block) { - sha_engine_state *engine = &engine_states[sha_engine_index(sha_type)]; - assert(engine->in_use && "SHA engine should be locked" ); +#ifndef NDEBUG + { + SemaphoreHandle_t *engine_state = sha_get_engine_state(sha_type); + assert(uxSemaphoreGetCount(engine_state) == 0 && + "SHA engine should be locked" ); + } +#endif esp_sha_lock_memory_block(); diff --git a/components/mbedtls/test/test_mbedtls_sha.c b/components/mbedtls/test/test_mbedtls_sha.c index aa81d63032..7b94e52cf7 100644 --- a/components/mbedtls/test/test_mbedtls_sha.c +++ b/components/mbedtls/test/test_mbedtls_sha.c @@ -255,3 +255,47 @@ TEST_CASE("mbedtls SHA256 clone", "[mbedtls]") TEST_ASSERT_EQUAL(0, mbedtls_sha256_finish_ret(&clone, sha256)); TEST_ASSERT_EQUAL_MEMORY_MESSAGE(sha256_thousand_as, sha256, 32, "SHA256 cloned calculation"); } + +typedef struct { + mbedtls_sha256_context ctx; + uint8_t result[32]; + int ret; + bool done; +} finalise_sha_param_t; + +static void tskFinaliseSha(void *v_param) +{ + finalise_sha_param_t *param = (finalise_sha_param_t *)v_param; + + for (int i = 0; i < 5; i++) { + TEST_ASSERT_EQUAL(0, mbedtls_sha256_update_ret(¶m->ctx, one_hundred_as, 100)); + } + + param->ret = mbedtls_sha256_finish_ret(¶m->ctx, param->result); + param->done = true; + vTaskDelete(NULL); +} + +TEST_CASE("mbedtls SHA session passed between tasks" , "[mbedtls]") +{ + finalise_sha_param_t param = { 0 }; + + mbedtls_sha256_init(¶m.ctx); + TEST_ASSERT_EQUAL(0, mbedtls_sha256_starts_ret(¶m.ctx, false)); + for (int i = 0; i < 5; i++) { + TEST_ASSERT_EQUAL(0, mbedtls_sha256_update_ret(¶m.ctx, one_hundred_as, 100)); + } + + // pass the SHA context off to a different task + // + // note: at the moment this doesn't crash even if a mutex semaphore is used as the + // engine lock, but it can crash... + xTaskCreate(tskFinaliseSha, "SHAFinalise", SHA_TASK_STACK_SIZE, ¶m, 3, NULL); + + while (!param.done) { + vTaskDelay(1); + } + + TEST_ASSERT_EQUAL(0, param.ret); + TEST_ASSERT_EQUAL_MEMORY_MESSAGE(sha256_thousand_as, param.result, 32, "SHA256 result from other task"); +}