* [PATCH] crypto/openssl: fix memory leaks in SM2 ops
@ 2023-09-19 13:04 Gowrishankar Muthukrishnan
2023-10-23 13:33 ` Akhil Goyal
2023-11-02 8:38 ` [PATCH v2] crypto/openssl: fix memory leaks in asym ops Gowrishankar Muthukrishnan
0 siblings, 2 replies; 13+ messages in thread
From: Gowrishankar Muthukrishnan @ 2023-09-19 13:04 UTC (permalink / raw)
To: dev; +Cc: anoobj, Akhil Goyal, Kai Ji, Gowrishankar Muthukrishnan
Fix memory leaks in SM2 ops, as reported by valgrind.
Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
---
drivers/crypto/openssl/rte_openssl_pmd.c | 45 ++++++++++++++----------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index 5e8624cebe..c69889d522 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -2674,10 +2674,13 @@ 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_BLD *param_bld = NULL;
+ ECDSA_SIG *ec_sign = NULL;
+ EVP_MD_CTX *md_ctx = NULL;
OSSL_PARAM *params = NULL;
+ EVP_MD *check_md = NULL;
EVP_PKEY *pkey = NULL;
BIGNUM *pkey_bn = NULL;
- uint8_t pubkey[64];
+ uint8_t pubkey[65];
size_t len = 0;
int ret = -1;
@@ -2787,10 +2790,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);
@@ -2842,17 +2842,12 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
op->sm2.s.length = BN_num_bytes(s);
BN_bn2bin(r, op->sm2.r.data);
BN_bn2bin(s, op->sm2.s.data);
-
- ECDSA_SIG_free(ec_sign);
}
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);
@@ -2902,19 +2897,16 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
goto err_sm2;
}
- 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;
-
- BN_free(r);
- BN_free(s);
- ECDSA_SIG_free(ec_sign);
}
break;
default:
@@ -2928,6 +2920,15 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
ret = 0;
cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
err_sm2:
+ if (ec_sign)
+ ECDSA_SIG_free(ec_sign);
+
+ if (check_md)
+ EVP_MD_free(check_md);
+
+ if (md_ctx)
+ EVP_MD_CTX_free(md_ctx);
+
if (kctx)
EVP_PKEY_CTX_free(kctx);
@@ -2943,6 +2944,12 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
if (param_bld)
OSSL_PARAM_BLD_free(param_bld);
+ if (params)
+ OSSL_PARAM_free(params);
+
+ if (pkey_bn)
+ BN_free(pkey_bn);
+
return ret;
}
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] crypto/openssl: fix memory leaks in SM2 ops
2023-09-19 13:04 [PATCH] crypto/openssl: fix memory leaks in SM2 ops Gowrishankar Muthukrishnan
@ 2023-10-23 13:33 ` Akhil Goyal
2023-11-02 8:38 ` [PATCH v2] crypto/openssl: fix memory leaks in asym ops Gowrishankar Muthukrishnan
1 sibling, 0 replies; 13+ messages in thread
From: Akhil Goyal @ 2023-10-23 13:33 UTC (permalink / raw)
To: Gowrishankar Muthukrishnan, dev
Cc: Anoob Joseph, Kai Ji, Gowrishankar Muthukrishnan
> Subject: [PATCH] crypto/openssl: fix memory leaks in SM2 ops
>
> Fix memory leaks in SM2 ops, as reported by valgrind.
>
> Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
> ---
> drivers/crypto/openssl/rte_openssl_pmd.c | 45 ++++++++++++++----------
> 1 file changed, 26 insertions(+), 19 deletions(-)
Please rebase this patch. It no longer applies cleanly on top of tree.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] crypto/openssl: fix memory leaks in asym ops
2023-09-19 13:04 [PATCH] crypto/openssl: fix memory leaks in SM2 ops Gowrishankar Muthukrishnan
2023-10-23 13:33 ` Akhil Goyal
@ 2023-11-02 8:38 ` Gowrishankar Muthukrishnan
2023-11-02 10:03 ` Gowrishankar Muthukrishnan
` (4 more replies)
1 sibling, 5 replies; 13+ messages in thread
From: Gowrishankar Muthukrishnan @ 2023-11-02 8:38 UTC (permalink / raw)
To: dev; +Cc: anoobj, Akhil Goyal, Kai Ji, Ciara Power, Gowrishankar Muthukrishnan
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(-)
diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index c234882417..5961457279 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -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);
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);
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)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] crypto/openssl: fix memory leaks in asym ops
2023-11-02 8:38 ` [PATCH v2] crypto/openssl: fix memory leaks in asym ops Gowrishankar Muthukrishnan
@ 2023-11-02 10:03 ` Gowrishankar Muthukrishnan
2023-11-02 22:47 ` Stephen Hemminger
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Gowrishankar Muthukrishnan @ 2023-11-02 10:03 UTC (permalink / raw)
To: dev; +Cc: anoobj, Akhil Goyal, Kai Ji, Ciara Power, Gowrishankar Muthukrishnan
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(-)
diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index c234882417..5961457279 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -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);
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);
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)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] crypto/openssl: fix memory leaks in asym ops
2023-11-02 8:38 ` [PATCH v2] crypto/openssl: fix memory leaks in asym ops Gowrishankar Muthukrishnan
2023-11-02 10:03 ` Gowrishankar Muthukrishnan
@ 2023-11-02 22:47 ` Stephen Hemminger
2023-11-03 15:19 ` [EXT] " Gowrishankar Muthukrishnan
2023-11-03 10:18 ` Power, Ciara
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Stephen Hemminger @ 2023-11-02 22:47 UTC (permalink / raw)
To: Gowrishankar Muthukrishnan; +Cc: dev, anoobj, Akhil Goyal, Kai Ji, Ciara Power
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2] crypto/openssl: fix memory leaks in asym ops
2023-11-02 8:38 ` [PATCH v2] crypto/openssl: fix memory leaks in asym ops Gowrishankar Muthukrishnan
2023-11-02 10:03 ` Gowrishankar Muthukrishnan
2023-11-02 22:47 ` Stephen Hemminger
@ 2023-11-03 10:18 ` Power, Ciara
2023-11-03 11:38 ` Power, Ciara
2023-11-03 15:15 ` [PATCH v3] " Gowrishankar Muthukrishnan
4 siblings, 0 replies; 13+ messages in thread
From: Power, Ciara @ 2023-11-03 10:18 UTC (permalink / raw)
To: Gowrishankar Muthukrishnan, dev; +Cc: anoobj, Akhil Goyal, Ji, Kai
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2] crypto/openssl: fix memory leaks in asym ops
2023-11-02 8:38 ` [PATCH v2] crypto/openssl: fix memory leaks in asym ops Gowrishankar Muthukrishnan
` (2 preceding siblings ...)
2023-11-03 10:18 ` Power, Ciara
@ 2023-11-03 11:38 ` Power, Ciara
2023-11-03 15:15 ` [PATCH v3] " Gowrishankar Muthukrishnan
4 siblings, 0 replies; 13+ messages in thread
From: Power, Ciara @ 2023-11-03 11:38 UTC (permalink / raw)
To: Gowrishankar Muthukrishnan, dev; +Cc: anoobj, Akhil Goyal, Ji, Kai
> -----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
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] crypto/openssl: fix memory leaks in asym ops
2023-11-02 8:38 ` [PATCH v2] crypto/openssl: fix memory leaks in asym ops Gowrishankar Muthukrishnan
` (3 preceding siblings ...)
2023-11-03 11:38 ` Power, Ciara
@ 2023-11-03 15:15 ` Gowrishankar Muthukrishnan
2023-11-03 15:39 ` Power, Ciara
` (2 more replies)
4 siblings, 3 replies; 13+ messages in thread
From: Gowrishankar Muthukrishnan @ 2023-11-03 15:15 UTC (permalink / raw)
To: dev; +Cc: anoobj, Akhil Goyal, Kai Ji, Ciara Power, Gowrishankar Muthukrishnan
Fix memory leaks in Asymmetric ops, as reported by valgrind.
Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
---
v3:
- changes as suggested in v2.
---
drivers/crypto/openssl/rte_openssl_pmd.c | 30 +++++++++++++-------
drivers/crypto/openssl/rte_openssl_pmd_ops.c | 16 +++++++----
2 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index c234882417..6fb827b600 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -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;
+
+ 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);
@@ -2044,6 +2048,9 @@ process_openssl_dsa_verify_op_evp(struct rte_crypto_op *cop,
if (dsa_ctx)
EVP_PKEY_CTX_free(dsa_ctx);
+ BN_free(pub_key);
+ EVP_PKEY_free(pkey);
+
return ret;
}
#else
@@ -2674,6 +2681,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 +2749,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 +2807,8 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
break;
case RTE_CRYPTO_ASYM_OP_VERIFY:
{
- unsigned char signbuf[128] = {0};
+ unsigned char signbuf[128] = {0}, *signbuf_new = NULL;
BIGNUM *r = NULL, *s = NULL;
- EVP_MD_CTX *md_ctx = NULL;
- ECDSA_SIG *ec_sign;
- EVP_MD *check_md;
size_t signlen;
kctx = EVP_PKEY_CTX_new_from_name(NULL, "SM2", NULL);
@@ -2857,11 +2861,12 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
r = NULL;
s = NULL;
- signlen = i2d_ECDSA_SIG(ec_sign, (unsigned char **)&signbuf);
+ signbuf_new = signbuf;
+ signlen = i2d_ECDSA_SIG(ec_sign, (unsigned char **)&signbuf_new);
if (signlen <= 0)
goto err_sm2;
- if (!EVP_DigestVerifyFinal(md_ctx, signbuf, signlen))
+ if (!EVP_DigestVerifyFinal(md_ctx, signbuf_new, signlen))
goto err_sm2;
BN_free(r);
@@ -2880,6 +2885,9 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
ret = 0;
cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
err_sm2:
+ EVP_MD_free(check_md);
+ 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..bef7671424 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:
{
@@ -1184,8 +1186,7 @@ static int openssl_set_asym_session_parameters(
#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
BIGNUM *p = NULL, *g = NULL;
BIGNUM *q = NULL, *priv_key = NULL;
- BIGNUM *pub_key = BN_new();
- BN_zero(pub_key);
+ BIGNUM *pub_key = NULL;
OSSL_PARAM_BLD *param_bld = NULL;
p = BN_bin2bn((const unsigned char *)
@@ -1363,6 +1364,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 +1375,7 @@ static int openssl_set_asym_session_parameters(
if (asym_session->u.sm2.params)
OSSL_PARAM_free(asym_session->u.sm2.params);
+ BN_free(pkey_bn);
return -1;
#else
OPENSSL_LOG(WARNING, "SM2 unsupported in current OpenSSL Version");
@@ -1452,6 +1455,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 +1466,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)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [EXT] Re: [PATCH v2] crypto/openssl: fix memory leaks in asym ops
2023-11-02 22:47 ` Stephen Hemminger
@ 2023-11-03 15:19 ` Gowrishankar Muthukrishnan
0 siblings, 0 replies; 13+ messages in thread
From: Gowrishankar Muthukrishnan @ 2023-11-03 15:19 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, Anoob Joseph, Akhil Goyal, Kai Ji, Ciara Power
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3] crypto/openssl: fix memory leaks in asym ops
2023-11-03 15:15 ` [PATCH v3] " Gowrishankar Muthukrishnan
@ 2023-11-03 15:39 ` Power, Ciara
2023-11-09 20:18 ` Akhil Goyal
2023-11-13 5:41 ` [PATCH v4] " Gowrishankar Muthukrishnan
2 siblings, 0 replies; 13+ messages in thread
From: Power, Ciara @ 2023-11-03 15:39 UTC (permalink / raw)
To: Gowrishankar Muthukrishnan, dev; +Cc: anoobj, Akhil Goyal, Ji, Kai
> -----Original Message-----
> From: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
> Sent: Friday, November 3, 2023 3:15 PM
> 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 v3] 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>
> ---
> v3:
> - changes as suggested in v2.
> ---
> drivers/crypto/openssl/rte_openssl_pmd.c | 30 +++++++++++++-------
> drivers/crypto/openssl/rte_openssl_pmd_ops.c | 16 +++++++----
> 2 files changed, 30 insertions(+), 16 deletions(-)
Acked-by: Ciara Power <ciara.power@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v3] crypto/openssl: fix memory leaks in asym ops
2023-11-03 15:15 ` [PATCH v3] " Gowrishankar Muthukrishnan
2023-11-03 15:39 ` Power, Ciara
@ 2023-11-09 20:18 ` Akhil Goyal
2023-11-13 5:41 ` [PATCH v4] " Gowrishankar Muthukrishnan
2 siblings, 0 replies; 13+ messages in thread
From: Akhil Goyal @ 2023-11-09 20:18 UTC (permalink / raw)
To: Gowrishankar Muthukrishnan, dev
Cc: Anoob Joseph, Kai Ji, Ciara Power, Gowrishankar Muthukrishnan
> Subject: [PATCH v3] 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>
> ---
> v3:
> - changes as suggested in v2.
The patch conflicts with Stephen's patch to remove the null checks before free.
Please rebase. Stephen's patch is merged.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4] crypto/openssl: fix memory leaks in asym ops
2023-11-03 15:15 ` [PATCH v3] " Gowrishankar Muthukrishnan
2023-11-03 15:39 ` Power, Ciara
2023-11-09 20:18 ` Akhil Goyal
@ 2023-11-13 5:41 ` Gowrishankar Muthukrishnan
2023-11-13 6:23 ` Akhil Goyal
2 siblings, 1 reply; 13+ messages in thread
From: Gowrishankar Muthukrishnan @ 2023-11-13 5:41 UTC (permalink / raw)
To: dev; +Cc: anoobj, Akhil Goyal, Kai Ji, Ciara Power, Gowrishankar Muthukrishnan
Fix memory leaks in Asymmetric ops, as reported by valgrind.
Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
---
v4:
- patch rebased.
---
drivers/crypto/openssl/rte_openssl_pmd.c | 28 ++++++++++++--------
drivers/crypto/openssl/rte_openssl_pmd_ops.c | 16 +++++++----
2 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c b/drivers/crypto/openssl/rte_openssl_pmd.c
index 090320602d..9d463520ff 100644
--- a/drivers/crypto/openssl/rte_openssl_pmd.c
+++ b/drivers/crypto/openssl/rte_openssl_pmd.c
@@ -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,16 +1951,16 @@ 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)
OSSL_PARAM_free(params);
EVP_PKEY_CTX_free(key_ctx);
EVP_PKEY_CTX_free(dsa_ctx);
- return -1;
+ return ret;
}
/* process dsa verify operation */
@@ -2032,6 +2033,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);
@@ -2040,6 +2042,9 @@ process_openssl_dsa_verify_op_evp(struct rte_crypto_op *cop,
EVP_PKEY_CTX_free(key_ctx);
EVP_PKEY_CTX_free(dsa_ctx);
+ BN_free(pub_key);
+ EVP_PKEY_free(pkey);
+
return ret;
}
#else
@@ -2666,6 +2671,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;
@@ -2731,10 +2739,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);
@@ -2792,11 +2797,8 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
break;
case RTE_CRYPTO_ASYM_OP_VERIFY:
{
- unsigned char signbuf[128] = {0};
+ unsigned char signbuf[128] = {0}, *signbuf_new = NULL;
BIGNUM *r = NULL, *s = NULL;
- EVP_MD_CTX *md_ctx = NULL;
- ECDSA_SIG *ec_sign;
- EVP_MD *check_md;
size_t signlen;
kctx = EVP_PKEY_CTX_new_from_name(NULL, "SM2", NULL);
@@ -2849,11 +2851,12 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
r = NULL;
s = NULL;
- signlen = i2d_ECDSA_SIG(ec_sign, (unsigned char **)&signbuf);
+ signbuf_new = signbuf;
+ signlen = i2d_ECDSA_SIG(ec_sign, (unsigned char **)&signbuf_new);
if (signlen <= 0)
goto err_sm2;
- if (!EVP_DigestVerifyFinal(md_ctx, signbuf, signlen))
+ if (!EVP_DigestVerifyFinal(md_ctx, signbuf_new, signlen))
goto err_sm2;
BN_free(r);
@@ -2872,6 +2875,9 @@ process_openssl_sm2_op_evp(struct rte_crypto_op *cop,
ret = 0;
cop->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
err_sm2:
+ EVP_MD_free(check_md);
+ EVP_MD_CTX_free(md_ctx);
+
EVP_PKEY_CTX_free(kctx);
EVP_PKEY_CTX_free(sctx);
diff --git a/drivers/crypto/openssl/rte_openssl_pmd_ops.c b/drivers/crypto/openssl/rte_openssl_pmd_ops.c
index 419a767817..db5579bdb1 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:
{
@@ -1184,8 +1186,7 @@ static int openssl_set_asym_session_parameters(
#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
BIGNUM *p = NULL, *g = NULL;
BIGNUM *q = NULL, *priv_key = NULL;
- BIGNUM *pub_key = BN_new();
- BN_zero(pub_key);
+ BIGNUM *pub_key = NULL;
OSSL_PARAM_BLD *param_bld = NULL;
p = BN_bin2bn((const unsigned char *)
@@ -1363,6 +1364,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 +1375,7 @@ static int openssl_set_asym_session_parameters(
if (asym_session->u.sm2.params)
OSSL_PARAM_free(asym_session->u.sm2.params);
+ BN_free(pkey_bn);
return -1;
#else
OPENSSL_LOG(WARNING, "SM2 unsupported in current OpenSSL Version");
@@ -1451,6 +1454,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
@@ -1460,6 +1465,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)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v4] crypto/openssl: fix memory leaks in asym ops
2023-11-13 5:41 ` [PATCH v4] " Gowrishankar Muthukrishnan
@ 2023-11-13 6:23 ` Akhil Goyal
0 siblings, 0 replies; 13+ messages in thread
From: Akhil Goyal @ 2023-11-13 6:23 UTC (permalink / raw)
To: Gowrishankar Muthukrishnan, dev
Cc: Anoob Joseph, Kai Ji, Ciara Power, Gowrishankar Muthukrishnan
> Subject: [PATCH v4] 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>
> ---
> v4:
> - patch rebased.
Applied to dpdk-next-crypto
Added-fixes tags and Ciara's ack from v3.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-11-13 6:23 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-19 13:04 [PATCH] crypto/openssl: fix memory leaks in SM2 ops Gowrishankar Muthukrishnan
2023-10-23 13:33 ` Akhil Goyal
2023-11-02 8:38 ` [PATCH v2] crypto/openssl: fix memory leaks in asym ops Gowrishankar Muthukrishnan
2023-11-02 10:03 ` Gowrishankar Muthukrishnan
2023-11-02 22:47 ` Stephen Hemminger
2023-11-03 15:19 ` [EXT] " Gowrishankar Muthukrishnan
2023-11-03 10:18 ` Power, Ciara
2023-11-03 11:38 ` Power, Ciara
2023-11-03 15:15 ` [PATCH v3] " Gowrishankar Muthukrishnan
2023-11-03 15:39 ` Power, Ciara
2023-11-09 20:18 ` Akhil Goyal
2023-11-13 5:41 ` [PATCH v4] " Gowrishankar Muthukrishnan
2023-11-13 6:23 ` Akhil Goyal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).