Fix several minor issues. Weili Qian (6): uadk_engine: fix double-free in AEAD and async init uadk_engine: use secure BN allocation for signing nonce uadk_engine: fix uninit read in ECDH derive uadk_engine: fix memory leaks in error paths uadk_engine: fix EC key management cleanup and secure allocation uadk_engine: refactor RSA set0 functions and fix keygen BN allocation Zhushuai Yin (3): uadk_provider: fix one-shot cipher padding and stream-mode handling uadk_provider: fix stream mode ciphertext error on non-aligned multi-update uadk_engine: fix stream mode ciphertext error on non-aligned multi-update src/uadk_aead.c | 4 +- src/uadk_async.c | 7 +- src/uadk_cipher.c | 74 ++++++++++++----- src/uadk_digest.c | 11 ++- src/uadk_ec.c | 9 +++ src/uadk_pkey.c | 4 +- src/uadk_prov_aead.c | 3 +- src/uadk_prov_cipher.c | 165 ++++++++++++++++++++++++++++++-------- src/uadk_prov_ec_kmgmt.c | 4 +- src/uadk_prov_ecdh_exch.c | 2 +- src/uadk_prov_ecx.c | 25 +++--- src/uadk_prov_pkey.c | 4 +- src/uadk_prov_rsa_kmgmt.c | 128 +++++++++-------------------- src/uadk_prov_rsa_sign.c | 9 ++- src/uadk_rsa.c | 92 +++++++++++---------- 15 files changed, 314 insertions(+), 227 deletions(-) -- 2.43.0
In uadk_e_ctx_init(), the error path frees priv->data without NULLing it, so a second re-init failure frees it again. The same issue exists in async_module_init() where poll_queue.head is freed on error but not NULLed. Fix both by setting the pointer to NULL immediately after free. Signed-off-by: Weili Qian <qianweili@huawei.com> --- src/uadk_aead.c | 4 ++-- src/uadk_async.c | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/uadk_aead.c b/src/uadk_aead.c index db5faf7..20e0631 100644 --- a/src/uadk_aead.c +++ b/src/uadk_aead.c @@ -320,8 +320,8 @@ static int uadk_e_ctx_init(struct aead_priv_ctx *priv, const unsigned char *ckey return UADK_E_SUCCESS; free_data: - if (priv->data) - free(priv->data); + free(priv->data); + priv->data = NULL; out: wd_aead_free_sess(priv->sess); priv->sess = 0; diff --git a/src/uadk_async.c b/src/uadk_async.c index 76aa6a1..d38a61a 100644 --- a/src/uadk_async.c +++ b/src/uadk_async.c @@ -390,6 +390,7 @@ destroy_empty_sem: sem_destroy(&poll_queue.empty_sem); free_head: OPENSSL_free(poll_queue.head); + poll_queue.head = NULL; destroy_mutex: pthread_mutex_destroy(&poll_queue.async_task_mutex); @@ -399,7 +400,6 @@ destroy_mutex: void async_module_uninit(void) { int error; - struct async_poll_task *task; /* Disable async poll state first */ uadk_e_set_async_poll_state(DISABLE_ASYNC_POLLING); @@ -413,9 +413,8 @@ void async_module_uninit(void) if (poll_queue.thread_id) pthread_join(poll_queue.thread_id, NULL); - task = poll_queue.head; - if (task) - OPENSSL_free(task); + if (poll_queue.head) + OPENSSL_free(poll_queue.head); poll_queue.head = NULL; -- 2.43.0
The ECDSA/SM2 signing nonce k is allocated with BN_new() in uadk_pkey_get_rand() and uadk_prov_pkey_get_rand(). Since leaking the nonce allows recovery of the private key, it should use BN_secure_new() and BN_clear_free() to keep it in secure memory and zero it on free. Signed-off-by: Weili Qian <qianweili@huawei.com> --- src/uadk_pkey.c | 4 ++-- src/uadk_prov_pkey.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/uadk_pkey.c b/src/uadk_pkey.c index 4f29f5e..9bb6a80 100644 --- a/src/uadk_pkey.c +++ b/src/uadk_pkey.c @@ -496,7 +496,7 @@ int uadk_ecc_get_rand(char *out, size_t out_len, void *usr) return -1; } - k = BN_new(); + k = BN_secure_new(); if (!k) return -ENOMEM; @@ -520,7 +520,7 @@ int uadk_ecc_get_rand(char *out, size_t out_len, void *usr) if (count < 0) ret = -1; err: - BN_free(k); + BN_clear_free(k); return ret; } diff --git a/src/uadk_prov_pkey.c b/src/uadk_prov_pkey.c index 41d0102..68750bd 100644 --- a/src/uadk_prov_pkey.c +++ b/src/uadk_prov_pkey.c @@ -134,7 +134,7 @@ int uadk_prov_ecc_get_rand(char *out, size_t out_len, void *usr) return UADK_P_INVALID; } - k = BN_new(); + k = BN_secure_new(); if (k == NULL) return -ENOMEM; @@ -158,7 +158,7 @@ int uadk_prov_ecc_get_rand(char *out, size_t out_len, void *usr) if (count < 0) ret = UADK_P_INVALID; err: - BN_free(k); + BN_clear_free(k); return ret; } -- 2.43.0
In ecdh_X9_63_kdf_derive(), the second ecdh_plain_derive() is checked with if (!ret) which only catches ret == 0, so a UADK_DO_SOFT (-224) return falls through to ecdh_kdf_X9_63() with unfilled stmp as KDF key. Fix by changing to if (ret != UADK_P_SUCCESS) to redirect to the free_stmp cleanup path and trigger software fallback. Signed-off-by: Weili Qian <qianweili@huawei.com> --- src/uadk_prov_ecdh_exch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uadk_prov_ecdh_exch.c b/src/uadk_prov_ecdh_exch.c index 01473a8..1b3b123 100644 --- a/src/uadk_prov_ecdh_exch.c +++ b/src/uadk_prov_ecdh_exch.c @@ -454,7 +454,7 @@ static int ecdh_X9_63_kdf_derive(struct ecdh_ctx *pecdhctx, unsigned char *secre } ret = ecdh_plain_derive(pecdhctx, stmp, &stmplen, stmplen); - if (!ret) + if (ret != UADK_P_SUCCESS) goto free_stmp; ret = ecdh_kdf_X9_63(secret, pecdhctx, stmp, stmplen); -- 2.43.0
Several functions return on error without freeing allocated resources: uadk_e_digest_soft_work() and uadk_e_digest_copy() skip cleanup labels, leaking the soft ctx; uadk_prov_aead_dupctx() jumps to the wrong label, leaking the WD session; uadk_prov_rsa_private_sign() does not free from_buf on error from rsa_create_pri_bn_ctx or is_valid_rsa_input. Fix by redirecting errors to the appropriate cleanup labels. Signed-off-by: Weili Qian <qianweili@huawei.com> --- src/uadk_digest.c | 11 +++++------ src/uadk_prov_aead.c | 3 +-- src/uadk_prov_rsa_sign.c | 9 +++++---- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/uadk_digest.c b/src/uadk_digest.c index efdc9b8..72d96b3 100644 --- a/src/uadk_digest.c +++ b/src/uadk_digest.c @@ -314,7 +314,7 @@ static int uadk_e_digest_soft_work(struct digest_priv_ctx *md_ctx, int len, ret = digest_soft_init(md_ctx); if (unlikely(!ret)) - return 0; + goto out; if (len != 0) { ret = digest_soft_update(md_ctx, md_ctx->data, len); @@ -632,8 +632,6 @@ static int uadk_e_digest_init(EVP_MD_CTX *ctx) if (unlikely(!priv->sess)) { priv->switch_flag = UADK_DO_SOFT; priv->data = NULL; - priv->soft_md = NULL; - priv->soft_ctx = NULL; return digest_soft_init(priv); } @@ -739,7 +737,7 @@ do_soft_digest: priv->switch_flag = UADK_DO_SOFT; ret = digest_soft_init(priv); if (!ret) - return ret; + goto out; /* filling buf has been executed */ if (processing_len < DIGEST_BLOCK_SIZE) { ret = digest_soft_update(priv, priv->data, DIGEST_BLOCK_SIZE); @@ -1015,7 +1013,7 @@ static int uadk_e_digest_copy(EVP_MD_CTX *to, const EVP_MD_CTX *from) t->soft_ctx = NULL; if (!digest_soft_init(t)) { fprintf(stderr, "failed to init soft for digest ctx copy.\n"); - return 0; + goto free_data; } if (f->soft_ctx) { memcpy(t->soft_ctx->md_data, f->soft_ctx->md_data, @@ -1023,7 +1021,7 @@ static int uadk_e_digest_copy(EVP_MD_CTX *to, const EVP_MD_CTX *from) } else if (f->last_update_bufflen) { if (!digest_soft_update(t, f->data, f->last_update_bufflen)) { fprintf(stderr, "failed to update for digest ctx copy.\n"); - return 0; + goto free_data; } t->last_update_bufflen = 0; } @@ -1058,6 +1056,7 @@ static int uadk_e_digest_copy(EVP_MD_CTX *to, const EVP_MD_CTX *from) return 1; free_data: + digest_soft_cleanup(t); if (t->data) { free(t->data); t->data = NULL; diff --git a/src/uadk_prov_aead.c b/src/uadk_prov_aead.c index 1919d47..83a7510 100644 --- a/src/uadk_prov_aead.c +++ b/src/uadk_prov_aead.c @@ -1680,7 +1680,7 @@ static void *uadk_prov_aead_dupctx(void *ctx) ret = uadk_prov_aead_copy_sess(dst_ctx); if (ret == UADK_OSSL_FAIL) - goto free_ctx; + goto free_sess; if (dst_ctx->sw_ctx) { dst_ctx->sw_ctx = EVP_CIPHER_CTX_dup(src_ctx->sw_ctx); @@ -1701,7 +1701,6 @@ free_dup: EVP_CIPHER_CTX_free(dst_ctx->sw_ctx); free_sess: uadk_prov_aead_free_sess(dst_ctx); -free_ctx: OPENSSL_clear_free(dst_ctx, sizeof(*dst_ctx)); return NULL; } diff --git a/src/uadk_prov_rsa_sign.c b/src/uadk_prov_rsa_sign.c index 5f309f3..e9f9651 100644 --- a/src/uadk_prov_rsa_sign.c +++ b/src/uadk_prov_rsa_sign.c @@ -295,7 +295,7 @@ static int uadk_prov_rsa_private_sign(int flen, const unsigned char *from, ret = rsa_create_pri_bn_ctx(rsa, prik, &from_buf, &num_bytes); if (ret <= 0 || flen > num_bytes) { ret = UADK_P_FAIL; - goto free_sess; + goto free_buf; } ret = add_rsa_prienc_padding(flen, from, from_buf, num_bytes, padding); @@ -305,8 +305,10 @@ static int uadk_prov_rsa_private_sign(int flen, const unsigned char *from, } ret = is_valid_rsa_input(from_buf, num_bytes, rsa); - if (!ret) - return UADK_P_FAIL; + if (!ret) { + ret = UADK_P_FAIL; + goto free_buf; + } ret = rsa_fill_prikey(rsa, rsa_sess, prik, from_buf, to); if (!ret) { @@ -324,7 +326,6 @@ static int uadk_prov_rsa_private_sign(int flen, const unsigned char *from, free_buf: rsa_free_pri_bn_ctx(from_buf); -free_sess: rsa_free_eng_session(rsa_sess); free_pkey: rsa_pkey_param_free(NULL, &prik); -- 2.43.0
The EC private key in uadk_prov_ec_kmgmt.c uses BN_new() instead of BN_secure_new(), leaving sensitive material in non-secure memory. The EC gen_init error path calls OPENSSL_free() instead of the proper cleanup function, skipping propq and ikm cleanup. The ECX gen_cleanup functions delegate to the default provider cleanup via get_default_x25519/x448_keymgmt().gen_cleanup(), which does not handle our extended fields (propq, dhkem_ikm). Replace both with a local uadk_keymgmt_ecx_cleanup() that properly frees all fields. Also add a comment in uadk_ec.c explaining why pkey_meth->ec must not be freed in uadk_ec_delete_meth() to prevent a double-free. Signed-off-by: Weili Qian <qianweili@huawei.com> --- src/uadk_ec.c | 9 +++++++++ src/uadk_prov_ec_kmgmt.c | 4 ++-- src/uadk_prov_ecx.c | 25 +++++++++++++++---------- 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/uadk_ec.c b/src/uadk_ec.c index d241e79..1114434 100644 --- a/src/uadk_ec.c +++ b/src/uadk_ec.c @@ -1464,6 +1464,15 @@ int uadk_ec_create_pmeth(struct uadk_pkey_meth *pkey_meth) if (pkey_meth->ec) return 1; + /* + * The meth is returned to OpenSSL via the pkey_meths callback + * (get_pkey_meths in uadk_pkey.c) and registered by ENGINE_set_pkey_meths. + * On ENGINE_free, engine_pkey_meths_free() (tb_pkmeth.c) iterates all + * registered nids, obtains the meth via the callback, and calls + * EVP_PKEY_meth_free() on it. Thus pkey_meth->ec must NOT be freed in + * uadk_ec_delete_meth() - doing so would cause a double-free because + * engine_pkey_meths_free() runs before the engine's destroy callback. + */ meth = EVP_PKEY_meth_new(EVP_PKEY_EC, 0); if (meth == NULL) { fprintf(stderr, "failed to EVP_PKEY_meth_new\n"); diff --git a/src/uadk_prov_ec_kmgmt.c b/src/uadk_prov_ec_kmgmt.c index 48c7b18..9896d09 100644 --- a/src/uadk_prov_ec_kmgmt.c +++ b/src/uadk_prov_ec_kmgmt.c @@ -202,7 +202,7 @@ static int ec_set_private_key(EC_KEY *ec, BIGNUM *priv_key) if (priv_k) goto set_key; - priv_k = BN_new(); + priv_k = BN_secure_new(); if (!priv_k) { UADK_ERR("failed to BN_new priv_k!\n"); return UADK_P_FAIL; @@ -450,7 +450,7 @@ static void *uadk_keymgmt_ec_gen_init(void *provctx, int selection, ret = uadk_keymgmt_ec_gen_set_params(gctx, params); if (!ret) { - OPENSSL_free(gctx); + uadk_keymgmt_ec_gen_cleanup(gctx); return NULL; } diff --git a/src/uadk_prov_ecx.c b/src/uadk_prov_ecx.c index 5550af8..1818219 100644 --- a/src/uadk_prov_ecx.c +++ b/src/uadk_prov_ecx.c @@ -446,6 +446,19 @@ static void uadk_prov_ecx_free_sess(handle_t sess) wd_ecc_free_sess(sess); } +static void uadk_keymgmt_ecx_cleanup(void *genctx) +{ + PROV_ECX_KEYMGMT_CTX *gctx = genctx; + + if (gctx) { + OPENSSL_free(gctx->propq); +# if OPENSSL_VERSION_NUMBER >= 0x30200000L + OPENSSL_clear_free(gctx->dhkem_ikm, gctx->dhkem_ikmlen); +# endif + OPENSSL_free(gctx); + } +} + static void *ossl_ecx_gen_init(void *provctx, int selection, const OSSL_PARAM params[], ECX_KEY_TYPE type) { @@ -475,11 +488,7 @@ static void *ossl_ecx_gen_init(void *provctx, int selection, const OSSL_PARAM pa static void uadk_keymgmt_x448_gen_cleanup(void *genctx) { - /* genctx will be freed in cleanup function */ - if (get_default_x448_keymgmt().gen_cleanup == NULL) - return; - - get_default_x448_keymgmt().gen_cleanup(genctx); + uadk_keymgmt_ecx_cleanup(genctx); } static void *uadk_keymgmt_x448_gen_init(void *provctx, int selection, @@ -1481,11 +1490,7 @@ static int uadk_keymgmt_x25519_get_params(void *key, OSSL_PARAM params[]) static void uadk_keymgmt_x25519_gen_cleanup(void *genctx) { - /* genctx will be freed in cleanup function */ - if (get_default_x25519_keymgmt().gen_cleanup == NULL) - return; - - get_default_x25519_keymgmt().gen_cleanup(genctx); + uadk_keymgmt_ecx_cleanup(genctx); } static void *uadk_keymgmt_x25519_gen_init(void *provctx, int selection, -- 2.43.0
The RSA set0 helper functions return int with NULL-parameter guards, but callers always pass valid pointers. Convert to void and remove the guards to simplify the code. RSA keygen uses BN_CTX_get to allocate temporary BIGNUMs, but BN_CTX_end wipes the context on success, making the BIGNUMs unavailable after the RSA object is populated via set0. Replace BN_CTX_get with individual BN_new/BN_secure_new allocations and explicit cleanup labels. Also fix BN type mismatches: RSA CRT iqmp should use BN_secure_new, and public-key BIGNUMs (n, e) should use BN_free instead of BN_clear_free. Signed-off-by: Weili Qian <qianweili@huawei.com> --- src/uadk_prov_rsa_kmgmt.c | 128 +++++++++++--------------------------- src/uadk_rsa.c | 92 ++++++++++++++------------- 2 files changed, 82 insertions(+), 138 deletions(-) diff --git a/src/uadk_prov_rsa_kmgmt.c b/src/uadk_prov_rsa_kmgmt.c index 7f2ea79..2da157c 100644 --- a/src/uadk_prov_rsa_kmgmt.c +++ b/src/uadk_prov_rsa_kmgmt.c @@ -123,93 +123,46 @@ static void uadk_rsa_set_flags(RSA *r, int flags) r->flags |= flags; } -static int uadk_rsa_set0_key(RSA *r, BIGNUM *n, BIGNUM *e, BIGNUM *d) +static void uadk_rsa_set0_key(RSA *r, BIGNUM *n, BIGNUM *e, BIGNUM *d) { - /* - * If the fields n and e in r are NULL, the corresponding input - * parameters MUST be non-NULL for n and e. d may be - * left NULL (in case only the public key is used). - */ - if ((!r->n && !n) - || (!r->e && !e)) - return UADK_P_FAIL; - - if (n != NULL) { - BN_free(r->n); - r->n = n; - } - if (e != NULL) { - BN_free(r->e); - r->e = e; - } - if (d != NULL) { - BN_clear_free(r->d); - r->d = d; - BN_set_flags(r->d, BN_FLG_CONSTTIME); - } + BN_free(r->n); + r->n = n; + BN_free(r->e); + r->e = e; + BN_clear_free(r->d); + r->d = d; + BN_set_flags(r->d, BN_FLG_CONSTTIME); r->dirty_cnt++; - - return UADK_P_SUCCESS; } -static int uadk_rsa_set0_factors(RSA *r, BIGNUM *p, BIGNUM *q) +static void uadk_rsa_set0_factors(RSA *r, BIGNUM *p, BIGNUM *q) { - /* - * If the fields p and q in r are NULL, the corresponding input - * parameters MUST be non-NULL. - */ - if ((!r->p && !p) || (!r->q && !q)) - return UADK_P_FAIL; - - if (p != NULL) { - BN_clear_free(r->p); - r->p = p; - BN_set_flags(r->p, BN_FLG_CONSTTIME); - } + BN_clear_free(r->p); + r->p = p; + BN_set_flags(r->p, BN_FLG_CONSTTIME); - if (q != NULL) { - BN_clear_free(r->q); - r->q = q; - BN_set_flags(r->q, BN_FLG_CONSTTIME); - } + BN_clear_free(r->q); + r->q = q; + BN_set_flags(r->q, BN_FLG_CONSTTIME); r->dirty_cnt++; - - return UADK_P_SUCCESS; } -static int uadk_rsa_set0_crt_params(RSA *r, BIGNUM *dmp1, BIGNUM *dmq1, BIGNUM *iqmp) +static void uadk_rsa_set0_crt_params(RSA *r, BIGNUM *dmp1, BIGNUM *dmq1, BIGNUM *iqmp) { - /* - * If the fields dmp1, dmq1 and iqmp in r are NULL, the corresponding input - * parameters MUST be non-NULL. - */ - if ((!r->dmp1 && !dmp1) - || (!r->dmq1 && !dmq1) - || (!r->iqmp && !iqmp)) - return UADK_P_FAIL; - - if (dmp1 != NULL) { - BN_clear_free(r->dmp1); - r->dmp1 = dmp1; - BN_set_flags(r->dmp1, BN_FLG_CONSTTIME); - } + BN_clear_free(r->dmp1); + r->dmp1 = dmp1; + BN_set_flags(r->dmp1, BN_FLG_CONSTTIME); - if (dmq1 != NULL) { - BN_clear_free(r->dmq1); - r->dmq1 = dmq1; - BN_set_flags(r->dmq1, BN_FLG_CONSTTIME); - } + BN_clear_free(r->dmq1); + r->dmq1 = dmq1; + BN_set_flags(r->dmq1, BN_FLG_CONSTTIME); - if (iqmp != NULL) { - BN_clear_free(r->iqmp); - r->iqmp = iqmp; - BN_set_flags(r->iqmp, BN_FLG_CONSTTIME); - } + BN_clear_free(r->iqmp); + r->iqmp = iqmp; + BN_set_flags(r->iqmp, BN_FLG_CONSTTIME); r->dirty_cnt++; - - return UADK_P_SUCCESS; } static int rsa_prime_mul_res(int num, struct rsa_prime_param *param, @@ -608,7 +561,7 @@ static int rsa_get_keygen_param(struct wd_rsa_req *req, handle_t ctx, RSA *rsa, if (!dmp1) goto free_bn_dq; - iqmp = BN_new(); + iqmp = BN_secure_new(); if (!iqmp) goto free_bn_dp; @@ -626,18 +579,15 @@ static int rsa_get_keygen_param(struct wd_rsa_req *req, handle_t ctx, RSA *rsa, BN_bin2bn((unsigned char *)wd_dq.data, wd_dq.dsize, dmq1); BN_bin2bn((unsigned char *)wd_dp.data, wd_dp.dsize, dmp1); - if (!(uadk_rsa_set0_key(rsa, n, bn_param->e, d) && - uadk_rsa_set0_factors(rsa, bn_param->p, bn_param->q) && - uadk_rsa_set0_crt_params(rsa, dmp1, dmq1, iqmp))) - goto free_bn_d; + uadk_rsa_set0_key(rsa, n, bn_param->e, d); + uadk_rsa_set0_factors(rsa, bn_param->p, bn_param->q); + uadk_rsa_set0_crt_params(rsa, dmp1, dmq1, iqmp); return UADK_P_SUCCESS; -free_bn_d: - BN_clear_free(d); -free_bn_n: - BN_clear_free(n); -free_bn_qinv: + free_bn_n: + BN_free(n); + free_bn_qinv: BN_clear_free(iqmp); free_bn_dp: BN_clear_free(dmp1); @@ -661,16 +611,14 @@ static void rsa_keygen_param_free(struct rsa_keygen_param **keygen_param, struct rsa_keypair **key_pair, int free_bn_ctx_tag) { /* - * When an abnormal situation occurs, uadk engine needs to - * switch to software keygen function, so we need to free - * BN we alloced before. But in normal situation, - * the BN should be freed by OpenSSL tools or users. - * Therefore, we use a tag to distinguish these cases. + * On success, p/q/e have been transferred to the RSA object via + * uadk_rsa_set0_*, so they must not be freed here. On any failure, + * they are still owned by us and must be freed. */ - if (free_bn_ctx_tag == UADK_DO_SOFT) { + if (free_bn_ctx_tag != UADK_P_SUCCESS) { BN_clear_free((*keygen_bn_param)->p); BN_clear_free((*keygen_bn_param)->q); - BN_clear_free((*keygen_bn_param)->e); + BN_free((*keygen_bn_param)->e); } OPENSSL_free(*key_pair); @@ -770,8 +718,6 @@ static int uadk_prov_rsa_keygen(RSA *rsa, int bits, BIGNUM *e, BN_GENCB *cb) } ret = rsa_get_keygen_param(&rsa_sess->req, rsa_sess->sess, rsa, bn_param); - if (!ret) - ret = UADK_P_FAIL; free_kg_in_out: rsa_free_keygen_data(rsa_sess); diff --git a/src/uadk_rsa.c b/src/uadk_rsa.c index 8aa4757..e718853 100644 --- a/src/uadk_rsa.c +++ b/src/uadk_rsa.c @@ -1081,13 +1081,12 @@ static int rsa_fill_prikey(RSA *rsa, struct uadk_rsa_sess *rsa_sess, } static int rsa_get_keygen_param(struct wd_rsa_req *req, handle_t ctx, RSA *rsa, - struct rsa_keygen_param_bn *bn_param, BN_CTX **bn_ctx_in) + struct rsa_keygen_param_bn *bn_param) { struct wd_rsa_kg_out *out = (struct wd_rsa_kg_out *)req->dst; struct wd_dtb wd_d, wd_n, wd_qinv, wd_dq, wd_dp; BIGNUM *dmp1, *dmq1, *iqmp, *n, *d; unsigned int key_bits, key_size; - BN_CTX *bn_ctx = *bn_ctx_in; key_bits = wd_rsa_get_key_bits(ctx); if (!key_bits) @@ -1097,25 +1096,25 @@ static int rsa_get_keygen_param(struct wd_rsa_req *req, handle_t ctx, RSA *rsa, wd_rsa_get_kg_out_params(out, &wd_d, &wd_n); wd_rsa_get_kg_out_crt_params(out, &wd_qinv, &wd_dq, &wd_dp); - dmp1 = BN_CTX_get(bn_ctx); + dmp1 = BN_secure_new(); if (!dmp1) return UADK_E_FAIL; - dmq1 = BN_CTX_get(bn_ctx); + dmq1 = BN_secure_new(); if (!dmq1) - return UADK_E_FAIL; + goto free_dmp1; - iqmp = BN_CTX_get(bn_ctx); + iqmp = BN_secure_new(); if (!iqmp) - return UADK_E_FAIL; + goto free_dmq1; - n = BN_CTX_get(bn_ctx); + n = BN_new(); if (!n) - return UADK_E_FAIL; + goto free_iqmp; - d = BN_CTX_get(bn_ctx); + d = BN_secure_new(); if (!d) - return UADK_E_FAIL; + goto free_n; BN_bin2bn((unsigned char *)wd_d.data, key_size, d); BN_bin2bn((unsigned char *)wd_n.data, key_size, n); @@ -1123,12 +1122,21 @@ static int rsa_get_keygen_param(struct wd_rsa_req *req, handle_t ctx, RSA *rsa, BN_bin2bn((unsigned char *)wd_dq.data, wd_dq.dsize, dmq1); BN_bin2bn((unsigned char *)wd_dp.data, wd_dp.dsize, dmp1); - if (!(RSA_set0_key(rsa, n, bn_param->e, d) && - RSA_set0_factors(rsa, bn_param->p, bn_param->q) && - RSA_set0_crt_params(rsa, dmp1, dmq1, iqmp))) - return UADK_E_FAIL; + RSA_set0_key(rsa, n, bn_param->e, d); + RSA_set0_factors(rsa, bn_param->p, bn_param->q); + RSA_set0_crt_params(rsa, dmp1, dmq1, iqmp); return UADK_E_SUCCESS; + + free_n: + BN_free(n); +free_iqmp: + BN_clear_free(iqmp); +free_dmq1: + BN_clear_free(dmq1); +free_dmp1: + BN_clear_free(dmp1); + return UADK_E_FAIL; } static void uadk_e_rsa_cb(void *req_t) @@ -1329,10 +1337,8 @@ static void rsa_free_keygen_data(struct uadk_rsa_sess *rsa_sess) static int rsa_keygen_param_alloc(struct rsa_keygen_param **keygen_param, struct rsa_keygen_param_bn **keygen_bn_param, - struct rsa_keypair **key_pair, BN_CTX **bn_ctx_in) + struct rsa_keypair **key_pair) { - BN_CTX *bn_ctx; - *keygen_param = OPENSSL_malloc(sizeof(struct rsa_keygen_param)); if (!(*keygen_param)) goto err; @@ -1346,30 +1352,24 @@ static int rsa_keygen_param_alloc(struct rsa_keygen_param **keygen_param, if (!(*key_pair)) goto free_keygen_bn_param; - bn_ctx = BN_CTX_new(); - if (!bn_ctx) - goto free_key_pair; - - BN_CTX_start(bn_ctx); - *bn_ctx_in = bn_ctx; - - (*keygen_bn_param)->e = BN_CTX_get(bn_ctx); + (*keygen_bn_param)->e = BN_new(); if (!(*keygen_bn_param)->e) - goto free_bn_ctx; + goto free_key_pair; - (*keygen_bn_param)->p = BN_CTX_get(bn_ctx); + (*keygen_bn_param)->p = BN_secure_new(); if (!(*keygen_bn_param)->p) - goto free_bn_ctx; + goto free_bn_e; - (*keygen_bn_param)->q = BN_CTX_get(bn_ctx); + (*keygen_bn_param)->q = BN_secure_new(); if (!(*keygen_bn_param)->q) - goto free_bn_ctx; + goto free_bn_p; return UADK_E_SUCCESS; -free_bn_ctx: - BN_CTX_end(bn_ctx); - BN_CTX_free(bn_ctx); +free_bn_p: + BN_clear_free((*keygen_bn_param)->p); +free_bn_e: + BN_free((*keygen_bn_param)->e); free_key_pair: OPENSSL_free(*key_pair); free_keygen_bn_param: @@ -1382,19 +1382,18 @@ err: static void rsa_keygen_param_free(struct rsa_keygen_param **keygen_param, struct rsa_keygen_param_bn **keygen_bn_param, - struct rsa_keypair **key_pair, BN_CTX **bn_ctx, + struct rsa_keypair **key_pair, int free_bn_ctx_tag) { /* - * When an abnormal situation occurs, uadk engine needs - * to switch to software keygen function, so we need to - * free BN ctx we alloced before. But in normal situation, - * the BN ctx should be freed by OpenSSL tools or users. - * Therefore, we use a tag to distinguish these cases. + * On success, e/p/q have been transferred to the RSA object via + * RSA_set0_*, so they must not be freed here. On failure, they are + * still owned by us and must be freed. */ - if (free_bn_ctx_tag == UADK_DO_SOFT) { - BN_CTX_end(*bn_ctx); - BN_CTX_free(*bn_ctx); + if (free_bn_ctx_tag != UADK_E_SUCCESS) { + BN_clear_free((*keygen_bn_param)->p); + BN_clear_free((*keygen_bn_param)->q); + BN_free((*keygen_bn_param)->e); } OPENSSL_free(*keygen_bn_param); @@ -1495,7 +1494,6 @@ static int uadk_e_rsa_keygen(RSA *rsa, int bits, BIGNUM *e, BN_GENCB *cb) struct rsa_keygen_param_bn *bn_param = NULL; struct rsa_keypair *key_pair = NULL; struct uadk_rsa_sess *rsa_sess; - BN_CTX *bn_ctx = NULL; int is_crt = 1; int ret; @@ -1509,7 +1507,7 @@ static int uadk_e_rsa_keygen(RSA *rsa, int bits, BIGNUM *e, BN_GENCB *cb) if (ret != UADK_INIT_SUCCESS) goto exe_soft; - ret = rsa_keygen_param_alloc(&keygen_param, &bn_param, &key_pair, &bn_ctx); + ret = rsa_keygen_param_alloc(&keygen_param, &bn_param, &key_pair); if (ret == -ENOMEM) return ret; @@ -1542,7 +1540,7 @@ static int uadk_e_rsa_keygen(RSA *rsa, int bits, BIGNUM *e, BN_GENCB *cb) goto free_kg_in_out; } - ret = rsa_get_keygen_param(&rsa_sess->req, rsa_sess->sess, rsa, bn_param, &bn_ctx); + ret = rsa_get_keygen_param(&rsa_sess->req, rsa_sess->sess, rsa, bn_param); if (!ret) ret = UADK_DO_SOFT; @@ -1551,7 +1549,7 @@ free_kg_in_out: free_sess: rsa_free_eng_session(rsa_sess); free_keygen: - rsa_keygen_param_free(&keygen_param, &bn_param, &key_pair, &bn_ctx, ret); + rsa_keygen_param_free(&keygen_param, &bn_param, &key_pair, ret); if (ret != UADK_DO_SOFT) return ret; soft_log: -- 2.43.0
From: Zhushuai Yin <yinzhushuai@huawei.com> The one-shot cipher callback (OSSL_FUNC_CIPHER_CIPHER, uadk_prov_cipher_cipher) always forwarded the input to uadk_prov_do_cipher and reported outl=inl. This produced wrong output for two cases: - Block ciphers with padding enabled (pad=1): the default provider does a raw encrypt/decrypt without PKCS#7 padding in the one-shot path (outl=inl), but the HW path appended padding, so the ciphertext length and bytes diverged from the default provider. - Stream modes CTR/CFB/OFB with non-16B-aligned input: the HW SEC engine requires 16B-aligned BDs, so non-aligned one-shot input corrupted the keystream and returned wrong ciphertext. Fix this by dispatching the one-shot path by mode: - Stream CTR/CFB/OFB: route through uadk_prov_do_cipher so non-aligned input falls back to SW. - Block pad=0: direct HW one-shot, requiring block-aligned input. - Block pad=1: delegate to the new uadk_prov_cipher_soft_oneshot(), which runs EVP_Cipher with padding disabled to match the default provider one-shot semantics (outl=inl). - XTS/CTS (blksize==1, non-stream): direct HW one-shot. Also sync the caller's padding setting into the SW fallback ctx in uadk_prov_cipher_soft_update() so SW respects pad=0/1 during update/final. Signed-off-by: Zhushuai Yin <yinzhushuai@huawei.com> --- src/uadk_prov_cipher.c | 133 ++++++++++++++++++++++++++++++++--------- 1 file changed, 104 insertions(+), 29 deletions(-) diff --git a/src/uadk_prov_cipher.c b/src/uadk_prov_cipher.c index caa0cba..1990116 100644 --- a/src/uadk_prov_cipher.c +++ b/src/uadk_prov_cipher.c @@ -344,6 +344,13 @@ static int uadk_prov_cipher_soft_update(struct cipher_priv_ctx *priv, unsigned c return UADK_P_FAIL; } + /* + * Sync padding setting so SW fallback respects the caller's pad config + * (e.g. one-shot OSSL_FUNC_CIPHER_CIPHER temporarily sets pad=0 for + * raw block cipher, matching default provider behavior). + */ + EVP_CIPHER_CTX_set_padding(priv->sw_ctx, priv->pad); + if (!EVP_CipherUpdate(priv->sw_ctx, out, outl, in, len)) { UADK_ERR("cipher soft update error!\n"); return UADK_P_FAIL; @@ -373,6 +380,42 @@ static int uadk_prov_cipher_soft_final(struct cipher_priv_ctx *priv, unsigned ch return UADK_P_SUCCESS; } +/* + * Block cipher pad=1 one-shot: align with default provider one-shot + * semantics - raw encrypt/decrypt without PKCS#7 padding, outl=inl. + * HW does not handle padding; delegate to SW via EVP_Cipher one-shot + * with padding disabled to match default ossl_cipher_generic_cipher. + */ +static int uadk_prov_cipher_soft_oneshot(struct cipher_priv_ctx *priv, + unsigned char *out, size_t *outl, + const unsigned char *in, size_t inl) +{ + int len; + + if (!priv->sw_cipher) { + uadk_create_cipher_soft_ctx(priv); + if (!priv->sw_cipher) + return UADK_P_FAIL; + } + + if (!EVP_CipherInit_ex2(priv->sw_ctx, priv->sw_cipher, priv->key, + priv->iv, priv->enc, NULL)) { + UADK_ERR("cipher soft oneshot init failed.\n"); + return UADK_P_FAIL; + } + EVP_CIPHER_CTX_set_padding(priv->sw_ctx, 0); + + len = EVP_Cipher(priv->sw_ctx, out, in, inl); + if (len < 0) { + UADK_ERR("cipher soft oneshot failed.\n"); + return UADK_P_FAIL; + } + + *outl = len; + + return UADK_P_SUCCESS; +} + static int uadk_prov_cipher_dev_init(struct cipher_priv_ctx *priv); static int uadk_cipher_poll(void *ctx) @@ -877,35 +920,6 @@ static OSSL_FUNC_cipher_gettable_ctx_params_fn uadk_prov_cipher_gettable_ctx_par static OSSL_FUNC_cipher_set_ctx_params_fn uadk_prov_cipher_set_ctx_params; static OSSL_FUNC_cipher_settable_ctx_params_fn uadk_prov_cipher_settable_ctx_params; -static int uadk_prov_cipher_cipher(void *vctx, unsigned char *output, size_t *outl, - size_t outsize, const unsigned char *input, - size_t inl) -{ - struct cipher_priv_ctx *priv = (struct cipher_priv_ctx *)vctx; - int ret; - - if (!vctx || !output || !input || !outl) - return UADK_P_FAIL; - - if (inl == 0) { - *outl = 0; - return UADK_P_SUCCESS; - } - - if (outsize < inl) { - UADK_ERR("invalid: cipher outsize is too small.\n"); - return UADK_P_FAIL; - } - - ret = uadk_prov_do_cipher(priv, output, outl, outsize, input, inl); - if (ret != UADK_P_SUCCESS) - return ret; - - *outl = inl; - - return UADK_P_SUCCESS; -} - static int uadk_prov_cipher_block_encrypto(struct cipher_priv_ctx *priv, unsigned char *out, size_t *outl, size_t outsize) { @@ -1086,6 +1100,67 @@ static int uadk_prov_cipher_stream_final(void *vctx, unsigned char *out, return UADK_P_SUCCESS; } +static int uadk_prov_cipher_cipher(void *vctx, unsigned char *output, size_t *outl, + size_t outsize, const unsigned char *input, + size_t inl) +{ + struct cipher_priv_ctx *priv = (struct cipher_priv_ctx *)vctx; + int ret; + + if (!vctx || !output || !outl || (!input && inl > 0)) + return UADK_P_FAIL; + + if (inl == 0) { + *outl = 0; + return UADK_P_SUCCESS; + } + + if (outsize < inl) + return UADK_P_FAIL; + + /* + * Stream mode CTR/CFB/OFB. HW requires 16B-aligned BDs; + * non-aligned input goes entirely to SW via uadk_prov_do_cipher. + */ + if (priv->blksize == 1 && + (priv->setup.mode == WD_CIPHER_CTR || + priv->setup.mode == WD_CIPHER_CFB || + priv->setup.mode == WD_CIPHER_OFB)) + return uadk_prov_do_cipher(priv, output, outl, outsize, input, inl); + + /* + * Block mode pad=0. Requires block-aligned input; + * direct HW one-shot, no update+final needed. + */ + if (priv->blksize > 1 && !priv->pad) { + if (inl & (priv->blksize - 1)) { + UADK_ERR("invalid: cipher pad=0 needs block-aligned input.\n"); + return UADK_P_FAIL; + } + ret = uadk_prov_hw_cipher(priv, output, outl, outsize, input, inl); + if (ret == UADK_P_SUCCESS) + *outl = inl; + return ret; + } + + /* + * Block mode pad=1. Align with default provider one-shot + * semantics: SW raw encrypt/decrypt without PKCS#7 padding, outl=inl. + */ + if (priv->blksize > 1 && priv->pad) + return uadk_prov_cipher_soft_oneshot(priv, output, outl, input, inl); + + /* + * XTS/CTS (blksize==1, non-CTR/CFB/OFB). HW natively + * supports non-aligned; direct HW one-shot. + */ + ret = uadk_prov_hw_cipher(priv, output, outl, outsize, input, inl); + if (ret == UADK_P_SUCCESS) + *outl = inl; + + return ret; +} + static int uadk_prov_cipher_einit(void *vctx, const unsigned char *key, size_t keylen, const unsigned char *iv, size_t ivlen, const OSSL_PARAM params[]) -- 2.43.0
From: Zhushuai Yin <yinzhushuai@huawei.com> The HW SEC engine requires each non-last BD (Buffer Descriptor) to have 16B-aligned in_bytes for stream modes (CTR/CFB/OFB). When an application calls EVP_EncryptUpdate multiple times with non-16B-aligned chunk sizes on the same ctx, each update generates one BD; non-aligned non-last BDs corrupt the HW internal byte-level keystream state, producing wrong ciphertext from the chunk boundary onward. Fix this in uadk_prov_cipher_stream_update() by detecting stream-mode (CTR/CFB/OFB) input whose length is not a multiple of the IV/block size and routing the whole input to the SW fallback, so the HW never sees a non-aligned non-last BD. Also fix uadk_prov_cipher_soft_update() to re-init the SW ctx only on the HW->SW transition (switch_flag != UADK_DO_SOFT) instead of on every call. Re-initing every call reset the IV to priv->iv, causing counter/IV reuse across consecutive SW updates and further ciphertext corruption; preserving the SW state keeps the counter/IV chain continuous. Signed-off-by: Zhushuai Yin <yinzhushuai@huawei.com> --- src/uadk_prov_cipher.c | 44 ++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/src/uadk_prov_cipher.c b/src/uadk_prov_cipher.c index 1990116..b6c1dbb 100644 --- a/src/uadk_prov_cipher.c +++ b/src/uadk_prov_cipher.c @@ -338,18 +338,25 @@ static int uadk_prov_cipher_soft_update(struct cipher_priv_ctx *priv, unsigned c if (!priv->sw_cipher) return UADK_P_FAIL; - if (!EVP_CipherInit_ex2(priv->sw_ctx, priv->sw_cipher, priv->key, priv->iv, - priv->enc, NULL)) { - UADK_ERR("cipher soft init error!\n"); - return UADK_P_FAIL; - } - /* - * Sync padding setting so SW fallback respects the caller's pad config - * (e.g. one-shot OSSL_FUNC_CIPHER_CIPHER temporarily sets pad=0 for - * raw block cipher, matching default provider behavior). + * Re-init SW ctx only on HW→SW transition or first SW call. Consecutive + * SW calls preserve state (counter/IV chain); re-initing every call + * resets IV to priv->iv, causing counter/IV reuse. */ - EVP_CIPHER_CTX_set_padding(priv->sw_ctx, priv->pad); + if (priv->switch_flag != UADK_DO_SOFT) { + if (!EVP_CipherInit_ex2(priv->sw_ctx, priv->sw_cipher, priv->key, priv->iv, + priv->enc, NULL)) { + UADK_ERR("cipher soft init error!\n"); + return UADK_P_FAIL; + } + + /* + * Sync padding setting so SW fallback respects the caller's pad config + * (e.g. one-shot OSSL_FUNC_CIPHER_CIPHER temporarily sets pad=0 for + * raw block cipher, matching default provider behavior). + */ + EVP_CIPHER_CTX_set_padding(priv->sw_ctx, priv->pad); + } if (!EVP_CipherUpdate(priv->sw_ctx, out, outl, in, len)) { UADK_ERR("cipher soft update error!\n"); @@ -1061,6 +1068,23 @@ static int uadk_prov_cipher_stream_update(void *vctx, unsigned char *output, goto do_soft; } + /* + * Stream mode non-aligned fallback: HW requires 16B-aligned BDs for + * multi-BD stream continuity in CTR/CFB/OFB. Non-aligned input would + * corrupt the HW keystream state. Switch the whole input to SW. + */ + if ((priv->setup.mode == WD_CIPHER_CTR || + priv->setup.mode == WD_CIPHER_CFB || + priv->setup.mode == WD_CIPHER_OFB) && + priv->ivlen && (inl & (priv->ivlen - 1))) { + if (!priv->sw_cipher) + uadk_create_cipher_soft_ctx(priv); + if (!priv->sw_cipher) + goto hw_path; + goto do_soft; + } + +hw_path: ret = uadk_prov_hw_cipher(priv, output, outl, outsize, input, inl); if (ret != UADK_P_SUCCESS) { if (priv->sw_cipher) -- 2.43.0
From: Zhushuai Yin <yinzhushuai@huawei.com> The HW SEC engine requires each non-last BD (Buffer Descriptor) to have 16B-aligned in_bytes for stream modes (CTR/CFB/OFB). When an application calls EVP_EncryptUpdate multiple times with non-16B-aligned chunk sizes on the same ctx, each update generates one BD; non-aligned non-last BDs corrupt the HW internal byte-level keystream state, producing wrong ciphertext from the chunk boundary onward. Fix this in uadk_e_do_cipher() by checking uadk_e_is_stream_mode() and routing non-aligned stream-mode input (inlen & STREAM_BLOCK_MASK) to the SW fallback, so the HW never sees a non-aligned non-last BD. After a successful SW fallback for stream modes, set switch_flag = UADK_DO_SOFT so subsequent updates keep using the SW path. SW advances the keystream in sw_ctx_data rather than priv->iv, so re-engaging HW with the stale priv->iv would desync the keystream. Non-stream modes are stateless per call and are unaffected. To keep the HW path readable, extract it into uadk_e_hw_cipher(). Signed-off-by: Zhushuai Yin <yinzhushuai@huawei.com> --- src/uadk_cipher.c | 74 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 21 deletions(-) diff --git a/src/uadk_cipher.c b/src/uadk_cipher.c index deceaee..bbe6e19 100644 --- a/src/uadk_cipher.c +++ b/src/uadk_cipher.c @@ -35,6 +35,7 @@ #define CTX_ASYNC_DEC 3 #define CTX_NUM 4 #define IV_LEN 16 +#define STREAM_BLOCK_MASK 0xF #define ENV_ENABLED 1 #define MAX_KEY_LEN 64 #define SMALL_PACKET_OFFLOAD_THRESHOLD_DEFAULT 192 @@ -699,12 +700,45 @@ static void uadk_e_ctx_init(EVP_CIPHER_CTX *ctx, struct cipher_priv_ctx *priv) } } +static bool uadk_e_is_stream_mode(struct cipher_priv_ctx *priv) +{ + return priv->setup.mode == WD_CIPHER_CTR || + priv->setup.mode == WD_CIPHER_CFB || + priv->setup.mode == WD_CIPHER_OFB; +} + +static int uadk_e_hw_cipher(EVP_CIPHER_CTX *ctx, struct cipher_priv_ctx *priv) +{ + struct async_op op; + int ret; + + uadk_e_ctx_init(ctx, priv); + if (unlikely(priv->switch_flag == UADK_DO_SOFT)) { + fprintf(stderr, "switch to soft cipher.\n"); + return 0; + } + + ret = async_setup_async_event_notification(&op); + if (!ret) { + fprintf(stderr, "failed to setup async event notification.\n"); + return 0; + } + + if (!op.job) + ret = do_cipher_sync(priv); + else + ret = do_cipher_async(priv, &op); + + (void)async_clear_async_event_notification(); + + return ret; +} + static int uadk_e_do_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out, const unsigned char *in, size_t inlen) { struct cipher_priv_ctx *priv = (struct cipher_priv_ctx *)EVP_CIPHER_CTX_get_cipher_data(ctx); - struct async_op op; int ret; if (unlikely(!priv)) { @@ -725,36 +759,34 @@ static int uadk_e_do_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out, if (priv->req.in_bytes <= priv->switch_threshold) goto out_soft; - uadk_e_ctx_init(ctx, priv); - if (unlikely(priv->switch_flag == UADK_DO_SOFT)) { - fprintf(stderr, "switch to soft cipher.\n"); + /* + * Stream mode non-aligned fallback: HW requires 16B-aligned BDs for + * non-last BD in CTR/CFB/OFB. Non-aligned input would corrupt the + * HW internal keystream state. Switch the whole input to SW. + */ + if (uadk_e_is_stream_mode(priv) && priv->sw_cipher && + (inlen & STREAM_BLOCK_MASK)) goto out_soft; - } - ret = async_setup_async_event_notification(&op); - if (!ret) { - fprintf(stderr, "failed to setup async event notification.\n"); + ret = uadk_e_hw_cipher(ctx, priv); + if (!ret) goto out_soft; - } - - if (!op.job) { - ret = do_cipher_sync(priv); - if (!ret) - goto out_notify; - } else { - ret = do_cipher_async(priv, &op); - if (!ret) - goto out_notify; - } return 1; -out_notify: - (void)async_clear_async_event_notification(); out_soft: ret = uadk_e_cipher_soft_work(ctx, out, in, inlen); if (ret != 1) fprintf(stderr, "do soft ciphers failed.\n"); + + /* + * Stream modes: SW advances keystream in sw_ctx_data, not priv->iv. + * Set switch_flag=SOFT to prevent HW re-engaging with stale iv. + * Non-stream modes are stateless per call, unaffected. + */ + if (ret == 1 && uadk_e_is_stream_mode(priv)) + priv->switch_flag = UADK_DO_SOFT; + return ret; } -- 2.43.0
participants (1)
-
Weili Qian