Hi Gowrishankar, > -----Original Message----- > From: Gowrishankar Muthukrishnan <gmuthukri...@marvell.com> > Sent: Thursday, November 2, 2023 10:04 AM > To: dev@dpdk.org > Cc: ano...@marvell.com; Akhil Goyal <gak...@marvell.com>; Ji, Kai > <kai...@intel.com>; Power, Ciara <ciara.po...@intel.com>; Gowrishankar > Muthukrishnan <gmuthukri...@marvell.com> > Subject: [PATCH v2] crypto/openssl: fix memory leaks in asym ops > > Fix memory leaks in Asymmetric ops, as reported by valgrind. > > Signed-off-by: Gowrishankar Muthukrishnan <gmuthukri...@marvell.com> > --- > v2: > - added more fixes. > --- > drivers/crypto/openssl/rte_openssl_pmd.c | 38 ++++++++++++++------ > drivers/crypto/openssl/rte_openssl_pmd_ops.c | 15 ++++++-- > 2 files changed, 39 insertions(+), 14 deletions(-) > <snip> > case RTE_CRYPTO_ASYM_OP_VERIFY: > { > - unsigned char signbuf[128] = {0}; > BIGNUM *r = NULL, *s = NULL; > - EVP_MD_CTX *md_ctx = NULL; > - ECDSA_SIG *ec_sign; > - EVP_MD *check_md; > + unsigned char *signbuf; > size_t signlen; > > kctx = EVP_PKEY_CTX_new_from_name(NULL, "SM2", > NULL); @@ -2857,13 +2862,18 @@ process_openssl_sm2_op_evp(struct > rte_crypto_op *cop, > r = NULL; > s = NULL; > > - signlen = i2d_ECDSA_SIG(ec_sign, (unsigned char > **)&signbuf); > - if (signlen <= 0) > + signlen = i2d_ECDSA_SIG(ec_sign, 0); > + signbuf = rte_malloc(NULL, signlen, 0); > + signlen = i2d_ECDSA_SIG(ec_sign, &signbuf); > + if (signlen <= 0) { > + rte_free(signbuf); > goto err_sm2; > + } > > if (!EVP_DigestVerifyFinal(md_ctx, signbuf, signlen)) > goto err_sm2; > > + rte_free(signbuf);
I am seeing some issues with this line: ==1788670==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f78bfe4d337 at pc 0x55bd318866c2 bp 0x7ffc91e02420 sp 0x7ffc91e02410 READ of size 1 at 0x7f78bfe4d337 thread T0 #0 0x55bd318866c1 in malloc_elem_from_data ../lib/eal/common/malloc_elem.h:315 #1 0x55bd31886bc7 in mem_free ../lib/eal/common/rte_malloc.c:37 #2 0x55bd31886c6c in rte_free ../lib/eal/common/rte_malloc.c:44 #3 0x55bd37795665 in process_openssl_sm2_op_evp ../drivers/crypto/openssl/rte_openssl_pmd.c:2890 #4 0x55bd37795c7b in process_asym_op ../drivers/crypto/openssl/rte_openssl_pmd.c:3088 #5 0x55bd377ac886 in openssl_pmd_enqueue_burst ../drivers/crypto/openssl/rte_openssl_pmd.c:3213 #6 0x55bd3011788a in rte_cryptodev_enqueue_burst ../lib/cryptodev/rte_cryptodev.h:2038 #7 0x55bd30125331 in test_sm2_sign ../app/test/test_cryptodev_asym.c:1976 Address 0x7f78bfe4d337 is a wild pointer. SUMMARY: AddressSanitizer: heap-buffer-overflow ../lib/eal/common/malloc_elem.h:315 in malloc_elem_from_data > BN_free(r); > BN_free(s); > ECDSA_SIG_free(ec_sign); > @@ -2880,6 +2890,12 @@ process_openssl_sm2_op_evp(struct > rte_crypto_op *cop, > ret = 0; > cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS; > err_sm2: > + if (check_md) > + EVP_MD_free(check_md); > + > + if (md_ctx) > + EVP_MD_CTX_free(md_ctx); > + > if (kctx) > EVP_PKEY_CTX_free(kctx); > > diff --git a/drivers/crypto/openssl/rte_openssl_pmd_ops.c > b/drivers/crypto/openssl/rte_openssl_pmd_ops.c > index 2862c294a9..98450f36cf 100644 > --- a/drivers/crypto/openssl/rte_openssl_pmd_ops.c > +++ b/drivers/crypto/openssl/rte_openssl_pmd_ops.c > @@ -958,9 +958,11 @@ static int openssl_set_asym_session_parameters( > rsa_ctx = EVP_PKEY_CTX_new(pkey, NULL); > asym_session->xfrm_type = > RTE_CRYPTO_ASYM_XFORM_RSA; > asym_session->u.r.ctx = rsa_ctx; > + EVP_PKEY_free(pkey); > EVP_PKEY_CTX_free(key_ctx); > + OSSL_PARAM_BLD_free(param_bld); > OSSL_PARAM_free(params); > - break; > + ret = 0; > #else > RSA *rsa = RSA_new(); > if (rsa == NULL) > @@ -1030,7 +1032,7 @@ static int openssl_set_asym_session_parameters( > } > asym_session->u.r.rsa = rsa; > asym_session->xfrm_type = > RTE_CRYPTO_ASYM_XFORM_RSA; > - break; > + ret = 0; > #endif > err_rsa: > BN_clear_free(n); > @@ -1042,7 +1044,7 @@ static int openssl_set_asym_session_parameters( > BN_clear_free(dmq1); > BN_clear_free(iqmp); > > - return -1; > + return ret; > } > case RTE_CRYPTO_ASYM_XFORM_MODEX: > { > @@ -1228,6 +1230,7 @@ static int openssl_set_asym_session_parameters( > } > asym_session->xfrm_type = > RTE_CRYPTO_ASYM_XFORM_DSA; > asym_session->u.s.param_bld = param_bld; > + BN_free(pub_key); This pub_key doesn't seem to be used in this " case RTE_CRYPTO_ASYM_XFORM_DSA:" Could we just remove it completely? In addition to the fixes here, I have more ASAN fixes that showed up for me. Will send that patch, and all issues should then be fixed between our two patches. Thanks, Ciara