Hi Gowrishankar,
> -----Original Message-----
> From: Gowrishankar Muthukrishnan <[email protected]>
> Sent: Thursday, November 2, 2023 10:04 AM
> To: [email protected]
> Cc: [email protected]; Akhil Goyal <[email protected]>; Ji, Kai
> <[email protected]>; Power, Ciara <[email protected]>; Gowrishankar
> Muthukrishnan <[email protected]>
> 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 <[email protected]>
> ---
> 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
