[PATCH 22.11] crypto/openssl: fix memory leaks in asym session

[Xueming(Steven) Li]

Hi Clara,

Thanks for the backporting, patch applied to 22.11 LTS candidate branch.

> -----Original Message-----
> From: Ciara Power <ciara.power at intel.com>
> Sent: 12/12/2023 20:11
> To: Xueming(Steven) Li <xuemingl at nvidia.com>
> Cc: stable at dpdk.org; Ciara Power <ciara.power at intel.com>; Kai Ji
> <kai.ji at intel.com>
> Subject: [PATCH 22.11] crypto/openssl: fix memory leaks in asym session
> 
> [upstream commit 47a85dda3f06 ]
> 
> Numerous memory leaks were detected by ASAN in the OpenSSL PMD
> asymmetric code path.
> 
> These are now fixed to free all variables allocated by OpenSSL functions such as
> BN_bin2bn and OSSL_PARAM_BLD_new.
> 
> Some need to exist until the op is processed, for example the BIGNUMs
> associated with DSA.
> The pointers for these are added to the private asym session so they can be
> accessed later when calling free.
> 
> Some cases need to be treated differently if OpenSSL < 3.0.
> It has slightly different handling of memory, as functions such as
> RSA_set0_key() take over memory management of values, so the caller should
> not free the values.
> 
> Fixes: 4c7ae22f1f83 ("crypto/openssl: update DSA routine with 3.0 EVP API")
> Fixes: c794b40c9258 ("crypto/openssl: update DH routine with 3.0 EVP API")
> Fixes: ac42813a0a7c ("crypto/openssl: add DH and DSA asym operations")
> Fixes: d7bd42f6db19 ("crypto/openssl: update RSA routine with 3.0 EVP API")
> 
> Signed-off-by: Ciara Power <ciara.power at intel.com>
> ---
>  drivers/crypto/openssl/openssl_pmd_private.h |  6 ++
>  drivers/crypto/openssl/rte_openssl_pmd.c     |  1 +
>  drivers/crypto/openssl/rte_openssl_pmd_ops.c | 96 +++++++++++++-------
>  3 files changed, 72 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/crypto/openssl/openssl_pmd_private.h
> b/drivers/crypto/openssl/openssl_pmd_private.h
> index ed6841e460..4e224b040b 100644
> --- a/drivers/crypto/openssl/openssl_pmd_private.h
> +++ b/drivers/crypto/openssl/openssl_pmd_private.h
> @@ -189,6 +189,8 @@ struct openssl_asym_session {
>  		struct dh {
>  			DH *dh_key;
>  			uint32_t key_op;
> +			BIGNUM *p;
> +			BIGNUM *g;
>  #if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
>  			OSSL_PARAM_BLD * param_bld;
>  			OSSL_PARAM_BLD *param_bld_peer;
> @@ -198,6 +200,10 @@ struct openssl_asym_session {
>  			DSA *dsa;
>  #if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
>  			OSSL_PARAM_BLD * param_bld;
> +			BIGNUM *p;
> +			BIGNUM *g;
> +			BIGNUM *q;
> +			BIGNUM *priv_key;
>  #endif
>  		} s;
>  	} u;
> diff --git a/drivers/crypto/openssl/rte_openssl_pmd.c
> b/drivers/crypto/openssl/rte_openssl_pmd.c
> index 6825b0469e..6ae31cb5cd 100644
> --- a/drivers/crypto/openssl/rte_openssl_pmd.c
> +++ b/drivers/crypto/openssl/rte_openssl_pmd.c
> @@ -1960,6 +1960,7 @@ 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);
> +	EVP_PKEY_free(pkey);
>  	return -1;
>  }
> 
> diff --git a/drivers/crypto/openssl/rte_openssl_pmd_ops.c
> b/drivers/crypto/openssl/rte_openssl_pmd_ops.c
> index defed4429e..24d6d48262 100644
> --- a/drivers/crypto/openssl/rte_openssl_pmd_ops.c
> +++ b/drivers/crypto/openssl/rte_openssl_pmd_ops.c
> @@ -1087,22 +1087,21 @@ static int openssl_set_asym_session_parameters(
>  	}
>  	case RTE_CRYPTO_ASYM_XFORM_DH:
>  	{
> -		BIGNUM *p = NULL;
> -		BIGNUM *g = NULL;
> -
> -		p = BN_bin2bn((const unsigned char *)
> +		DH *dh = NULL;
> +#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
> +		BIGNUM **p = &asym_session->u.dh.p;
> +		BIGNUM **g = &asym_session->u.dh.g;
> +		*p = BN_bin2bn((const unsigned char *)
>  				xform->dh.p.data,
>  				xform->dh.p.length,
> -				p);
> -		g = BN_bin2bn((const unsigned char *)
> +				*p);
> +		*g = BN_bin2bn((const unsigned char *)
>  				xform->dh.g.data,
>  				xform->dh.g.length,
> -				g);
> -		if (!p || !g)
> +				*g);
> +		if (!*p || !*g)
>  			goto err_dh;
> 
> -		DH *dh = NULL;
> -#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
>  		OSSL_PARAM_BLD *param_bld = NULL;
>  		param_bld = OSSL_PARAM_BLD_new();
>  		if (!param_bld) {
> @@ -1112,9 +1111,9 @@ static int openssl_set_asym_session_parameters(
>  		if ((!OSSL_PARAM_BLD_push_utf8_string(param_bld,
>  					"group", "ffdhe2048", 0))
>  			|| (!OSSL_PARAM_BLD_push_BN(param_bld,
> -					OSSL_PKEY_PARAM_FFC_P, p))
> +					OSSL_PKEY_PARAM_FFC_P, *p))
>  			|| (!OSSL_PARAM_BLD_push_BN(param_bld,
> -					OSSL_PKEY_PARAM_FFC_G, g))) {
> +					OSSL_PKEY_PARAM_FFC_G, *g))) {
>  			OSSL_PARAM_BLD_free(param_bld);
>  			goto err_dh;
>  		}
> @@ -1129,9 +1128,9 @@ static int openssl_set_asym_session_parameters(
>  		if ((!OSSL_PARAM_BLD_push_utf8_string(param_bld_peer,
>  					"group", "ffdhe2048", 0))
>  			|| (!OSSL_PARAM_BLD_push_BN(param_bld_peer,
> -					OSSL_PKEY_PARAM_FFC_P, p))
> +					OSSL_PKEY_PARAM_FFC_P, *p))
>  			|| (!OSSL_PARAM_BLD_push_BN(param_bld_peer,
> -					OSSL_PKEY_PARAM_FFC_G, g))) {
> +					OSSL_PKEY_PARAM_FFC_G, *g))) {
>  			OSSL_PARAM_BLD_free(param_bld);
>  			OSSL_PARAM_BLD_free(param_bld_peer);
>  			goto err_dh;
> @@ -1140,6 +1139,20 @@ static int openssl_set_asym_session_parameters(
>  		asym_session->u.dh.param_bld = param_bld;
>  		asym_session->u.dh.param_bld_peer = param_bld_peer;  #else
> +		BIGNUM *p = NULL;
> +		BIGNUM *g = NULL;
> +
> +		p = BN_bin2bn((const unsigned char *)
> +				xform->dh.p.data,
> +				xform->dh.p.length,
> +				p);
> +		g = BN_bin2bn((const unsigned char *)
> +				xform->dh.g.data,
> +				xform->dh.g.length,
> +				g);
> +		if (!p || !g)
> +			goto err_dh;
> +
>  		dh = DH_new();
>  		if (dh == NULL) {
>  			OPENSSL_LOG(ERR,
> @@ -1158,41 +1171,48 @@ static int openssl_set_asym_session_parameters(
> 
>  err_dh:
>  		OPENSSL_LOG(ERR, " failed to set dh params\n");
> +#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
> +		BN_free(*p);
> +		BN_free(*g);
> +#else
>  		BN_free(p);
>  		BN_free(g);
> +#endif
>  		return -1;
>  	}
>  	case RTE_CRYPTO_ASYM_XFORM_DSA:
>  	{
>  #if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
> -		BIGNUM *p = NULL, *g = NULL;
> -		BIGNUM *q = NULL, *priv_key = NULL;
> +		BIGNUM **p = &asym_session->u.s.p;
> +		BIGNUM **g = &asym_session->u.s.g;
> +		BIGNUM **q = &asym_session->u.s.q;
> +		BIGNUM **priv_key = &asym_session->u.s.priv_key;
>  		BIGNUM *pub_key = BN_new();
>  		BN_zero(pub_key);
>  		OSSL_PARAM_BLD *param_bld = NULL;
> 
> -		p = BN_bin2bn((const unsigned char *)
> +		*p = BN_bin2bn((const unsigned char *)
>  				xform->dsa.p.data,
>  				xform->dsa.p.length,
> -				p);
> +				*p);
> 
> -		g = BN_bin2bn((const unsigned char *)
> +		*g = BN_bin2bn((const unsigned char *)
>  				xform->dsa.g.data,
>  				xform->dsa.g.length,
> -				g);
> +				*g);
> 
> -		q = BN_bin2bn((const unsigned char *)
> +		*q = BN_bin2bn((const unsigned char *)
>  				xform->dsa.q.data,
>  				xform->dsa.q.length,
> -				q);
> -		if (!p || !q || !g)
> +				*q);
> +		if (!*p || !*q || !*g)
>  			goto err_dsa;
> 
> -		priv_key = BN_bin2bn((const unsigned char *)
> +		*priv_key = BN_bin2bn((const unsigned char *)
>  				xform->dsa.x.data,
>  				xform->dsa.x.length,
> -				priv_key);
> -		if (priv_key == NULL)
> +				*priv_key);
> +		if (*priv_key == NULL)
>  			goto err_dsa;
> 
>  		param_bld = OSSL_PARAM_BLD_new();
> @@ -1201,10 +1221,11 @@ static int openssl_set_asym_session_parameters(
>  			goto err_dsa;
>  		}
> 
> -		if (!OSSL_PARAM_BLD_push_BN(param_bld,
> OSSL_PKEY_PARAM_FFC_P, p)
> -			|| !OSSL_PARAM_BLD_push_BN(param_bld,
> OSSL_PKEY_PARAM_FFC_G, g)
> -			|| !OSSL_PARAM_BLD_push_BN(param_bld,
> OSSL_PKEY_PARAM_FFC_Q, q)
> -			|| !OSSL_PARAM_BLD_push_BN(param_bld,
> OSSL_PKEY_PARAM_PRIV_KEY, priv_key)) {
> +		if (!OSSL_PARAM_BLD_push_BN(param_bld,
> OSSL_PKEY_PARAM_FFC_P, *p)
> +			|| !OSSL_PARAM_BLD_push_BN(param_bld,
> OSSL_PKEY_PARAM_FFC_G, *g)
> +			|| !OSSL_PARAM_BLD_push_BN(param_bld,
> OSSL_PKEY_PARAM_FFC_Q, *q)
> +			|| !OSSL_PARAM_BLD_push_BN(param_bld,
> OSSL_PKEY_PARAM_PRIV_KEY,
> +			*priv_key)) {
>  			OSSL_PARAM_BLD_free(param_bld);
>  			OPENSSL_LOG(ERR, "failed to allocate resources\n");
>  			goto err_dsa;
> @@ -1268,18 +1289,25 @@ static int openssl_set_asym_session_parameters(
>  		if (ret) {
>  			DSA_free(dsa);
>  			OPENSSL_LOG(ERR, "Failed to set keys\n");
> -			return -1;
> +			goto err_dsa;
>  		}
>  		asym_session->u.s.dsa = dsa;
>  		asym_session->xfrm_type = RTE_CRYPTO_ASYM_XFORM_DSA;
>  		break;
>  #endif
>  err_dsa:
> +#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
> +		BN_free(*p);
> +		BN_free(*q);
> +		BN_free(*g);
> +		BN_free(*priv_key);
> +#else
>  		BN_free(p);
>  		BN_free(q);
>  		BN_free(g);
>  		BN_free(priv_key);
>  		BN_free(pub_key);
> +#endif
>  		return -1;
>  	}
>  	default:
> @@ -1357,10 +1385,16 @@ static void openssl_reset_asym_session(struct
> openssl_asym_session *sess)
>  		if (sess->u.dh.dh_key)
>  			DH_free(sess->u.dh.dh_key);
>  #endif
> +		BN_clear_free(sess->u.dh.p);
> +		BN_clear_free(sess->u.dh.g);
>  		break;
>  	case RTE_CRYPTO_ASYM_XFORM_DSA:
>  #if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
>  		sess->u.s.param_bld = NULL;
> +		BN_clear_free(sess->u.s.p);
> +		BN_clear_free(sess->u.s.q);
> +		BN_clear_free(sess->u.s.g);
> +		BN_clear_free(sess->u.s.priv_key);
>  #else
>  		if (sess->u.s.dsa)
>  			DSA_free(sess->u.s.dsa);
> --
> 2.25.1