[PATCH 1/3] crypto/ipsec_mb: fix qp setup null pointer dereference

[PATCH 1/3] crypto/ipsec_mb: fix qp setup null pointer dereference

[Ciara Power]

When setting up a qp in a secondary process, the local qp pointer is set
to the stored device qp, configured by the primary process for that
device, but only if that device qp is not NULL.
If the device qp was not set up correctly by the primary process and has
a NULL value, the local qp variable stays at the default initialised
value, NULL. This causes a NULL pointer dereference later in the
function when using the qp value.

This is fixed by always setting the local qp to the value of the device
qp stored, and then checking if qp is NULL, returning an error if it is.

Coverity issue: 374382
Fixes: 72a169278a56 ("crypto/ipsec_mb: support multi-process")
Cc: stable@dpdk.org

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 drivers/crypto/ipsec_mb/ipsec_mb_ops.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
index 189262c4ad..6efa417d67 100644
--- a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
+++ b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
@@ -221,8 +221,11 @@ ipsec_mb_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
 				IMB_VERSION_STR, IMB_MP_REQ_VER_STR);
 		return -EINVAL;
 #endif
-		if (dev->data->queue_pairs[qp_id] != NULL)
-			qp = dev->data->queue_pairs[qp_id];
+		qp = dev->data->queue_pairs[qp_id];
+		if (qp == NULL) {
+			IPSEC_MB_LOG(ERR, "Primary process hasn't configured device qp.");
+			return -EINVAL;
+		}
 	} else {
 		/* Free memory prior to re-allocation if needed. */
 		if (dev->data->queue_pairs[qp_id] != NULL)
-- 
2.25.1

[PATCH 2/3] crypto/ipsec_mb: fix qp cleanup null pointer dereference

[Ciara Power]

The qp was being used in the cleanup without checking if it was non NULL.
A check is now added to verify qp is non NULL before use.

Coverity issue: 374375
Fixes: c75542ae4200 ("crypto/ipsec_mb: introduce IPsec_mb framework")
Cc: roy.fan.zhang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 drivers/crypto/ipsec_mb/ipsec_mb_ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
index 6efa417d67..1ebd23e8f0 100644
--- a/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
+++ b/drivers/crypto/ipsec_mb/ipsec_mb_ops.c
@@ -285,6 +285,8 @@ ipsec_mb_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
 	return 0;
 
 qp_setup_cleanup:
+	if (qp == NULL)
+		return ret;
 #if IMB_VERSION(1, 1, 0) > IMB_VERSION_NUM
 	if (qp->mb_mgr)
 		free_mb_mgr(qp->mb_mgr);
@@ -294,8 +296,7 @@ ipsec_mb_qp_setup(struct rte_cryptodev *dev, uint16_t qp_id,
 	if (qp->mb_mgr_mz)
 		rte_memzone_free(qp->mb_mgr_mz);
 #endif
-	if (qp)
-		rte_free(qp);
+	rte_free(qp);
 	return ret;
 }
 
-- 
2.25.1

[PATCH 3/3] crypto/ipsec_mb: fix tainted data for session

[Ciara Power]

Downcasting a void * to struct aesni_gcm_session * caused the session
data to be treated as tainted.
Removing the void * temporary variable and adding a cast avoids this
issue.

Coverity issue: 374377
Fixes: 746825e5c0ea ("crypto/ipsec_mb: move aesni_gcm PMD")
Cc: piotrx.bronowski@intel.com
Cc: stable@dpdk.org

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 drivers/crypto/ipsec_mb/pmd_aesni_gcm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/ipsec_mb/pmd_aesni_gcm.c b/drivers/crypto/ipsec_mb/pmd_aesni_gcm.c
index 2c203795ab..e5ad629fe5 100644
--- a/drivers/crypto/ipsec_mb/pmd_aesni_gcm.c
+++ b/drivers/crypto/ipsec_mb/pmd_aesni_gcm.c
@@ -713,19 +713,17 @@ aesni_gcm_process_bulk(struct rte_cryptodev *dev,
 			__rte_unused union rte_crypto_sym_ofs ofs,
 			struct rte_crypto_sym_vec *vec)
 {
-	void *sess_priv;
 	struct aesni_gcm_session *s;
 	struct gcm_context_data gdata_ctx;
 	IMB_MGR *mb_mgr;
 
-	sess_priv = get_sym_session_private_data(sess, dev->driver_id);
-	if (unlikely(sess_priv == NULL)) {
+	s = (struct aesni_gcm_session *) get_sym_session_private_data(sess,
+		dev->driver_id);
+	if (unlikely(s == NULL)) {
 		aesni_gcm_fill_error_code(vec, EINVAL);
 		return 0;
 	}
 
-	s = sess_priv;
-
 	/* get per-thread MB MGR, create one if needed */
 	mb_mgr = get_per_thread_mb_mgr();
 	if (unlikely(mb_mgr == NULL))
-- 
2.25.1

RE: [PATCH 1/3] crypto/ipsec_mb: fix qp setup null pointer dereference

[Zhang, Roy Fan]

> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Friday, December 10, 2021 2:10 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Mcnamara, John <john.mcnamara@intel.com>; Zhang,
> Roy Fan <roy.fan.zhang@intel.com>; Power, Ciara <ciara.power@intel.com>;
> De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: [PATCH 1/3] crypto/ipsec_mb: fix qp setup null pointer dereference
> 
> When setting up a qp in a secondary process, the local qp pointer is set
> to the stored device qp, configured by the primary process for that
> device, but only if that device qp is not NULL.
> If the device qp was not set up correctly by the primary process and has
> a NULL value, the local qp variable stays at the default initialised
> value, NULL. This causes a NULL pointer dereference later in the
> function when using the qp value.
> 
> This is fixed by always setting the local qp to the value of the device
> qp stored, and then checking if qp is NULL, returning an error if it is.
> 
> Coverity issue: 374382
> Fixes: 72a169278a56 ("crypto/ipsec_mb: support multi-process")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

RE: [PATCH 2/3] crypto/ipsec_mb: fix qp cleanup null pointer dereference

[Zhang, Roy Fan]

> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Friday, December 10, 2021 2:10 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Mcnamara, John <john.mcnamara@intel.com>; Zhang,
> Roy Fan <roy.fan.zhang@intel.com>; Power, Ciara <ciara.power@intel.com>;
> De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: [PATCH 2/3] crypto/ipsec_mb: fix qp cleanup null pointer
> dereference
> 
> The qp was being used in the cleanup without checking if it was non NULL.
> A check is now added to verify qp is non NULL before use.
> 
> Coverity issue: 374375
> Fixes: c75542ae4200 ("crypto/ipsec_mb: introduce IPsec_mb framework")
> Cc: roy.fan.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

RE: [PATCH 3/3] crypto/ipsec_mb: fix tainted data for session

[Zhang, Roy Fan]

> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Friday, December 10, 2021 2:10 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Mcnamara, John <john.mcnamara@intel.com>; Zhang,
> Roy Fan <roy.fan.zhang@intel.com>; Power, Ciara <ciara.power@intel.com>;
> Bronowski, PiotrX <piotrx.bronowski@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Subject: [PATCH 3/3] crypto/ipsec_mb: fix tainted data for session
> 
> Downcasting a void * to struct aesni_gcm_session * caused the session
> data to be treated as tainted.
> Removing the void * temporary variable and adding a cast avoids this
> issue.
> 
> Coverity issue: 374377
> Fixes: 746825e5c0ea ("crypto/ipsec_mb: move aesni_gcm PMD")
> Cc: piotrx.bronowski@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

RE: [EXT] [PATCH 3/3] crypto/ipsec_mb: fix tainted data for session

[Akhil Goyal]

> Downcasting a void * to struct aesni_gcm_session * caused the session
> data to be treated as tainted.
> Removing the void * temporary variable and adding a cast avoids this
> issue.
> 
> Coverity issue: 374377
> Fixes: 746825e5c0ea ("crypto/ipsec_mb: move aesni_gcm PMD")
> Cc: piotrx.bronowski@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
Series Applied to dpdk-next-crypto

Thanks.