> -----Original Message-----
> From: Power, Ciara
> Sent: Friday, November 3, 2023 10:18 AM
> To: Gowrishankar Muthukrishnan <gmuthukri...@marvell.com>;
> dev@dpdk.org
> Cc: ano...@marvell.com; Akhil Goyal <gak...@marvell.com>; Ji, Kai
> <kai...@intel.com>
> Subject: RE: [PATCH v2] crypto/openssl: fix memory leaks in asym ops
>
> 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
>
>
Quickly looked into this - seem i2d_ECDSA_SIG changes the pointer passed in, so
signbuf no longer points to the allocated memory afterwards.
Temp pointer is needed here, something like:
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -2814,7 +2814,7 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
case RTE_CRYPTO_ASYM_OP_VERIFY:
{
BIGNUM *r = NULL, *s = NULL;
- unsigned char *signbuf;
+ unsigned char *signbuf, *signbuf_tmp= NULL;
size_t signlen;
kctx = EVP_PKEY_CTX_new_from_name(NULL, "SM2", NULL);
@@ -2869,7 +2869,8 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
signlen = i2d_ECDSA_SIG(ec_sign, 0);
signbuf = rte_malloc(NULL, signlen, 0);
- signlen = i2d_ECDSA_SIG(ec_sign, &signbuf);
+ signbuf_tmp = signbuf;
+ signlen = i2d_ECDSA_SIG(ec_sign, &signbuf_tmp);
if (signlen <= 0) {
rte_free(signbuf);
goto err_sm2;
Thanks,
Ciara
>
>
> > 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
>
>
