[RFC 0/5] Fix memset usage

[RFC 0/5] Fix memset usage

[Stephen Hemminger]

Bruce mentioned recent security scan of DPDK
  	https://pvs-studio.com/en/blog/posts/cpp/1183/

There are some low hanging fruit things that can be easily addressed.

Stephen Hemminger (5):
  crypto/qat: fix memset of SHA3 state
  crypto/qat: use secure memset
  bus/uacce: remove memset before free
  compress/octeontx: remove memset before rte_free
  test: remove unneeded memset

 app/test/test_cmdline_cirbuf.c          |  2 --
 drivers/bus/uacce/uacce.c               |  1 -
 drivers/compress/octeontx/otx_zip.c     |  1 -
 drivers/compress/octeontx/otx_zip_pmd.c |  2 --
 drivers/crypto/qat/qat_asym.c           |  2 +-
 drivers/crypto/qat/qat_sym_session.c    | 10 +++++-----
 6 files changed, 6 insertions(+), 12 deletions(-)

-- 
2.45.2

[RFC 1/5] crypto/qat: fix memset of SHA3 state

[Stephen Hemminger]

The memset was always doing 0 bytes since size computed later.

PVS studio bug 24-27

Fixes: 3a80d7fb2ecd ("crypto/qat: support SHA3 plain hash")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/crypto/qat/qat_sym_session.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c
index 50d687fd37..8ccd1babc0 100644
--- a/drivers/crypto/qat/qat_sym_session.c
+++ b/drivers/crypto/qat/qat_sym_session.c
@@ -2346,7 +2346,7 @@ int qat_sym_cd_cipher_set(struct qat_sym_session *cdesc,
 	return 0;
 }
 
