RE: [PATCH v2] crypto/openssl: fix memory leaks in asym ops

["Power, Ciara" <ciara.power@intel.com>]

> -----Original Message-----
> From: Power, Ciara
> Sent: Friday, November 3, 2023 10:18 AM
> To: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>;
> dev@dpdk.org
> Cc: anoobj@marvell.com; Akhil Goyal <gakhil@marvell.com>; Ji, Kai
> <kai.ji@intel.com>
> Subject: RE: [PATCH v2] crypto/openssl: fix memory leaks in asym ops
> 
> Hi Gowrishankar,
> 
> > -----Original Message-----
> > From: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
> > Sent: Thursday, November 2, 2023 10:04 AM
> > To: dev@dpdk.org
> > Cc: anoobj@marvell.com; Akhil Goyal <gakhil@marvell.com>; Ji, Kai
> > <kai.ji@intel.com>; Power, Ciara <ciara.power@intel.com>; Gowrishankar
> > Muthukrishnan <gmuthukrishn@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 <gmuthukrishn@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
> 
>