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