Skip to content

Commit ca44675

Browse files
committed
[Nuvoton] Fix crypto AC management
1. For SHA AC, use atomic flag to manage its ownership. (1) Nuvoton SHA AC doesn't support SHA context save & restore, so S/W SHA fallback has been supported before. To make non-blocking 'acquire' semantics clearer, introduce 'try_acquire' to substitute for 'acquire'. (2) No biting CPU due to mechanism above. (3) No deadlock due to mechanism above. 2. For AES/DES/ECC AC, change to mutex to manage their ownership. (1) Change crypto-misc.c to crypto-misc.cpp to utilize C++ SingletonPtr which guarantees thread-safe mutex construct-on-first-use. (2) With change to crypto-misc.cpp, add 'extern "C"' modifier to CRYPTO_IRQHandler() to avoid name mangling in C++. (3) No priority inversion because mutex has osMutexPrioInherit attribute bit set. (4) No deadlock because these AC are all locked for a short sequence of operations rather than the whole lifetime of mbedtls context. (5) For double mbedtls_internal_ecp_init() issue, it has been fixed in upper mbedtls layer. So no need to change ecc init/free flow.
1 parent b16b1db commit ca44675

File tree

14 files changed

+178
-119
lines changed

14 files changed

+178
-119
lines changed

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/aes/aes_alt.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,9 @@ static void __nvt_aes_crypt( mbedtls_aes_context *ctx,
144144
error("Buffer for AES alter. DMA requires to be word-aligned and located in 0x20000000-0x2FFFFFFF region.");
145145
}
146146