-int qat_sym_cd_auth_set(struct qat_sym_session *cdesc,
+static int qat_sym_cd_auth_set(struct qat_sym_session *cdesc,
 		const uint8_t *authkey,
 		uint32_t authkeylen,
 		uint32_t aad_length,
@@ -2619,27 +2619,27 @@ int qat_sym_cd_auth_set(struct qat_sym_session *cdesc,
 		break;
 	case ICP_QAT_HW_AUTH_ALGO_SHA3_224:
 		/* Plain SHA3-224 */
-		memset(cdesc->cd_cur_ptr, 0, state1_size);
 		state1_size = qat_hash_get_state1_size(
 				cdesc->qat_hash_alg);
+		memset(cdesc->cd_cur_ptr, 0, state1_size);
 		break;
 	case ICP_QAT_HW_AUTH_ALGO_SHA3_256:
 		/* Plain SHA3-256 */
-		memset(cdesc->cd_cur_ptr, 0, state1_size);
 		state1_size = qat_hash_get_state1_size(
 				cdesc->qat_hash_alg);
+		memset(cdesc->cd_cur_ptr, 0, state1_size);
 		break;
 	case ICP_QAT_HW_AUTH_ALGO_SHA3_384:
 		/* Plain SHA3-384 */
-		memset(cdesc->cd_cur_ptr, 0, state1_size);
 		state1_size = qat_hash_get_state1_size(
 				cdesc->qat_hash_alg);
+		memset(cdesc->cd_cur_ptr, 0, state1_size);
 		break;
 	case ICP_QAT_HW_AUTH_ALGO_SHA3_512:
 		/* Plain SHA3-512 */
-		memset(cdesc->cd_cur_ptr, 0, state1_size);
 		state1_size = qat_hash_get_state1_size(
 				cdesc->qat_hash_alg);
+		memset(cdesc->cd_cur_ptr, 0, state1_size);
 		break;
 	case ICP_QAT_HW_AUTH_ALGO_AES_XCBC_MAC:
 		state1_size = ICP_QAT_HW_AES_XCBC_MAC_STATE1_SZ;
-- 
2.45.2

[RFC 2/5] crypto/qat: use secure memset

[Stephen Hemminger]

Regular memset maybe removed by compiler if done before a free
function. Use the C11 memset_s instead to ensure security
parameters are cleared.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/crypto/qat/qat_asym.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/qat/qat_asym.c b/drivers/crypto/qat/qat_asym.c
index f5b56b2f71..a2b87ddbfd 100644
--- a/drivers/crypto/qat/qat_asym.c
+++ b/drivers/crypto/qat/qat_asym.c
@@ -103,7 +103,7 @@ static const struct rte_driver cryptodev_qat_asym_driver = {
 
 #define PARAM_CLR(what) \
 	do { \
-		memset(what.data, 0, what.length); \
+		memset_s(what.data, 0, what.length); \
 		rte_free(what.data);	\
 	} while (0)
 
-- 
2.45.2

[RFC 3/5] bus/uacce: remove memset before free

[Stephen Hemminger]

Doing memset before free maybe removed by compiler, and
is flagged by security scanning tools as potential problem.
In this case the memset is unnecessary.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/bus/uacce/uacce.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/bus/uacce/uacce.c b/drivers/bus/uacce/uacce.c
index c1529c38c0..35c1027245 100644
--- a/drivers/bus/uacce/uacce.c
+++ b/drivers/bus/uacce/uacce.c
@@ -454,7 +454,6 @@ uacce_cleanup(void)
 		dev->device.driver = NULL;
 
 free:
-		memset(dev, 0, sizeof(*dev));
 		free(dev);
 	}
 
-- 
2.45.2

[RFC 4/5] compress/octeontx: remove memset before rte_free

[Stephen Hemminger]

Calling memset before rte_free not necessary, and could be
removed by the compiler. In this case, the data is not security
sensitive so the memset can be removed. Some security scanning
tools will flag this.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/compress/octeontx/otx_zip.c     | 1 -
 drivers/compress/octeontx/otx_zip_pmd.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/compress/octeontx/otx_zip.c b/drivers/compress/octeontx/otx_zip.c
index 11471dcbb4..331d2d9475 100644
--- a/drivers/compress/octeontx/otx_zip.c
+++ b/drivers/compress/octeontx/otx_zip.c
@@ -81,7 +81,6 @@ zipvf_q_term(struct zipvf_qp *qp)
 	struct zip_vf *vf = qp->vf;
 
 	if (cmdq->va != NULL) {
-		memset(cmdq->va, 0, ZIP_MAX_CMDQ_SIZE);
 		rte_free(cmdq->va);
 	}
 
diff --git a/drivers/compress/octeontx/otx_zip_pmd.c b/drivers/compress/octeontx/otx_zip_pmd.c
index c8f456b319..74e3e942ad 100644
--- a/drivers/compress/octeontx/otx_zip_pmd.c
+++ b/drivers/compress/octeontx/otx_zip_pmd.c
@@ -479,8 +479,6 @@ zip_pmd_stream_free(struct rte_compressdev *dev, void *stream)
 				(void *)&(z_stream->bufs[0]),
 				(MAX_BUFS_PER_STREAM * ZIP_BURST_SIZE));
 
-	/* Zero out the whole structure */
-	memset(stream, 0, sizeof(struct zip_stream));
 	rte_free(stream);
 
 	return 0;
-- 
2.45.2

[RFC 5/5] test: remove unneeded memset

[Stephen Hemminger]

Since tmp is not used later in the function, this memset
is unnecessary. Even though this is harmless,
it causes tools that look for security issues
around memset to flag this a bug.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_cmdline_cirbuf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/test/test_cmdline_cirbuf.c b/app/test/test_cmdline_cirbuf.c
index 8ac326cb02..1be357abf6 100644
--- a/app/test/test_cmdline_cirbuf.c
+++ b/app/test/test_cmdline_cirbuf.c
@@ -281,8 +281,6 @@ test_cirbuf_string_add_del_reverse(void)
 		printf("Error: buffer should have been empty!\n");
 		return -1;
 	}
-	/* clear tmp buffer */
-	memset(tmp, 0, sizeof(tmp));
 
 	/*
 	 * reinitialize circular buffer
-- 
2.45.2

Re: [RFC 3/5] bus/uacce: remove memset before free

[fengchengwen]

On 2024/11/14 2:56, Stephen Hemminger wrote:
> Doing memset before free maybe removed by compiler, and
> is flagged by security scanning tools as potential problem.
> In this case the memset is unnecessary.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/bus/uacce/uacce.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/bus/uacce/uacce.c b/drivers/bus/uacce/uacce.c
> index c1529c38c0..35c1027245 100644
> --- a/drivers/bus/uacce/uacce.c
> +++ b/drivers/bus/uacce/uacce.c
> @@ -454,7 +454,6 @@ uacce_cleanup(void)
>  		dev->device.driver = NULL;
>  
>  free:
> -		memset(dev, 0, sizeof(*dev));

It should replaced by TAILQ_REMOVE(&uacce_bus.device_list, dev, next);

And I also find other bus have the same problem, later I will push one patchset.

>  		free(dev);
>  	}
>  

Re: [RFC 0/5] Fix memset usage

[Bruce Richardson]

On Wed, Nov 13, 2024 at 10:55:59AM -0800, Stephen Hemminger wrote:
> Bruce mentioned recent security scan of DPDK
>   	https://pvs-studio.com/en/blog/posts/cpp/1183/
> 
> There are some low hanging fruit things that can be easily addressed.
> 
> Stephen Hemminger (5):
>   crypto/qat: fix memset of SHA3 state
>   crypto/qat: use secure memset
>   bus/uacce: remove memset before free
>   compress/octeontx: remove memset before rte_free
>   test: remove unneeded memset
> 
These fixes look fine to me.

Thanks.

Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>