From bc12b7563fb2dee48753b75ec327c0fdc0ee61ea Mon Sep 17 00:00:00 2001 From: David Garske Date: Tue, 10 Feb 2026 14:51:51 -0800 Subject: [PATCH] Peer review improvements --- doc/dox_comments/header_files/ecc.h | 2 +- src/internal.c | 4 +- src/tls.c | 59 ++++++++++++++++++++++++----- wolfcrypt/src/asn.c | 10 ++--- wolfcrypt/src/curve25519.c | 54 +++++++++++++------------- wolfcrypt/src/ecc.c | 18 ++++++--- wolfcrypt/test/test.c | 8 ++-- wolfssl/wolfcrypt/curve25519.h | 2 +- 8 files changed, 101 insertions(+), 56 deletions(-) diff --git a/doc/dox_comments/header_files/ecc.h b/doc/dox_comments/header_files/ecc.h index 7f77bd244c..45d75f3449 100644 --- a/doc/dox_comments/header_files/ecc.h +++ b/doc/dox_comments/header_files/ecc.h @@ -2093,7 +2093,7 @@ int wc_ecc_decrypt(ecc_key* privKey, ecc_key* pubKey, const byte* msg, \return 0 Returned upon successfully setting the callback context the input message \param key pointer to the ecc_key object - \param ctx pointer to ecc_nb_ctx_t structure with stack data cache for SP + \param ctx pointer to ecc nb_ctx_t structure with stack data cache for SP _Example_ \code diff --git a/src/internal.c b/src/internal.c index ba509abbb7..5560a183f3 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8191,8 +8191,8 @@ void FreeKey(WOLFSSL* ssl, int type, void** pKey) case DYNAMIC_TYPE_CURVE25519: #if defined(WC_X25519_NONBLOCK) && \ defined(WOLFSSL_ASYNC_CRYPT_SW) - if (((curve25519_key*)*pKey)->nbCtx != NULL) { - XFREE(((curve25519_key*)*pKey)->nbCtx, ssl->heap, + if (((curve25519_key*)*pKey)->nb_ctx != NULL) { + XFREE(((curve25519_key*)*pKey)->nb_ctx, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); } #endif diff --git a/src/tls.c b/src/tls.c index 1300daedef..429a399891 100644 --- a/src/tls.c +++ b/src/tls.c @@ -8014,16 +8014,16 @@ static int TLSX_KeyShare_GenX25519Key(WOLFSSL *ssl, KeyShareEntry* kse) * INVALID_DEVID there is no async loop to retry on FP_WOULDBLOCK, so * skip non-blocking setup and use blocking mode instead. */ if (ret == 0 && ssl->devId != INVALID_DEVID) { - x25519_nb_ctx_t* nbCtx = (x25519_nb_ctx_t*)XMALLOC( + x25519_nb_ctx_t* nb_ctx = (x25519_nb_ctx_t*)XMALLOC( sizeof(x25519_nb_ctx_t), ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); - if (nbCtx == NULL) { + if (nb_ctx == NULL) { ret = MEMORY_E; } else { - ret = wc_curve25519_set_nonblock(key, nbCtx); + ret = wc_curve25519_set_nonblock(key, nb_ctx); if (ret != 0) { - XFREE(nbCtx, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(nb_ctx, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); } } } @@ -8088,8 +8088,8 @@ static int TLSX_KeyShare_GenX25519Key(WOLFSSL *ssl, KeyShareEntry* kse) kse->pubKey = NULL; if (key != NULL) { #if defined(WC_X25519_NONBLOCK) && defined(WOLFSSL_ASYNC_CRYPT_SW) - if (key->nbCtx != NULL) { - XFREE(key->nbCtx, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); + if (key->nb_ctx != NULL) { + XFREE(key->nb_ctx, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); } #endif wc_curve25519_free(key); @@ -8279,6 +8279,28 @@ static int TLSX_KeyShare_GenEccKey(WOLFSSL *ssl, KeyShareEntry* kse) /* Initialize an ECC key struct for the ephemeral key */ ret = wc_ecc_init_ex((ecc_key*)kse->key, ssl->heap, ssl->devId); + #if defined(WC_ECC_NONBLOCK) && defined(WOLFSSL_ASYNC_CRYPT_SW) && \ + defined(WC_ASYNC_ENABLE_ECC) + /* Only set non-blocking context when async device is active. With + * INVALID_DEVID there is no async loop to retry on FP_WOULDBLOCK, so + * skip non-blocking setup and use blocking mode instead. */ + if (ret == 0 && ssl->devId != INVALID_DEVID) { + ecc_nb_ctx_t* eccNbCtx = (ecc_nb_ctx_t*)XMALLOC( + sizeof(ecc_nb_ctx_t), ssl->heap, + DYNAMIC_TYPE_TMP_BUFFER); + if (eccNbCtx == NULL) { + ret = MEMORY_E; + } + else { + ret = wc_ecc_set_nonblock((ecc_key*)kse->key, eccNbCtx); + if (ret != 0) { + XFREE(eccNbCtx, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); + } + } + } + #endif /* WC_ECC_NONBLOCK && WOLFSSL_ASYNC_CRYPT_SW && + WC_ASYNC_ENABLE_ECC */ + if (ret == 0) { kse->keyLen = keySize; kse->pubKeyLen = keySize * 2 + 1; @@ -8852,8 +8874,8 @@ static void TLSX_KeyShare_FreeAll(KeyShareEntry* list, void* heap) #ifdef HAVE_CURVE25519 #if defined(WC_X25519_NONBLOCK) && defined(WOLFSSL_ASYNC_CRYPT_SW) if (current->key != NULL && - ((curve25519_key*)current->key)->nbCtx != NULL) { - XFREE(((curve25519_key*)current->key)->nbCtx, heap, + ((curve25519_key*)current->key)->nb_ctx != NULL) { + XFREE(((curve25519_key*)current->key)->nb_ctx, heap, DYNAMIC_TYPE_TMP_BUFFER); } #endif @@ -8900,6 +8922,15 @@ static void TLSX_KeyShare_FreeAll(KeyShareEntry* list, void* heap) } else { #ifdef HAVE_ECC + #if defined(WC_ECC_NONBLOCK) && \ + defined(WOLFSSL_ASYNC_CRYPT_SW) && \ + defined(WC_ASYNC_ENABLE_ECC) + if (current->key != NULL && + ((ecc_key*)current->key)->nb_ctx != NULL) { + XFREE(((ecc_key*)current->key)->nb_ctx, heap, + DYNAMIC_TYPE_TMP_BUFFER); + } + #endif wc_ecc_free((ecc_key*)current->key); #endif } @@ -8907,6 +8938,14 @@ static void TLSX_KeyShare_FreeAll(KeyShareEntry* list, void* heap) #endif else { #ifdef HAVE_ECC + #if defined(WC_ECC_NONBLOCK) && defined(WOLFSSL_ASYNC_CRYPT_SW) && \ + defined(WC_ASYNC_ENABLE_ECC) + if (current->key != NULL && + ((ecc_key*)current->key)->nb_ctx != NULL) { + XFREE(((ecc_key*)current->key)->nb_ctx, heap, + DYNAMIC_TYPE_TMP_BUFFER); + } + #endif wc_ecc_free((ecc_key*)current->key); #endif } @@ -9248,8 +9287,8 @@ static int TLSX_KeyShare_ProcessX25519_ex(WOLFSSL* ssl, } if (keyShareEntry->key != NULL) { #if defined(WC_X25519_NONBLOCK) && defined(WOLFSSL_ASYNC_CRYPT_SW) - if (((curve25519_key*)keyShareEntry->key)->nbCtx != NULL) { - XFREE(((curve25519_key*)keyShareEntry->key)->nbCtx, ssl->heap, + if (((curve25519_key*)keyShareEntry->key)->nb_ctx != NULL) { + XFREE(((curve25519_key*)keyShareEntry->key)->nb_ctx, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); } #endif diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 27d3a67073..6d744eff5e 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -18456,7 +18456,7 @@ int ConfirmSignature(SignatureCtx* sigCtx, word32 idx = 0; #if defined(WC_ECC_NONBLOCK) && defined(WOLFSSL_ASYNC_CRYPT_SW) && \ defined(WC_ASYNC_ENABLE_ECC) - ecc_nb_ctx_t* nbCtx; + ecc_nb_ctx_t* nb_ctx; #endif /* WC_ECC_NONBLOCK && WOLFSSL_ASYNC_CRYPT_SW && WC_ASYNC_ENABLE_ECC */ @@ -18479,15 +18479,15 @@ int ConfirmSignature(SignatureCtx* sigCtx, * retry on FP_WOULDBLOCK, so let the WC_ECC_NONBLOCK_ONLY * blocking fallback handle it instead. */ if (sigCtx->devId != INVALID_DEVID) { - nbCtx = (ecc_nb_ctx_t*)XMALLOC(sizeof(ecc_nb_ctx_t), + nb_ctx = (ecc_nb_ctx_t*)XMALLOC(sizeof(ecc_nb_ctx_t), sigCtx->heap, DYNAMIC_TYPE_TMP_BUFFER); - if (nbCtx == NULL) { + if (nb_ctx == NULL) { ERROR_OUT(MEMORY_E, exit_cs); } - ret = wc_ecc_set_nonblock(sigCtx->key.ecc, nbCtx); + ret = wc_ecc_set_nonblock(sigCtx->key.ecc, nb_ctx); if (ret != 0) { - XFREE(nbCtx, sigCtx->heap, + XFREE(nb_ctx, sigCtx->heap, DYNAMIC_TYPE_TMP_BUFFER); goto exit_cs; } diff --git a/wolfcrypt/src/curve25519.c b/wolfcrypt/src/curve25519.c index af74763a86..aed4eef0b0 100644 --- a/wolfcrypt/src/curve25519.c +++ b/wolfcrypt/src/curve25519.c @@ -459,13 +459,13 @@ static int wc_curve25519_make_pub_nb(curve25519_key* key) if (key == NULL) { ret = BAD_FUNC_ARG; } - else if (key->nbCtx == NULL) { + else if (key->nb_ctx == NULL) { WOLFSSL_MSG("wc_curve25519_make_pub_nb called with NULL non-blocking " "context."); ret = BAD_FUNC_ARG; } - if (ret == 0 && key->nbCtx->state == 0) { + if (ret == 0 && key->nb_ctx->state == 0) { /* check clamping */ ret = curve25519_priv_clamp_check(key->k); if (ret == 0) { @@ -474,7 +474,7 @@ static int wc_curve25519_make_pub_nb(curve25519_key* key) } if (ret == 0) { ret = curve25519_nb(key->p.point, key->k, (byte*)kCurve25519BasePoint, - key->nbCtx); + key->nb_ctx); } return ret; @@ -488,13 +488,13 @@ static int wc_curve25519_make_key_nb(WC_RNG* rng, int keysize, if (key == NULL || rng == NULL) { ret = BAD_FUNC_ARG; } - else if (key->nbCtx == NULL) { + else if (key->nb_ctx == NULL) { WOLFSSL_MSG("wc_curve25519_make_key_nb called with NULL non-blocking " "context."); ret = BAD_FUNC_ARG; } - if (ret == 0 && key->nbCtx->state == 0) { + if (ret == 0 && key->nb_ctx->state == 0) { ret = wc_curve25519_make_priv(rng, keysize, key->k); if (ret == 0) { key->privSet = 1; @@ -512,19 +512,19 @@ static int wc_curve25519_make_key_nb(WC_RNG* rng, int keysize, int wc_curve25519_set_nonblock(curve25519_key* key, x25519_nb_ctx_t* ctx) { - int ret = 0; - - if (key != NULL) { - if (ctx != NULL) { - XMEMSET(ctx, 0, sizeof(x25519_nb_ctx_t)); - } - key->nbCtx = ctx; + if (key == NULL) { + return BAD_FUNC_ARG; } - else { - ret = BAD_FUNC_ARG; + /* If a different context is already set, clear it before replacing. + * The caller is responsible for freeing any heap-allocated context. */ + if (key->nb_ctx != NULL && key->nb_ctx != ctx) { + XMEMSET(key->nb_ctx, 0, sizeof(x25519_nb_ctx_t)); } - - return ret; + if (ctx != NULL) { + XMEMSET(ctx, 0, sizeof(x25519_nb_ctx_t)); + } + key->nb_ctx = ctx; + return 0; } #endif /* WC_X25519_NONBLOCK */ @@ -567,7 +567,7 @@ int wc_curve25519_make_key(WC_RNG* rng, int keysize, curve25519_key* key) #ifdef WOLFSSL_SE050 ret = se050_curve25519_create_key(key, keysize); #elif defined(WC_X25519_NONBLOCK) - if (key->nbCtx != NULL) { + if (key->nb_ctx != NULL) { ret = wc_curve25519_make_key_nb(rng, keysize, key); } else @@ -612,17 +612,17 @@ static int wc_curve25519_shared_secret_nb(curve25519_key* privKey, { int ret = FP_WOULDBLOCK; - switch (privKey->nbCtx->ssState) { + switch (privKey->nb_ctx->ssState) { case 0: - XMEMSET(&privKey->nbCtx->o, 0, sizeof(privKey->nbCtx->o)); - privKey->nbCtx->ssState = 1; + XMEMSET(&privKey->nb_ctx->o, 0, sizeof(privKey->nb_ctx->o)); + privKey->nb_ctx->ssState = 1; break; case 1: - ret = curve25519_nb(privKey->nbCtx->o.point, privKey->k, - pubKey->p.point, privKey->nbCtx); + ret = curve25519_nb(privKey->nb_ctx->o.point, privKey->k, + pubKey->p.point, privKey->nb_ctx); if (ret == 0) { ret = FP_WOULDBLOCK; - privKey->nbCtx->ssState = 2; + privKey->nb_ctx->ssState = 2; } break; case 2: @@ -632,7 +632,7 @@ static int wc_curve25519_shared_secret_nb(curve25519_key* privKey, byte t = 0; for (i = 0; i < CURVE25519_KEYSIZE; i++) { - t |= privKey->nbCtx->o.point[i]; + t |= privKey->nb_ctx->o.point[i]; } if (t == 0) { ret = ECC_OUT_OF_RANGE_E; @@ -640,7 +640,7 @@ static int wc_curve25519_shared_secret_nb(curve25519_key* privKey, else #endif /* WOLFSSL_ECDHX_SHARED_NOT_ZERO */ { - curve25519_copy_point(out, privKey->nbCtx->o.point, endian); + curve25519_copy_point(out, privKey->nb_ctx->o.point, endian); *outlen = CURVE25519_KEYSIZE; ret = 0; } @@ -651,7 +651,7 @@ static int wc_curve25519_shared_secret_nb(curve25519_key* privKey, } if (ret != FP_WOULDBLOCK) { - XMEMSET(privKey->nbCtx, 0, sizeof(x25519_nb_ctx_t)); + XMEMSET(privKey->nb_ctx, 0, sizeof(x25519_nb_ctx_t)); } return ret; @@ -714,7 +714,7 @@ int wc_curve25519_shared_secret_ex(curve25519_key* private_key, #endif /* WOLFSSL_ASYNC_CRYPT && WC_ASYNC_ENABLE_X25519 && * WOLFSSL_ASYNC_CRYPT_SW */ - if (private_key->nbCtx != NULL) { + if (private_key->nb_ctx != NULL) { ret = wc_curve25519_shared_secret_nb(private_key, public_key, out, outlen, endian); } diff --git a/wolfcrypt/src/ecc.c b/wolfcrypt/src/ecc.c index da309eb2ab..59a14fd136 100644 --- a/wolfcrypt/src/ecc.c +++ b/wolfcrypt/src/ecc.c @@ -55,7 +55,7 @@ Possible ECC enable options: * WOLFSSL_ECC_CURVE_STATIC: default off (on for windows) * For the ECC curve parameters `ecc_set_type` use fixed * array for hex string - * WC_ECC_NONBLOCK: Enable non-blocking support for sign/verify. + * WC_ECC_NONBLOCK: Enable non-blocking support for sign/verify/keygen/secret. * Requires SP with WOLFSSL_SP_NONBLOCK * WC_ECC_NONBLOCK_ONLY Enable the non-blocking function only, no fall-back to * normal blocking API's @@ -15627,12 +15627,18 @@ int wc_ecc_get_key_id(ecc_key* key, word32* keyId) /* Enable ECC support for non-blocking operations */ int wc_ecc_set_nonblock(ecc_key *key, ecc_nb_ctx_t* ctx) { - if (key) { - if (ctx) { - XMEMSET(ctx, 0, sizeof(ecc_nb_ctx_t)); - } - key->nb_ctx = ctx; + if (key == NULL) { + return BAD_FUNC_ARG; } + /* If a different context is already set, clear it before replacing. + * The caller is responsible for freeing any heap-allocated context. */ + if (key->nb_ctx != NULL && key->nb_ctx != ctx) { + XMEMSET(key->nb_ctx, 0, sizeof(ecc_nb_ctx_t)); + } + if (ctx != NULL) { + XMEMSET(ctx, 0, sizeof(ecc_nb_ctx_t)); + } + key->nb_ctx = ctx; return 0; } #endif /* WC_ECC_NONBLOCK */ diff --git a/wolfcrypt/test/test.c b/wolfcrypt/test/test.c index fce1446aea..699d9fb55c 100644 --- a/wolfcrypt/test/test.c +++ b/wolfcrypt/test/test.c @@ -38594,7 +38594,7 @@ static wc_test_ret_t curve255519_der_test(void) static int x25519_nonblock_test(WC_RNG* rng) { int ret = 0; - x25519_nb_ctx_t nbCtx; + x25519_nb_ctx_t nb_ctx; curve25519_key userA; curve25519_key userB; #ifdef HAVE_CURVE25519_SHARED_SECRET @@ -38605,14 +38605,14 @@ static int x25519_nonblock_test(WC_RNG* rng) #endif int count; - XMEMSET(&nbCtx, 0, sizeof(nbCtx)); + XMEMSET(&nb_ctx, 0, sizeof(nb_ctx)); ret = wc_curve25519_init(&userA); if (ret != 0) { printf("wc_curve25519_init 1 %d\n", ret); return -10722; } - ret = wc_curve25519_set_nonblock(&userA, &nbCtx); + ret = wc_curve25519_set_nonblock(&userA, &nb_ctx); if (ret != 0) { printf("wc_curve25519_set_nonblock 1 %d\n", ret); wc_curve25519_free(&userA); @@ -38639,7 +38639,7 @@ static int x25519_nonblock_test(WC_RNG* rng) wc_curve25519_free(&userA); return -10724; } - ret = wc_curve25519_set_nonblock(&userB, &nbCtx); + ret = wc_curve25519_set_nonblock(&userB, &nb_ctx); if (ret != 0) { printf("wc_curve25519_set_nonblock 2 %d\n", ret); wc_curve25519_free(&userA); diff --git a/wolfssl/wolfcrypt/curve25519.h b/wolfssl/wolfcrypt/curve25519.h index f73b9c31b2..2e11503595 100644 --- a/wolfssl/wolfcrypt/curve25519.h +++ b/wolfssl/wolfcrypt/curve25519.h @@ -141,7 +141,7 @@ struct curve25519_key { #endif #ifdef WC_X25519_NONBLOCK - x25519_nb_ctx_t* nbCtx; + x25519_nb_ctx_t* nb_ctx; #endif /* WC_X25519_NONBLOCK */ /* bit fields */