147-
/* TODO: Change busy-wait to other means to release CPU */
148147
/* Acquire ownership of AES H/W */
149-
while (! crypto_aes_acquire());
150-
148+
crypto_aes_acquire();
149+
151150
/* Init crypto module */
152151
crypto_init();
153152
/* Enable AES interrupt */

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/des/des_alt.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -349,10 +349,9 @@ static int mbedtls_des_docrypt(uint16_t keyopt, uint8_t key[3][MBEDTLS_DES_KEY_S
349349
error("Buffer for DES alter. DMA requires to be word-aligned and located in 0x20000000-0x2FFFFFFF region.");
350350
}
351351

352-
/* TODO: Change busy-wait to other means to release CPU */
353352
/* Acquire ownership of DES H/W */
354-
while (! crypto_des_acquire());
355-
353+
crypto_des_acquire();
354+
356355
/* Init crypto module */
357356
crypto_init();
358357
/* Enable DES interrupt */

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/ecp/ecp_internal_alt.c

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -223,12 +223,23 @@ unsigned char mbedtls_internal_ecp_grp_capable( const mbedtls_ecp_group *grp )
223223

224224
int mbedtls_internal_ecp_init( const mbedtls_ecp_group *grp )
225225
{
226-
/* TODO: Change busy-wait with other means to release CPU */
226+
/* Behavior of mbedtls_internal_ecp_init()/mbedtls_internal_ecp_free()
227+
*
228+
* mbedtls_internal_ecp_init()/mbedtls_internal_ecp_free() are like pre-op/post-op calls
229+
* and they guarantee:
230+
*
231+
* 1. Paired
232+
* 2. No overlapping
233+
* 3. Upper public function cannot return when ECP alter. is still activated.
234+
*/
235+
227236
/* Acquire ownership of ECC accelerator */
228-
while (! crypto_ecc_acquire());
237+
crypto_ecc_acquire();
229238

230-
/* Init crypto module */
239+
/* Initialize crypto module */
231240
crypto_init();
241+
242+
/* Enable ECC interrupt */
232243
ECC_ENABLE_INT();
233244

234245
return 0;
@@ -238,9 +249,10 @@ void mbedtls_internal_ecp_free( const mbedtls_ecp_group *grp )
238249
{
239250
/* Disable ECC interrupt */
240251
ECC_DISABLE_INT();
252+
241253
/* Uninit crypto module */
242254
crypto_uninit();
243-
255+
244256
/* Release ownership of ECC accelerator */
245257
crypto_ecc_release();
246258
}
@@ -590,7 +602,7 @@ NU_STATIC int internal_run_eccop(const mbedtls_ecp_group *grp,
590602
ret = MBEDTLS_ERR_ECP_BAD_INPUT_DATA;
591603
goto cleanup;
592604
}
593-
605+
594606
/* Configure ECC curve coefficients A/B */
595607
/* Special case for A = -3 */
596608
if (grp->A.p == NULL) {
@@ -644,7 +656,7 @@ NU_STATIC int internal_run_eccop(const mbedtls_ecp_group *grp,
644656
cleanup:
645657

646658
mbedtls_mpi_free(&N_);
647-
659+
648660
return ret;
649661
}
650662

@@ -698,7 +710,7 @@ NU_STATIC int internal_run_modop(mbedtls_mpi *r,
698710
const mbedtls_mpi *Np;
699711

700712
mbedtls_mpi_init(&N_);
701-
713+
702714
/* Use INTERNAL_MPI_NORM(Np, N1, N_, P) to get normalized MPI
703715
*
704716
* N_: Holds normalized MPI if the passed-in MPI N1 is not

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/sha/sha1_alt.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
*/
3232
static void mbedtls_sha1_init_internal(mbedtls_sha1_context *ctx, int try_hw)
3333
{
34-
if (try_hw && crypto_sha_acquire()) {
34+
if (try_hw && crypto_sha_try_acquire()) {
3535
ctx->active_ctx = &ctx->hw_ctx;
3636
mbedtls_sha1_hw_init(&ctx->hw_ctx);
3737
} else {

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/sha/sha256_alt.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
*/
3232
static void mbedtls_sha256_init_internal(mbedtls_sha256_context *ctx, int try_hw)
3333
{
34-
if (try_hw && crypto_sha_acquire()) {
34+
if (try_hw && crypto_sha_try_acquire()) {
3535
ctx->active_ctx = &ctx->hw_ctx;
3636
mbedtls_sha256_hw_init(&ctx->hw_ctx);
3737
} else {

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/sha/sha512_alt.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
*/
3232
static void mbedtls_sha512_init_internal(mbedtls_sha512_context *ctx, int try_hw)
3333
{
34-
if (try_hw && crypto_sha_acquire()) {
34+
if (try_hw && crypto_sha_try_acquire()) {
3535
ctx->active_ctx = &ctx->hw_ctx;
3636
mbedtls_sha512_hw_init(&ctx->hw_ctx);
3737
} else {

features/mbedtls/targets/TARGET_NUVOTON/TARGET_NUC472/aes/aes_alt.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,9 @@ static void __nvt_aes_crypt( mbedtls_aes_context *ctx,
144144
error("Buffer for AES alter. DMA requires to be word-aligned and located in 0x20000000-0x2FFFFFFF region.");
145145
}
146146

147-
/* TODO: Change busy-wait to other means to release CPU */
148147
/* Acquire ownership of AES H/W */
149-
while (! crypto_aes_acquire());
150-
148+
crypto_aes_acquire();
149+
151150
/* Init crypto module */
152151
crypto_init();
153152
/* Enable AES interrupt */

features/mbedtls/targets/TARGET_NUVOTON/TARGET_NUC472/des/des_alt.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -348,11 +348,10 @@ static int mbedtls_des_docrypt(uint16_t keyopt, uint8_t key[3][MBEDTLS_DES_KEY_S
348348
(! crypto_dma_buff_compat(dmabuf_out, MAXSIZE_DMABUF, 8))) {
349349
error("Buffer for DES alter. DMA requires to be word-aligned and located in 0x20000000-0x2FFFFFFF region.");
350350
}
351-
352-
/* TODO: Change busy-wait to other means to release CPU */
351+
353352
/* Acquire ownership of DES H/W */
354-
while (! crypto_des_acquire());
355-
353+
crypto_des_acquire();
354+
356355
/* Init crypto module */
357356
crypto_init();
358357
/* Enable DES interrupt */

features/mbedtls/targets/TARGET_NUVOTON/TARGET_NUC472/sha/sha1_alt.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
*/
3232
static void mbedtls_sha1_init_internal(mbedtls_sha1_context *ctx, int try_hw)
3333
{
34-
if (try_hw && crypto_sha_acquire()) {
34+
if (try_hw && crypto_sha_try_acquire()) {
3535
ctx->active_ctx = &ctx->hw_ctx;
3636
mbedtls_sha1_hw_init(&ctx->hw_ctx);
3737
} else {

features/mbedtls/targets/TARGET_NUVOTON/TARGET_NUC472/sha/sha256_alt.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
*/
3232
static void mbedtls_sha256_init_internal(mbedtls_sha256_context *ctx, int try_hw)
3333
{
34-
if (try_hw && crypto_sha_acquire()) {
34+
if (try_hw && crypto_sha_try_acquire()) {
3535
ctx->active_ctx = &ctx->hw_ctx;
3636
mbedtls_sha256_hw_init(&ctx->hw_ctx);
3737
} else {

targets/TARGET_NUVOTON/TARGET_M480/crypto/crypto-misc.c renamed to targets/TARGET_NUVOTON/TARGET_M480/crypto/crypto-misc.cpp

Lines changed: 56 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -19,26 +19,48 @@
1919
#include "mbed_assert.h"
2020
#include "mbed_critical.h"
2121
#include "mbed_error.h"
22+
#include "cmsis_os2.h"
23+
#include "mbed_rtos_storage.h"
24+
#include <string.h>
2225
#include <limits.h>
2326
#include "nu_modutil.h"
2427
#include "nu_bitutil.h"
2528
#include "crypto-misc.h"
29+
#include "SingletonPtr.h"
30+
#include "Mutex.h"
31+
32+
/* Consideration for choosing proper synchronization mechanism
33+
*
34+
* 1. We choose mutex to synchronize access to crypto non-SHA AC. We can guarantee:
35+
* (1) No deadlock
36+
* We just lock mutex for a short sequence of operations rather than the whole lifetime
37+
* of crypto context.
38+
* (2) No priority inversion
39+
* Mutex supports priority inheritance and it is enabled.
40+
* 2. We choose atomic flag to synchronize access to crypto SHA AC. We can guarantee:
41+
* (1) No deadlock
42+
* With SHA AC not supporting context save & restore, we provide SHA S/W fallback when
43+
* SHA AC is not available.
44+
* (2) No biting CPU
45+
* Same reason as above.
46+
*/
47+
48+
/* Mutex for crypto AES AC management */
49+
static SingletonPtr<rtos::Mutex> crypto_aes_mutex;
50+
51+
/* Mutex for crypto DES AC management */
52+
static SingletonPtr<rtos::Mutex> crypto_des_mutex;
53+
54+
/* Mutex for crypto ECC AC management */
55+
static SingletonPtr<rtos::Mutex> crypto_ecc_mutex;
56+
57+
/* Atomic flag for crypto SHA AC management */
58+
static core_util_atomic_flag crypto_sha_atomic_flag = CORE_UTIL_ATOMIC_FLAG_INIT;
2659

27-
/* Track if AES H/W is available */
28-
static uint16_t crypto_aes_avail = 1;
29-
/* Track if DES H/W is available */
30-
static uint16_t crypto_des_avail = 1;
31-
/* Track if SHA H/W is available */
32-
static uint16_t crypto_sha_avail = 1;
33-
/* Track if ECC H/W is available */
34-
static uint16_t crypto_ecc_avail = 1;
3560

3661
/* Crypto (AES, DES, SHA, etc.) init counter. Crypto's keeps active as it is non-zero. */
3762
static uint16_t crypto_init_counter = 0U;
3863

39-
static bool crypto_submodule_acquire(uint16_t *submodule_avail);
40-
static void crypto_submodule_release(uint16_t *submodule_avail);
41-
4264
/* Crypto done flags */
4365
#define CRYPTO_DONE_OK BIT0 /* Done with OK */
4466
#define CRYPTO_DONE_ERR BIT1 /* Done with error */
@@ -119,44 +141,52 @@ void crypto_zeroize32(uint32_t *v, size_t n)
119141
}
120142
}
121143

122-
bool crypto_aes_acquire(void)
144+
void crypto_aes_acquire(void)
123145
{
124-
return crypto_submodule_acquire(&crypto_aes_avail);
146+
/* Don't check return code of Mutex::lock(void)
147+
*
148+
* This function treats RTOS errors as fatal system errors, so it can only return osOK.
149+
* Use of the return value is deprecated, as the return is expected to become void in
150+
* the future.
151+
*/
152+
crypto_aes_mutex->lock();
125153
}
126154

127155
void crypto_aes_release(void)
128156
{
129-
crypto_submodule_release(&crypto_aes_avail);
157+
crypto_aes_mutex->unlock();
130158
}
131159

132-
bool crypto_des_acquire(void)
160+
void crypto_des_acquire(void)
133161
{
134-
return crypto_submodule_acquire(&crypto_des_avail);
162+
/* Don't check return code of Mutex::lock(void) */
163+
crypto_des_mutex->lock();
135164
}
136165

137166
void crypto_des_release(void)
138167
{
139-
crypto_submodule_release(&crypto_des_avail);
168+
crypto_des_mutex->unlock();
140169
}
141170

142-
bool crypto_sha_acquire(void)
171+
void crypto_ecc_acquire(void)
143172
{
144-
return crypto_submodule_acquire(&crypto_sha_avail);
173+
/* Don't check return code of Mutex::lock(void) */
174+
crypto_ecc_mutex->lock();
145175
}
146176

147-
void crypto_sha_release(void)
177+
void crypto_ecc_release(void)
148178
{
149-
crypto_submodule_release(&crypto_sha_avail);
179+
crypto_ecc_mutex->unlock();
150180
}
151181

152-
bool crypto_ecc_acquire(void)
182+
bool crypto_sha_try_acquire(void)
153183
{
154-
return crypto_submodule_acquire(&crypto_ecc_avail);
184+
return !core_util_atomic_flag_test_and_set(&crypto_sha_atomic_flag);
155185
}
156186

157-
void crypto_ecc_release(void)
187+
void crypto_sha_release(void)
158188
{
159-
crypto_submodule_release(&crypto_ecc_avail);
189+
core_util_atomic_flag_clear(&crypto_sha_atomic_flag);
160190
}
161191

162192
void crypto_prng_prestart(void)
@@ -240,18 +270,6 @@ bool crypto_dma_buffs_overlap(const void *in_buff, size_t in_buff_size, const vo
240270
return overlap;
241271
}
242272

243-
static bool crypto_submodule_acquire(uint16_t *submodule_avail)
244-
{
245-
uint16_t expectedCurrentValue = 1;
246-
return core_util_atomic_cas_u16(submodule_avail, &expectedCurrentValue, 0);
247-
}
248-
249-
static void crypto_submodule_release(uint16_t *submodule_avail)
250-
{
251-
uint16_t expectedCurrentValue = 0;
252-
while (! core_util_atomic_cas_u16(submodule_avail, &expectedCurrentValue, 1));
253-
}
254-
255273
static void crypto_submodule_prestart(volatile uint16_t *submodule_done)
256274
{
257275
*submodule_done = 0;
@@ -285,7 +303,7 @@ static bool crypto_submodule_wait(volatile uint16_t *submodule_done)
285303
}
286304

287305
/* Crypto interrupt handler */
288-
void CRYPTO_IRQHandler()
306+
extern "C" void CRYPTO_IRQHandler()
289307
{
290308
uint32_t intsts;
291309

targets/TARGET_NUVOTON/TARGET_M480/crypto/crypto-misc.h

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -32,26 +32,33 @@ void crypto_uninit(void);
3232
void crypto_zeroize(void *v, size_t n);
3333
void crypto_zeroize32(uint32_t *v, size_t n);
3434

35-
/* Acquire/release ownership of AES H/W */
36-
/* NOTE: If "acquire" succeeds, "release" must be done to pair it. */
37-
bool crypto_aes_acquire(void);
35+
/* Acquire/release ownership of crypto sub-module
36+
*
37+
* \note "acquire" is blocking until ownership is acquired
38+
*
39+
* \note "acquire"/"release" must be paired.
40+
*
41+
* \note Recursive "acquire" is allowed because the underlying synchronization
42+
* primitive mutex supports it.
43+
*/
44+
void crypto_aes_acquire(void);
3845
void crypto_aes_release(void);
39-
40-
/* Acquire/release ownership of DES H/W */
41-
/* NOTE: If "acquire" succeeds, "release" must be done to pair it. */
42-
bool crypto_des_acquire(void);
46+
void crypto_des_acquire(void);
4347
void crypto_des_release(void);
48+
void crypto_ecc_acquire(void);
49+
void crypto_ecc_release(void);
4450

45-
/* Acquire/release ownership of SHA H/W */
46-
/* NOTE: If "acquire" succeeds, "release" must be done to pair it. */
47-
bool crypto_sha_acquire(void);
51+
/* Acquire/release ownership of crypto sub-module
52+
*
53+
* \return false if crytpo sub-module is held by another thread or
54+
* another mbedtls context.
55+
* true if successful
56+
*
57+
* \note Successful "try_acquire" and "release" must be paired.
58+
*/
59+
bool crypto_sha_try_acquire(void);
4860
void crypto_sha_release(void);
4961

50-
/* Acquire/release ownership of ECC H/W */
51-
/* NOTE: If "acquire" succeeds, "release" must be done to pair it. */
52-
bool crypto_ecc_acquire(void);
53-
void crypto_ecc_release(void);
54-
5562
/* Flow control between crypto/xxx start and crypto/xxx ISR
5663
*
5764
* crypto_xxx_prestart/crypto_xxx_wait encapsulate control flow between crypto/xxx start and crypto/xxx ISR.

0 commit comments

Comments
 (0)