From d8e3ba575e88e115332d00a5d00b606d0174538b Mon Sep 17 00:00:00 2001 From: Paul Adelsbach Date: Tue, 18 Aug 2026 19:21:22 -0700 Subject: [PATCH 1/4] Add CryptoCb_Free for Falcon/FN-DSA --- wolfcrypt/src/falcon.c | 9 +++++++++ wolfcrypt/test/test.c | 11 ++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/wolfcrypt/src/falcon.c b/wolfcrypt/src/falcon.c index 45db2ed169..4d7fbb5921 100644 --- a/wolfcrypt/src/falcon.c +++ b/wolfcrypt/src/falcon.c @@ -9046,6 +9046,15 @@ int wc_falcon_get_level(falcon_key* key, byte* level) void wc_falcon_free(falcon_key* key) { if (key != NULL) { +#if defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) + if (key->devId != INVALID_DEVID) { + (void)wc_CryptoCb_Free(key->devId, WC_ALGO_TYPE_PK, + WC_PK_TYPE_PQC_SIG_KEYGEN, + WC_PQC_SIG_TYPE_FALCON, + (void*)key); + /* always continue to software cleanup */ + } +#endif ForceZero(key, sizeof(*key)); } } diff --git a/wolfcrypt/test/test.c b/wolfcrypt/test/test.c index 0556fd5351..86ca57a8c3 100644 --- a/wolfcrypt/test/test.c +++ b/wolfcrypt/test/test.c @@ -82082,7 +82082,8 @@ static int myCryptoDevCb(int devIdArg, wc_CryptoInfo* info, void* ctx) break; } #endif -#if defined(WOLFSSL_HAVE_MLDSA) || defined(WOLFSSL_HAVE_SLHDSA) +#if defined(WOLFSSL_HAVE_MLDSA) || defined(WOLFSSL_HAVE_SLHDSA) || \ + defined(HAVE_FALCON) case WC_PK_TYPE_PQC_SIG_KEYGEN: { #ifdef WOLFSSL_HAVE_MLDSA @@ -82100,6 +82101,14 @@ static int myCryptoDevCb(int devIdArg, wc_CryptoInfo* info, void* ctx) wc_SlhDsaKey_Free(slh); ret = 0; } + #endif + #ifdef HAVE_FALCON + if (info->free.subType == WC_PQC_SIG_TYPE_FALCON) { + falcon_key* fk = (falcon_key*)info->free.obj; + fk->devId = INVALID_DEVID; + wc_falcon_free(fk); + ret = 0; + } #endif break; } From d089b17a0a04bf573305ef88355b24bd4623f290 Mon Sep 17 00:00:00 2001 From: Paul Adelsbach Date: Tue, 18 Aug 2026 20:02:17 -0700 Subject: [PATCH 2/4] Add missing CryptoCb_Free test cases and CI --- .github/configs/pq-all.json | 9 +++++ tests/api/test_mldsa.c | 59 +++++++++++++++++++++++++++++++ tests/api/test_mldsa.h | 4 ++- tests/api/test_mlkem.c | 70 +++++++++++++++++++++++++++++++++++++ tests/api/test_mlkem.h | 4 ++- tests/api/test_slhdsa.c | 60 +++++++++++++++++++++++++++++++ tests/api/test_slhdsa.h | 4 ++- 7 files changed, 207 insertions(+), 3 deletions(-) diff --git a/.github/configs/pq-all.json b/.github/configs/pq-all.json index 0a147cdbd1..748f19d24c 100644 --- a/.github/configs/pq-all.json +++ b/.github/configs/pq-all.json @@ -71,6 +71,15 @@ "--enable-dilithium=yes,no-ctx", "--enable-dual-alg-certs", "--disable-qt", "CPPFLAGS=-pedantic -Wdeclaration-after-statement -Wnull-dereference -DWOLFCRYPT_TEST_LINT -DNO_WOLFSSL_CIPHER_SUITE_TEST -DTEST_LIBWOLFSSL_SOURCES_INCLUSION_SEQUENCE"]}, +{"name": "pq-cryptocbutils", "minutes": 3, + "comment": "The only config pairing --enable-cryptocbutils with the PQC algorithms. --enable-all already turns cryptocb on, but the utility callbacks (copy, free, setkey, export) default to off, so without this entry WOLF_CRYPTO_CB_FREE is never compiled alongside a PQC algorithm and the free-callback tests for ML-DSA, SLH-DSA and ML-KEM all skip. Falcon and FrodoKEM are named explicitly: experimental only auto-enables them under --enable-all-quantum-crypto, which nothing here sets.", + "configure": ["--enable-intelasm", "--enable-sp-asm", + "--enable-all", "--enable-testcert", "--enable-experimental", + "--enable-mlkem=yes,kyber,ml-kem", "--enable-slhdsa", + "--enable-dilithium", "--enable-falcon", "--enable-frodokem", + "--enable-cryptocb", "--enable-cryptocbutils", + "--disable-qt", + "CPPFLAGS=-pedantic -Wdeclaration-after-statement -Wnull-dereference -DWOLFCRYPT_TEST_LINT -DNO_WOLFSSL_CIPHER_SUITE_TEST -DTEST_LIBWOLFSSL_SOURCES_INCLUSION_SEQUENCE"]}, {"name": "pq-asynccrypt-dual-alg", "minutes": 2.5, "comment": "Async crypto with dual-algorithm certs and PQC signatures. WOLFSSL_ASYNC_CRYPT turns off the streaming CertificateVerify path, so this is the only config that compiles the in-place fragmented send and the async args holder that Scv13Args must fit.", "configure": ["--enable-asynccrypt-sw", "--enable-dual-alg-certs", diff --git a/tests/api/test_mldsa.c b/tests/api/test_mldsa.c index c7abe5f6dc..3bcf57b604 100644 --- a/tests/api/test_mldsa.c +++ b/tests/api/test_mldsa.c @@ -43,6 +43,7 @@ #endif #include +#include #ifdef WOLFSSL_HAVE_MLDSA #include #endif @@ -31401,3 +31402,61 @@ int test_wc_MldsaDerDecisionCoverage(void) #endif /* WOLFSSL_HAVE_MLDSA && WOLFSSL_MLDSA_NO_ASN1 && ... */ return EXPECT_RESULT(); } + +#if defined(WOLFSSL_HAVE_MLDSA) && defined(WOLF_CRYPTO_CB) && \ + defined(WOLF_CRYPTO_CB_FREE) + #define TEST_MLDSA_CB_FREE + #define TEST_MLDSA_CB_FREE_DEVID 0x4D4C4453 +#endif + +#ifdef TEST_MLDSA_CB_FREE +/* Stands in for a device holding state for the key. Counting the call proves + * wc_MlDsaKey_Free told the device rather than only cleaning up in software, + * which would leave the device side of the key behind. */ +static int mldsa_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) +{ + int* frees = (int*)ctx; + + (void)devIdArg; + + if ((info != NULL) && (info->algo_type == WC_ALGO_TYPE_FREE) && + (info->free.algo == WC_ALGO_TYPE_PK) && + (info->free.type == WC_PK_TYPE_PQC_SIG_KEYGEN) && + (info->free.subType == WC_PQC_SIG_TYPE_MLDSA)) { + if (frees != NULL) { + (*frees)++; + } + return 0; + } + + return WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE); +} +#endif /* TEST_MLDSA_CB_FREE */ + +/* Freeing a key that names a device has to tell that device, so it can + * release what it holds. A key with no device must not. */ +int test_mldsa_cb_free(void) +{ + EXPECT_DECLS; +#ifdef TEST_MLDSA_CB_FREE + wc_MlDsaKey* key = NULL; + int frees = 0; + + ExpectIntEQ(wc_CryptoCb_RegisterDevice(TEST_MLDSA_CB_FREE_DEVID, + mldsa_cb_free_cb, &frees), 0); + + ExpectNotNull(key = (wc_MlDsaKey*)XMALLOC(sizeof(wc_MlDsaKey), NULL, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectIntEQ(wc_MlDsaKey_Init(key, NULL, TEST_MLDSA_CB_FREE_DEVID), 0); + wc_MlDsaKey_Free(key); + ExpectIntEQ(frees, 1); + + ExpectIntEQ(wc_MlDsaKey_Init(key, NULL, INVALID_DEVID), 0); + wc_MlDsaKey_Free(key); + ExpectIntEQ(frees, 1); + + XFREE(key, NULL, DYNAMIC_TYPE_TMP_BUFFER); + wc_CryptoCb_UnRegisterDevice(TEST_MLDSA_CB_FREE_DEVID); +#endif + return EXPECT_RESULT(); +} diff --git a/tests/api/test_mldsa.h b/tests/api/test_mldsa.h index 4a81faba1c..df6c6bb1f0 100644 --- a/tests/api/test_mldsa.h +++ b/tests/api/test_mldsa.h @@ -68,6 +68,7 @@ int test_wc_MldsaFeatureCoverage(void); int test_mldsa_legacy_shim(void); int test_wc_MldsaDecisionCoverage2(void); int test_wc_MldsaDerDecisionCoverage(void); +int test_mldsa_cb_free(void); #define TEST_MLDSA_DECLS \ TEST_DECL_GROUP("mldsa", test_mldsa), \ @@ -99,6 +100,7 @@ int test_wc_MldsaDerDecisionCoverage(void); TEST_DECL_GROUP("mldsa", test_wc_MldsaFeatureCoverage), \ TEST_DECL_GROUP("mldsa", test_mldsa_legacy_shim), \ TEST_DECL_GROUP("mldsa", test_wc_MldsaDecisionCoverage2), \ - TEST_DECL_GROUP("mldsa", test_wc_MldsaDerDecisionCoverage) + TEST_DECL_GROUP("mldsa", test_wc_MldsaDerDecisionCoverage), \ + TEST_DECL_GROUP("mldsa", test_mldsa_cb_free) #endif /* WOLFCRYPT_TEST_MLDSA_H */ diff --git a/tests/api/test_mlkem.c b/tests/api/test_mlkem.c index 759f25f644..e0623365cf 100644 --- a/tests/api/test_mlkem.c +++ b/tests/api/test_mlkem.c @@ -32,6 +32,7 @@ #include #endif #include +#include #include #include @@ -4648,3 +4649,72 @@ int test_wc_mlkem_encode_key_len_decision(void) #endif return EXPECT_RESULT(); } /* END test_wc_mlkem_encode_key_len_decision */ + +#if defined(WOLFSSL_HAVE_MLKEM) && defined(WOLF_CRYPTO_CB) && \ + defined(WOLF_CRYPTO_CB_FREE) + #define TEST_MLKEM_CB_FREE + #define TEST_MLKEM_CB_FREE_DEVID 0x4D4C4B4D + #ifndef WOLFSSL_NO_ML_KEM_512 + #define TEST_MLKEM_CB_FREE_TYPE WC_ML_KEM_512 + #elif !defined(WOLFSSL_NO_ML_KEM_768) + #define TEST_MLKEM_CB_FREE_TYPE WC_ML_KEM_768 + #elif !defined(WOLFSSL_NO_ML_KEM_1024) + #define TEST_MLKEM_CB_FREE_TYPE WC_ML_KEM_1024 + #else + #undef TEST_MLKEM_CB_FREE + #endif +#endif + +#ifdef TEST_MLKEM_CB_FREE +/* Stands in for a device holding state for the key. Counting the call proves + * wc_MlKemKey_Free told the device rather than only cleaning up in software, + * which would leave the device side of the key behind. */ +static int mlkem_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) +{ + int* frees = (int*)ctx; + + (void)devIdArg; + + if ((info != NULL) && (info->algo_type == WC_ALGO_TYPE_FREE) && + (info->free.algo == WC_ALGO_TYPE_PK) && + (info->free.type == WC_PK_TYPE_PQC_KEM_KEYGEN) && + (info->free.subType == WC_PQC_KEM_TYPE_MLKEM)) { + if (frees != NULL) { + (*frees)++; + } + return 0; + } + + return WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE); +} +#endif /* TEST_MLKEM_CB_FREE */ + +/* Freeing a key that names a device has to tell that device, so it can + * release what it holds. A key with no device must not. */ +int test_wc_mlkem_cb_free(void) +{ + EXPECT_DECLS; +#ifdef TEST_MLKEM_CB_FREE + MlKemKey* key = NULL; + int frees = 0; + + ExpectIntEQ(wc_CryptoCb_RegisterDevice(TEST_MLKEM_CB_FREE_DEVID, + mlkem_cb_free_cb, &frees), 0); + + ExpectNotNull(key = (MlKemKey*)XMALLOC(sizeof(MlKemKey), NULL, + DYNAMIC_TYPE_TMP_BUFFER)); + ExpectIntEQ(wc_MlKemKey_Init(key, TEST_MLKEM_CB_FREE_TYPE, NULL, + TEST_MLKEM_CB_FREE_DEVID), 0); + ExpectIntEQ(wc_MlKemKey_Free(key), 0); + ExpectIntEQ(frees, 1); + + ExpectIntEQ(wc_MlKemKey_Init(key, TEST_MLKEM_CB_FREE_TYPE, NULL, + INVALID_DEVID), 0); + ExpectIntEQ(wc_MlKemKey_Free(key), 0); + ExpectIntEQ(frees, 1); + + XFREE(key, NULL, DYNAMIC_TYPE_TMP_BUFFER); + wc_CryptoCb_UnRegisterDevice(TEST_MLKEM_CB_FREE_DEVID); +#endif + return EXPECT_RESULT(); +} diff --git a/tests/api/test_mlkem.h b/tests/api/test_mlkem.h index 7f11e37f65..e8028c5295 100644 --- a/tests/api/test_mlkem.h +++ b/tests/api/test_mlkem.h @@ -36,6 +36,7 @@ int test_wc_mlkem_init_id_decision(void); int test_wc_mlkem_init_label_decision(void); int test_wc_mlkem_encapsulate_pubkey_unset_decision(void); int test_wc_mlkem_encode_key_len_decision(void); +int test_wc_mlkem_cb_free(void); #define TEST_MLKEM_DECLS \ TEST_DECL_GROUP("mlkem", test_wc_mlkem_make_key_kats), \ @@ -49,6 +50,7 @@ int test_wc_mlkem_encode_key_len_decision(void); TEST_DECL_GROUP("mlkem", test_wc_mlkem_init_id_decision), \ TEST_DECL_GROUP("mlkem", test_wc_mlkem_init_label_decision), \ TEST_DECL_GROUP("mlkem", test_wc_mlkem_encapsulate_pubkey_unset_decision), \ - TEST_DECL_GROUP("mlkem", test_wc_mlkem_encode_key_len_decision) + TEST_DECL_GROUP("mlkem", test_wc_mlkem_encode_key_len_decision), \ + TEST_DECL_GROUP("mlkem", test_wc_mlkem_cb_free) #endif /* WOLFCRYPT_TEST_MLKEM_H */ diff --git a/tests/api/test_slhdsa.c b/tests/api/test_slhdsa.c index bd8c578971..d5b43970d9 100644 --- a/tests/api/test_slhdsa.c +++ b/tests/api/test_slhdsa.c @@ -3871,3 +3871,63 @@ int test_slhdsa_get_sigalg_info(void) #endif /* WOLFSSL_HAVE_SLHDSA && OPENSSL_EXTRA */ return EXPECT_RESULT(); } + +#if defined(WOLFSSL_HAVE_SLHDSA) && defined(WOLF_CRYPTO_CB) && \ + defined(WOLF_CRYPTO_CB_FREE) + #define TEST_SLHDSA_CB_FREE + #define TEST_SLHDSA_CB_FREE_DEVID 0x534C4844 +#endif + +#ifdef TEST_SLHDSA_CB_FREE +/* Stands in for a device holding state for the key. Counting the call proves + * wc_SlhDsaKey_Free told the device rather than only cleaning up in software, + * which would leave the device side of the key behind. */ +static int slhdsa_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) +{ + int* frees = (int*)ctx; + + (void)devIdArg; + + if ((info != NULL) && (info->algo_type == WC_ALGO_TYPE_FREE) && + (info->free.algo == WC_ALGO_TYPE_PK) && + (info->free.type == WC_PK_TYPE_PQC_SIG_KEYGEN) && + (info->free.subType == WC_PQC_SIG_TYPE_SLHDSA)) { + if (frees != NULL) { + (*frees)++; + } + return 0; + } + + return WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE); +} +#endif /* TEST_SLHDSA_CB_FREE */ + +/* Freeing a key that names a device has to tell that device, so it can + * release what it holds. A key with no device must not. */ +int test_slhdsa_cb_free(void) +{ + EXPECT_DECLS; +#ifdef TEST_SLHDSA_CB_FREE + SlhDsaKey key; + int frees = 0; + + XMEMSET(&key, 0, sizeof(key)); + + ExpectIntEQ(wc_CryptoCb_RegisterDevice(TEST_SLHDSA_CB_FREE_DEVID, + slhdsa_cb_free_cb, &frees), 0); + + ExpectIntEQ(wc_SlhDsaKey_Init(&key, WC_SLHDSA_DEFAULT_PARAM, NULL, + TEST_SLHDSA_CB_FREE_DEVID), 0); + wc_SlhDsaKey_Free(&key); + ExpectIntEQ(frees, 1); + + XMEMSET(&key, 0, sizeof(key)); + ExpectIntEQ(wc_SlhDsaKey_Init(&key, WC_SLHDSA_DEFAULT_PARAM, NULL, + INVALID_DEVID), 0); + wc_SlhDsaKey_Free(&key); + ExpectIntEQ(frees, 1); + + wc_CryptoCb_UnRegisterDevice(TEST_SLHDSA_CB_FREE_DEVID); +#endif + return EXPECT_RESULT(); +} diff --git a/tests/api/test_slhdsa.h b/tests/api/test_slhdsa.h index 33f03e1945..88cc04d9eb 100644 --- a/tests/api/test_slhdsa.h +++ b/tests/api/test_slhdsa.h @@ -49,6 +49,7 @@ int test_slhdsa_tls13_certverify_multi_stall(void); int test_mldsa_tls13_certverify_maxfrag_stream(void); int test_slhdsa_dev_private_key(void); int test_slhdsa_tls13_certverify_bad_signature(void); +int test_slhdsa_cb_free(void); #define TEST_SLHDSA_DECLS \ TEST_DECL_GROUP("slhdsa", test_wc_slhdsa), \ @@ -75,6 +76,7 @@ int test_slhdsa_tls13_certverify_bad_signature(void); TEST_DECL_GROUP("slhdsa", test_slhdsa_tls13_certverify_multi_stall), \ TEST_DECL_GROUP("slhdsa", test_mldsa_tls13_certverify_maxfrag_stream), \ TEST_DECL_GROUP("slhdsa", test_slhdsa_dev_private_key), \ - TEST_DECL_GROUP("slhdsa", test_slhdsa_tls13_certverify_bad_signature) + TEST_DECL_GROUP("slhdsa", test_slhdsa_tls13_certverify_bad_signature), \ + TEST_DECL_GROUP("slhdsa", test_slhdsa_cb_free) #endif /* WOLFCRYPT_TEST_SLHDSA_H */ From 07edb52139a9249d4ef9a251123751a8259d2626 Mon Sep 17 00:00:00 2001 From: Paul Adelsbach Date: Wed, 19 Aug 2026 17:05:23 -0700 Subject: [PATCH 3/4] PR feedback: add/update tests, ensure devId is invalidated --- tests/api/test_falcon.c | 100 +++++++++++++++++++++++++++++++++++++++ tests/api/test_falcon.h | 4 +- tests/api/test_mldsa.c | 63 ++++++++++++++++++++---- tests/api/test_mlkem.c | 82 ++++++++++++++++++++++++++------ tests/api/test_slhdsa.c | 63 ++++++++++++++++++++---- wolfcrypt/src/falcon.c | 5 ++ wolfcrypt/src/wc_mldsa.c | 5 ++ wolfcrypt/src/wc_mlkem.c | 10 ++++ 8 files changed, 297 insertions(+), 35 deletions(-) diff --git a/tests/api/test_falcon.c b/tests/api/test_falcon.c index bf264fb777..ddb24962cd 100644 --- a/tests/api/test_falcon.c +++ b/tests/api/test_falcon.c @@ -36,6 +36,7 @@ #include #include #include +#include #include #include @@ -1035,3 +1036,102 @@ int test_wc_FalconDecisionCoverage(void) #endif /* HAVE_FALCON */ return EXPECT_RESULT(); } + +#if defined(HAVE_FALCON) && defined(WOLF_CRYPTO_CB) && \ + defined(WOLF_CRYPTO_CB_FREE) + #define TEST_FALCON_CB_FREE + #define TEST_FALCON_CB_FREE_DEVID 0x46414C43 +#endif + +#ifdef TEST_FALCON_CB_FREE +/* What the free callback saw, so the test can check the contract rather than + * just that something fired. */ +typedef struct { + int frees; /* matching free callbacks seen */ + int badObj; /* callback was handed the wrong object */ + int wiped; /* callback saw a key already cleaned up */ + int ret; /* what the callback returns */ + const void* obj; /* object the free is expected to name */ +} FalconCbFreeCtx; + +/* Stands in for a device holding state for the key. Counting the call proves + * wc_falcon_free told the device rather than only cleaning up in software, + * which would leave the device side of the key behind. */ +static int falcon_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) +{ + FalconCbFreeCtx* seen = (FalconCbFreeCtx*)ctx; + + (void)devIdArg; + + if ((seen != NULL) && (info != NULL) && + (info->algo_type == WC_ALGO_TYPE_FREE) && + (info->free.algo == WC_ALGO_TYPE_PK) && + (info->free.type == WC_PK_TYPE_PQC_SIG_KEYGEN) && + (info->free.subType == WC_PQC_SIG_TYPE_FALCON)) { + const falcon_key* fk = (const falcon_key*)info->free.obj; + + seen->frees++; + if ((fk == NULL) || ((const void*)fk != seen->obj)) { + seen->badObj++; + } + /* The device gets the key while it is still whole: it may need to + * read it to release the right resource, so the software wipe has + * to come after this call, not before. */ + else if (fk->devId != TEST_FALCON_CB_FREE_DEVID) { + seen->wiped++; + } + return seen->ret; + } + + return WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE); +} +#endif /* TEST_FALCON_CB_FREE */ + +/* Freeing a key that names a device has to tell that device, so it can + * release what it holds. A key with no device must not, and neither must a + * second free of a key already freed: a freed key names no device. A device + * that reports an error does not stop the software cleanup. */ +int test_falcon_cb_free(void) +{ + EXPECT_DECLS; +#ifdef TEST_FALCON_CB_FREE + falcon_key key; + FalconCbFreeCtx seen; + + XMEMSET(&key, 0, sizeof(key)); + XMEMSET(&seen, 0, sizeof(seen)); + seen.obj = &key; + + ExpectIntEQ(wc_CryptoCb_RegisterDevice(TEST_FALCON_CB_FREE_DEVID, + falcon_cb_free_cb, &seen), 0); + + ExpectIntEQ(wc_falcon_init_ex(&key, NULL, TEST_FALCON_CB_FREE_DEVID), 0); + wc_falcon_free(&key); + ExpectIntEQ(seen.frees, 1); + ExpectIntEQ(seen.badObj, 0); + ExpectIntEQ(seen.wiped, 0); + ExpectIntEQ(key.devId, INVALID_DEVID); + + wc_falcon_free(&key); + ExpectIntEQ(seen.frees, 1); + + /* A device that fails still leaves the key cleaned up locally. */ + seen.ret = WC_NO_ERR_TRACE(WC_HW_E); + XMEMSET(&key, 0, sizeof(key)); + ExpectIntEQ(wc_falcon_init_ex(&key, NULL, TEST_FALCON_CB_FREE_DEVID), 0); + ExpectIntEQ(wc_falcon_set_level(&key, 1), 0); + wc_falcon_free(&key); + ExpectIntEQ(seen.frees, 2); + ExpectIntEQ(key.devId, INVALID_DEVID); + ExpectIntEQ(key.level, 0); + seen.ret = 0; + + XMEMSET(&key, 0, sizeof(key)); + ExpectIntEQ(wc_falcon_init_ex(&key, NULL, INVALID_DEVID), 0); + wc_falcon_free(&key); + ExpectIntEQ(seen.frees, 2); + + wc_CryptoCb_UnRegisterDevice(TEST_FALCON_CB_FREE_DEVID); +#endif + return EXPECT_RESULT(); +} diff --git a/tests/api/test_falcon.h b/tests/api/test_falcon.h index 44061bcb43..75f5d42897 100644 --- a/tests/api/test_falcon.h +++ b/tests/api/test_falcon.h @@ -32,6 +32,7 @@ int test_wc_falcon_check_key(void); int test_wc_falcon_der(void); int test_wc_falcon_error_paths(void); int test_wc_FalconDecisionCoverage(void); +int test_falcon_cb_free(void); #define TEST_FALCON_DECLS \ TEST_DECL_GROUP("falcon", test_wc_falcon_sizes), \ @@ -41,6 +42,7 @@ int test_wc_FalconDecisionCoverage(void); TEST_DECL_GROUP("falcon", test_wc_falcon_check_key), \ TEST_DECL_GROUP("falcon", test_wc_falcon_der), \ TEST_DECL_GROUP("falcon", test_wc_falcon_error_paths), \ - TEST_DECL_GROUP("falcon", test_wc_FalconDecisionCoverage) + TEST_DECL_GROUP("falcon", test_wc_FalconDecisionCoverage), \ + TEST_DECL_GROUP("falcon", test_falcon_cb_free) #endif /* WOLFCRYPT_TEST_FALCON_H */ diff --git a/tests/api/test_mldsa.c b/tests/api/test_mldsa.c index 3bcf57b604..80d02f50b9 100644 --- a/tests/api/test_mldsa.c +++ b/tests/api/test_mldsa.c @@ -31410,23 +31410,43 @@ int test_wc_MldsaDerDecisionCoverage(void) #endif #ifdef TEST_MLDSA_CB_FREE +/* What the free callback saw, so the test can check the contract rather than + * just that something fired. */ +typedef struct { + int frees; /* matching free callbacks seen */ + int badObj; /* callback was handed the wrong object */ + int wiped; /* callback saw a key already cleaned up */ + int ret; /* what the callback returns */ + const void* obj; /* object the free is expected to name */ +} MlDsaCbFreeCtx; + /* Stands in for a device holding state for the key. Counting the call proves * wc_MlDsaKey_Free told the device rather than only cleaning up in software, * which would leave the device side of the key behind. */ static int mldsa_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) { - int* frees = (int*)ctx; + MlDsaCbFreeCtx* seen = (MlDsaCbFreeCtx*)ctx; (void)devIdArg; - if ((info != NULL) && (info->algo_type == WC_ALGO_TYPE_FREE) && + if ((seen != NULL) && (info != NULL) && + (info->algo_type == WC_ALGO_TYPE_FREE) && (info->free.algo == WC_ALGO_TYPE_PK) && (info->free.type == WC_PK_TYPE_PQC_SIG_KEYGEN) && (info->free.subType == WC_PQC_SIG_TYPE_MLDSA)) { - if (frees != NULL) { - (*frees)++; + const wc_MlDsaKey* dil = (const wc_MlDsaKey*)info->free.obj; + + seen->frees++; + if ((dil == NULL) || ((const void*)dil != seen->obj)) { + seen->badObj++; } - return 0; + /* The device gets the key while it is still whole: it may need to + * read it to release the right resource, so the software wipe has + * to come after this call, not before. */ + else if (dil->devId != TEST_MLDSA_CB_FREE_DEVID) { + seen->wiped++; + } + return seen->ret; } return WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE); @@ -31434,26 +31454,49 @@ static int mldsa_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) #endif /* TEST_MLDSA_CB_FREE */ /* Freeing a key that names a device has to tell that device, so it can - * release what it holds. A key with no device must not. */ + * release what it holds. A key with no device must not, and neither must a + * second free of a key already freed: a freed key names no device. A device + * that reports an error does not stop the software cleanup. */ int test_mldsa_cb_free(void) { EXPECT_DECLS; #ifdef TEST_MLDSA_CB_FREE wc_MlDsaKey* key = NULL; - int frees = 0; + MlDsaCbFreeCtx seen; + + XMEMSET(&seen, 0, sizeof(seen)); ExpectIntEQ(wc_CryptoCb_RegisterDevice(TEST_MLDSA_CB_FREE_DEVID, - mldsa_cb_free_cb, &frees), 0); + mldsa_cb_free_cb, &seen), 0); ExpectNotNull(key = (wc_MlDsaKey*)XMALLOC(sizeof(wc_MlDsaKey), NULL, DYNAMIC_TYPE_TMP_BUFFER)); + seen.obj = key; ExpectIntEQ(wc_MlDsaKey_Init(key, NULL, TEST_MLDSA_CB_FREE_DEVID), 0); wc_MlDsaKey_Free(key); - ExpectIntEQ(frees, 1); + ExpectIntEQ(seen.frees, 1); + ExpectIntEQ(seen.badObj, 0); + ExpectIntEQ(seen.wiped, 0); + if (key != NULL) { + ExpectIntEQ(key->devId, INVALID_DEVID); + } + + wc_MlDsaKey_Free(key); + ExpectIntEQ(seen.frees, 1); + + /* A device that fails still leaves the key cleaned up locally. */ + seen.ret = WC_NO_ERR_TRACE(WC_HW_E); + ExpectIntEQ(wc_MlDsaKey_Init(key, NULL, TEST_MLDSA_CB_FREE_DEVID), 0); + wc_MlDsaKey_Free(key); + ExpectIntEQ(seen.frees, 2); + if (key != NULL) { + ExpectIntEQ(key->devId, INVALID_DEVID); + } + seen.ret = 0; ExpectIntEQ(wc_MlDsaKey_Init(key, NULL, INVALID_DEVID), 0); wc_MlDsaKey_Free(key); - ExpectIntEQ(frees, 1); + ExpectIntEQ(seen.frees, 2); XFREE(key, NULL, DYNAMIC_TYPE_TMP_BUFFER); wc_CryptoCb_UnRegisterDevice(TEST_MLDSA_CB_FREE_DEVID); diff --git a/tests/api/test_mlkem.c b/tests/api/test_mlkem.c index e0623365cf..1da57c826a 100644 --- a/tests/api/test_mlkem.c +++ b/tests/api/test_mlkem.c @@ -4650,8 +4650,8 @@ int test_wc_mlkem_encode_key_len_decision(void) return EXPECT_RESULT(); } /* END test_wc_mlkem_encode_key_len_decision */ -#if defined(WOLFSSL_HAVE_MLKEM) && defined(WOLF_CRYPTO_CB) && \ - defined(WOLF_CRYPTO_CB_FREE) +#if defined(WOLFSSL_HAVE_MLKEM) && !defined(WOLFSSL_NO_ML_KEM) && \ + defined(WOLF_CRYPTO_CB) && defined(WOLF_CRYPTO_CB_FREE) #define TEST_MLKEM_CB_FREE #define TEST_MLKEM_CB_FREE_DEVID 0x4D4C4B4D #ifndef WOLFSSL_NO_ML_KEM_512 @@ -4666,23 +4666,43 @@ int test_wc_mlkem_encode_key_len_decision(void) #endif #ifdef TEST_MLKEM_CB_FREE +/* What the free callback saw, so the test can check the contract rather than + * just that something fired. */ +typedef struct { + int frees; /* matching free callbacks seen */ + int badObj; /* callback was handed the wrong object */ + int wiped; /* callback saw a key already cleaned up */ + int ret; /* what the callback returns */ + const void* obj; /* object the free is expected to name */ +} MlKemCbFreeCtx; + /* Stands in for a device holding state for the key. Counting the call proves * wc_MlKemKey_Free told the device rather than only cleaning up in software, * which would leave the device side of the key behind. */ static int mlkem_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) { - int* frees = (int*)ctx; + MlKemCbFreeCtx* seen = (MlKemCbFreeCtx*)ctx; (void)devIdArg; - if ((info != NULL) && (info->algo_type == WC_ALGO_TYPE_FREE) && + if ((seen != NULL) && (info != NULL) && + (info->algo_type == WC_ALGO_TYPE_FREE) && (info->free.algo == WC_ALGO_TYPE_PK) && (info->free.type == WC_PK_TYPE_PQC_KEM_KEYGEN) && (info->free.subType == WC_PQC_KEM_TYPE_MLKEM)) { - if (frees != NULL) { - (*frees)++; + const MlKemKey* mk = (const MlKemKey*)info->free.obj; + + seen->frees++; + if ((mk == NULL) || ((const void*)mk != seen->obj)) { + seen->badObj++; + } + /* The device gets the key while it is still whole: it may need to + * read it to release the right resource, so the software wipe has + * to come after this call, not before. */ + else if (mk->devId != TEST_MLKEM_CB_FREE_DEVID) { + seen->wiped++; } - return 0; + return seen->ret; } return WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE); @@ -4690,28 +4710,62 @@ static int mlkem_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) #endif /* TEST_MLKEM_CB_FREE */ /* Freeing a key that names a device has to tell that device, so it can - * release what it holds. A key with no device must not. */ + * release what it holds. A key with no device must not, and neither must a + * second free of a key already freed: neither the key nor the hash and PRF + * objects inside it name a device once freed. A device that reports an error + * does not stop the software cleanup. */ int test_wc_mlkem_cb_free(void) { EXPECT_DECLS; #ifdef TEST_MLKEM_CB_FREE MlKemKey* key = NULL; - int frees = 0; + MlKemCbFreeCtx seen; + int freeRet; + + XMEMSET(&seen, 0, sizeof(seen)); ExpectIntEQ(wc_CryptoCb_RegisterDevice(TEST_MLKEM_CB_FREE_DEVID, - mlkem_cb_free_cb, &frees), 0); + mlkem_cb_free_cb, &seen), 0); ExpectNotNull(key = (MlKemKey*)XMALLOC(sizeof(MlKemKey), NULL, DYNAMIC_TYPE_TMP_BUFFER)); + seen.obj = key; ExpectIntEQ(wc_MlKemKey_Init(key, TEST_MLKEM_CB_FREE_TYPE, NULL, TEST_MLKEM_CB_FREE_DEVID), 0); - ExpectIntEQ(wc_MlKemKey_Free(key), 0); - ExpectIntEQ(frees, 1); + /* Freed outside the assertion so the hash and PRF objects are always + * disposed of, even once an earlier check has failed. */ + freeRet = wc_MlKemKey_Free(key); + ExpectIntEQ(freeRet, 0); + ExpectIntEQ(seen.frees, 1); + ExpectIntEQ(seen.badObj, 0); + ExpectIntEQ(seen.wiped, 0); + if (key != NULL) { + ExpectIntEQ(key->devId, INVALID_DEVID); + ExpectIntEQ(key->hash.devId, INVALID_DEVID); + ExpectIntEQ(key->prf.devId, INVALID_DEVID); + } + + freeRet = wc_MlKemKey_Free(key); + ExpectIntEQ(freeRet, 0); + ExpectIntEQ(seen.frees, 1); + + /* A device that fails still leaves the key cleaned up locally. */ + seen.ret = WC_NO_ERR_TRACE(WC_HW_E); + ExpectIntEQ(wc_MlKemKey_Init(key, TEST_MLKEM_CB_FREE_TYPE, NULL, + TEST_MLKEM_CB_FREE_DEVID), 0); + freeRet = wc_MlKemKey_Free(key); + ExpectIntEQ(freeRet, 0); + ExpectIntEQ(seen.frees, 2); + if (key != NULL) { + ExpectIntEQ(key->devId, INVALID_DEVID); + } + seen.ret = 0; ExpectIntEQ(wc_MlKemKey_Init(key, TEST_MLKEM_CB_FREE_TYPE, NULL, INVALID_DEVID), 0); - ExpectIntEQ(wc_MlKemKey_Free(key), 0); - ExpectIntEQ(frees, 1); + freeRet = wc_MlKemKey_Free(key); + ExpectIntEQ(freeRet, 0); + ExpectIntEQ(seen.frees, 2); XFREE(key, NULL, DYNAMIC_TYPE_TMP_BUFFER); wc_CryptoCb_UnRegisterDevice(TEST_MLKEM_CB_FREE_DEVID); diff --git a/tests/api/test_slhdsa.c b/tests/api/test_slhdsa.c index d5b43970d9..cbf674fd68 100644 --- a/tests/api/test_slhdsa.c +++ b/tests/api/test_slhdsa.c @@ -3879,23 +3879,44 @@ int test_slhdsa_get_sigalg_info(void) #endif #ifdef TEST_SLHDSA_CB_FREE +/* What the free callback saw, so the test can check the contract rather than + * just that something fired. */ +typedef struct { + int frees; /* matching free callbacks seen */ + int badObj; /* callback was handed the wrong object */ + int wiped; /* callback saw a key already cleaned up */ + int ret; /* what the callback returns */ + const void* obj; /* object the free is expected to name */ +} SlhDsaCbFreeCtx; + /* Stands in for a device holding state for the key. Counting the call proves * wc_SlhDsaKey_Free told the device rather than only cleaning up in software, * which would leave the device side of the key behind. */ static int slhdsa_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) { - int* frees = (int*)ctx; + SlhDsaCbFreeCtx* seen = (SlhDsaCbFreeCtx*)ctx; (void)devIdArg; - if ((info != NULL) && (info->algo_type == WC_ALGO_TYPE_FREE) && + if ((seen != NULL) && (info != NULL) && + (info->algo_type == WC_ALGO_TYPE_FREE) && (info->free.algo == WC_ALGO_TYPE_PK) && (info->free.type == WC_PK_TYPE_PQC_SIG_KEYGEN) && (info->free.subType == WC_PQC_SIG_TYPE_SLHDSA)) { - if (frees != NULL) { - (*frees)++; + const SlhDsaKey* slh = (const SlhDsaKey*)info->free.obj; + + seen->frees++; + if ((slh == NULL) || ((const void*)slh != seen->obj)) { + seen->badObj++; } - return 0; + /* The device gets the key while it is still whole: it may need to + * read it to release the right resource, so the software wipe has + * to come after this call, not before. */ + else if ((slh->devId != TEST_SLHDSA_CB_FREE_DEVID) || + (slh->params == NULL)) { + seen->wiped++; + } + return seen->ret; } return WC_NO_ERR_TRACE(CRYPTOCB_UNAVAILABLE); @@ -3903,29 +3924,51 @@ static int slhdsa_cb_free_cb(int devIdArg, wc_CryptoInfo* info, void* ctx) #endif /* TEST_SLHDSA_CB_FREE */ /* Freeing a key that names a device has to tell that device, so it can - * release what it holds. A key with no device must not. */ + * release what it holds. A key with no device must not, and neither must a + * second free of a key already freed: a freed key names no device. A device + * that reports an error does not stop the software cleanup. */ int test_slhdsa_cb_free(void) { EXPECT_DECLS; #ifdef TEST_SLHDSA_CB_FREE SlhDsaKey key; - int frees = 0; + SlhDsaCbFreeCtx seen; XMEMSET(&key, 0, sizeof(key)); + XMEMSET(&seen, 0, sizeof(seen)); + seen.obj = &key; ExpectIntEQ(wc_CryptoCb_RegisterDevice(TEST_SLHDSA_CB_FREE_DEVID, - slhdsa_cb_free_cb, &frees), 0); + slhdsa_cb_free_cb, &seen), 0); ExpectIntEQ(wc_SlhDsaKey_Init(&key, WC_SLHDSA_DEFAULT_PARAM, NULL, TEST_SLHDSA_CB_FREE_DEVID), 0); wc_SlhDsaKey_Free(&key); - ExpectIntEQ(frees, 1); + ExpectIntEQ(seen.frees, 1); + ExpectIntEQ(seen.badObj, 0); + ExpectIntEQ(seen.wiped, 0); + ExpectIntEQ(key.devId, INVALID_DEVID); + ExpectNull(key.params); + + wc_SlhDsaKey_Free(&key); + ExpectIntEQ(seen.frees, 1); + + /* A device that fails still leaves the key cleaned up locally. */ + seen.ret = WC_NO_ERR_TRACE(WC_HW_E); + XMEMSET(&key, 0, sizeof(key)); + ExpectIntEQ(wc_SlhDsaKey_Init(&key, WC_SLHDSA_DEFAULT_PARAM, NULL, + TEST_SLHDSA_CB_FREE_DEVID), 0); + wc_SlhDsaKey_Free(&key); + ExpectIntEQ(seen.frees, 2); + ExpectIntEQ(key.devId, INVALID_DEVID); + ExpectNull(key.params); + seen.ret = 0; XMEMSET(&key, 0, sizeof(key)); ExpectIntEQ(wc_SlhDsaKey_Init(&key, WC_SLHDSA_DEFAULT_PARAM, NULL, INVALID_DEVID), 0); wc_SlhDsaKey_Free(&key); - ExpectIntEQ(frees, 1); + ExpectIntEQ(seen.frees, 2); wc_CryptoCb_UnRegisterDevice(TEST_SLHDSA_CB_FREE_DEVID); #endif diff --git a/wolfcrypt/src/falcon.c b/wolfcrypt/src/falcon.c index 4d7fbb5921..07e06f29f7 100644 --- a/wolfcrypt/src/falcon.c +++ b/wolfcrypt/src/falcon.c @@ -9056,6 +9056,11 @@ void wc_falcon_free(falcon_key* key) } #endif ForceZero(key, sizeof(*key)); +#ifdef WOLF_CRYPTO_CB + /* Zeroing leaves devId at 0, which is a usable device id. Mark the + * key as having no device so a second free does not call out again. */ + key->devId = INVALID_DEVID; +#endif } } diff --git a/wolfcrypt/src/wc_mldsa.c b/wolfcrypt/src/wc_mldsa.c index 353ef26288..9c436dd403 100644 --- a/wolfcrypt/src/wc_mldsa.c +++ b/wolfcrypt/src/wc_mldsa.c @@ -12185,6 +12185,11 @@ void wc_MlDsaKey_Free(wc_MlDsaKey* key) #endif /* Ensure all private data is zeroized. */ ForceZero(key, sizeof(*key)); +#ifdef WOLF_CRYPTO_CB + /* Zeroing leaves devId at 0, which is a usable device id. Mark the + * key as having no device so a second free does not call out again. */ + key->devId = INVALID_DEVID; +#endif } } diff --git a/wolfcrypt/src/wc_mlkem.c b/wolfcrypt/src/wc_mlkem.c index fb4f400a71..541b3def44 100644 --- a/wolfcrypt/src/wc_mlkem.c +++ b/wolfcrypt/src/wc_mlkem.c @@ -591,6 +591,10 @@ int wc_MlKemKey_Free(MlKemKey* key) /* Ensure all private data is zeroed. */ ForceZero(&key->hash, sizeof(key->hash)); ForceZero(&key->prf, sizeof(key->prf)); +#ifdef WOLF_CRYPTO_CB + key->hash.devId = INVALID_DEVID; + key->prf.devId = INVALID_DEVID; +#endif #ifdef WOLFSSL_MLKEM_DYNAMIC_KEYS if (key->priv != NULL) { ForceZero(key->priv, key->privAllocSz); @@ -615,6 +619,12 @@ int wc_MlKemKey_Free(MlKemKey* key) /* Clear flags as values are no longer set. */ key->flags = 0; +#ifdef WOLF_CRYPTO_CB + /* Mark the key as having no device so a second free does not call + * out to it again. */ + key->devCtx = NULL; + key->devId = INVALID_DEVID; +#endif } return 0; From cecf35171d47457d15ce33e060f03efbaf99ab8c Mon Sep 17 00:00:00 2001 From: Paul Adelsbach Date: Thu, 20 Aug 2026 17:46:42 -0700 Subject: [PATCH 4/4] PR feedback: test updates, invalidate shake.devId --- tests/api/test_falcon.c | 3 +++ tests/api/test_mldsa.c | 1 + tests/api/test_mlkem.c | 3 +++ wolfcrypt/src/wc_mldsa.c | 4 +++- 4 files changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/api/test_falcon.c b/tests/api/test_falcon.c index ddb24962cd..eb4c788ac4 100644 --- a/tests/api/test_falcon.c +++ b/tests/api/test_falcon.c @@ -1102,6 +1102,9 @@ int test_falcon_cb_free(void) XMEMSET(&seen, 0, sizeof(seen)); seen.obj = &key; + /* No key to free, and nothing to tell a device about. */ + wc_falcon_free(NULL); + ExpectIntEQ(wc_CryptoCb_RegisterDevice(TEST_FALCON_CB_FREE_DEVID, falcon_cb_free_cb, &seen), 0); diff --git a/tests/api/test_mldsa.c b/tests/api/test_mldsa.c index 80d02f50b9..b0d4b8c18d 100644 --- a/tests/api/test_mldsa.c +++ b/tests/api/test_mldsa.c @@ -31479,6 +31479,7 @@ int test_mldsa_cb_free(void) ExpectIntEQ(seen.wiped, 0); if (key != NULL) { ExpectIntEQ(key->devId, INVALID_DEVID); + ExpectIntEQ(key->shake.devId, INVALID_DEVID); } wc_MlDsaKey_Free(key); diff --git a/tests/api/test_mlkem.c b/tests/api/test_mlkem.c index 1da57c826a..03ebc7183f 100644 --- a/tests/api/test_mlkem.c +++ b/tests/api/test_mlkem.c @@ -4724,6 +4724,9 @@ int test_wc_mlkem_cb_free(void) XMEMSET(&seen, 0, sizeof(seen)); + /* No key to free, and nothing to tell a device about. */ + ExpectIntEQ(wc_MlKemKey_Free(NULL), 0); + ExpectIntEQ(wc_CryptoCb_RegisterDevice(TEST_MLKEM_CB_FREE_DEVID, mlkem_cb_free_cb, &seen), 0); diff --git a/wolfcrypt/src/wc_mldsa.c b/wolfcrypt/src/wc_mldsa.c index 9c436dd403..6547717025 100644 --- a/wolfcrypt/src/wc_mldsa.c +++ b/wolfcrypt/src/wc_mldsa.c @@ -12187,8 +12187,10 @@ void wc_MlDsaKey_Free(wc_MlDsaKey* key) ForceZero(key, sizeof(*key)); #ifdef WOLF_CRYPTO_CB /* Zeroing leaves devId at 0, which is a usable device id. Mark the - * key as having no device so a second free does not call out again. */ + * key and the SHAKE object inside it as having no device: a second + * free runs the SHAKE free again and would otherwise call out. */ key->devId = INVALID_DEVID; + key->shake.devId = INVALID_DEVID; #endif } }