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

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

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




>  			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