[v2] crypto/openssl: fix memory leaks in asym ops
Checks
Commit Message
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(-)
Comments
On Thu, 2 Nov 2023 14:08:31 +0530
Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com> wrote:
> + if (pub_key)
> + BN_free(pub_key);
> + if (pkey)
> + EVP_PKEY_free(pkey);
>
All these checks for null are unnecessary:
EVP_PKEY_free() decrements the reference count of key and, if the reference count is zero,
frees it up. If key is NULL, nothing is done.
Let me add those functions to cocci nullfree script as well.
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
> -----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
>
>
> All these checks for null are unnecessary:
>
> EVP_PKEY_free() decrements the reference count of key and, if the
> reference count is zero,
> frees it up. If key is NULL, nothing is done.
>
>
> Let me add those functions to cocci nullfree script as well.
Ack.
@@ -1897,6 +1897,7 @@ process_openssl_dsa_sign_op_evp(struct rte_crypto_op *cop,
size_t outlen;
unsigned char *dsa_sign_data;
const unsigned char *dsa_sign_data_p;
+ int ret = -1;
cop->status = RTE_CRYPTO_OP_STATUS_ERROR;
params = OSSL_PARAM_BLD_to_param(param_bld);
@@ -1950,9 +1951,9 @@ process_openssl_dsa_sign_op_evp(struct rte_crypto_op *cop,
cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
}
+ ret = 0;
DSA_SIG_free(sign);
OPENSSL_free(dsa_sign_data);
- return 0;
err_dsa_sign:
if (params)
@@ -1961,7 +1962,9 @@ process_openssl_dsa_sign_op_evp(struct rte_crypto_op *cop,
EVP_PKEY_CTX_free(key_ctx);
if (dsa_ctx)
EVP_PKEY_CTX_free(dsa_ctx);
- return -1;
+ if (pkey)
+ EVP_PKEY_free(pkey);
+ return ret;
}
/* process dsa verify operation */
@@ -2034,6 +2037,7 @@ process_openssl_dsa_verify_op_evp(struct rte_crypto_op *cop,
ret = 0;
}
+ OPENSSL_free(dsa_sig);
err_dsa_verify:
if (sign)
DSA_SIG_free(sign);
@@ -2043,6 +2047,10 @@ process_openssl_dsa_verify_op_evp(struct rte_crypto_op *cop,
EVP_PKEY_CTX_free(key_ctx);
if (dsa_ctx)
EVP_PKEY_CTX_free(dsa_ctx);
+ if (pub_key)
+ BN_free(pub_key);
+ if (pkey)
+ EVP_PKEY_free(pkey);
return ret;
}
@@ -2674,6 +2682,9 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
EVP_PKEY_CTX *kctx = NULL, *sctx = NULL, *cctx = NULL;
struct rte_crypto_asym_op *op = cop->asym;
OSSL_PARAM *params = sess->u.sm2.params;
+ EVP_MD_CTX *md_ctx = NULL;
+ ECDSA_SIG *ec_sign = NULL;
+ EVP_MD *check_md = NULL;
EVP_PKEY *pkey = NULL;
int ret = -1;
@@ -2739,10 +2750,7 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
{
unsigned char signbuf[128] = {0};
const unsigned char *signptr;
- EVP_MD_CTX *md_ctx = NULL;
const BIGNUM *r, *s;
- ECDSA_SIG *ec_sign;
- EVP_MD *check_md;
size_t signlen;
kctx = EVP_PKEY_CTX_new_from_name(NULL, "SM2", NULL);
@@ -2800,11 +2808,8 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
break;
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);
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);
@@ -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);
break;
#else
@@ -1363,6 +1366,7 @@ static int openssl_set_asym_session_parameters(
asym_session->u.sm2.params = params;
OSSL_PARAM_BLD_free(param_bld);
+ BN_free(pkey_bn);
asym_session->xfrm_type = RTE_CRYPTO_ASYM_XFORM_SM2;
break;
@@ -1373,6 +1377,8 @@ static int openssl_set_asym_session_parameters(
if (asym_session->u.sm2.params)
OSSL_PARAM_free(asym_session->u.sm2.params);
+ if (pkey_bn)
+ BN_free(pkey_bn);
return -1;
#else
OPENSSL_LOG(WARNING, "SM2 unsupported in current OpenSSL Version");
@@ -1452,6 +1458,8 @@ static void openssl_reset_asym_session(struct openssl_asym_session *sess)
break;
case RTE_CRYPTO_ASYM_XFORM_DH:
#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
+ OSSL_PARAM_BLD_free(sess->u.dh.param_bld);
+ OSSL_PARAM_BLD_free(sess->u.dh.param_bld_peer);
sess->u.dh.param_bld = NULL;
sess->u.dh.param_bld_peer = NULL;
#else
@@ -1461,6 +1469,7 @@ static void openssl_reset_asym_session(struct openssl_asym_session *sess)
break;
case RTE_CRYPTO_ASYM_XFORM_DSA:
#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
+ OSSL_PARAM_BLD_free(sess->u.s.param_bld);
sess->u.s.param_bld = NULL;
#else
if (sess->u.s.dsa)