diff mbox series

[v2] cryptodev: avoid algorithm strings null pointers

Message ID TY3PR01MB11575D0AC8023CEAAE444C877C250A@TY3PR01MB11575.jpnprd01.prod.outlook.com (mailing list archive)
State Rejected, archived
Delegated to: akhil goyal
Headers
Series [v2] cryptodev: avoid algorithm strings null pointers |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch-unit-testing success Testing PASS

Commit Message

liu xixin June 8, 2023, 8:03 a.m. UTC 
  The crypto algorithm strings identifiers that are Continuous may be null,
so there is needed to add null judgment.
When testing with dpdk-test-crypto-perf and passing in the parameter
--auth-algo sm3-hmac, The program caused a segfault due to a null pointer
passed in by strcmp.
Adding this patch can solve the segfault problem.

Signed-off-by: xixin.liu <liuxixin2020@outlook.com>
---
 lib/cryptodev/rte_cryptodev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Akhil Goyal June 9, 2023, 6:46 a.m. UTC | #1 
> Subject: [EXT] [PATCH v2] cryptodev: avoid algorithm strings null pointers
> 
> The crypto algorithm strings identifiers that are Continuous may be null,
> so there is needed to add null judgment.
> When testing with dpdk-test-crypto-perf and passing in the parameter
> --auth-algo sm3-hmac, The program caused a segfault due to a null pointer
> passed in by strcmp.
> Adding this patch can solve the segfault problem.

I believe this is a fix and you should add fixes tag for this and need to be backported.

> 
> Signed-off-by: xixin.liu <liuxixin2020@outlook.com>

Signoff format is not correct.
Please follow https://doc.dpdk.org/guides/contributing/patches.html


> ---
>  lib/cryptodev/rte_cryptodev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> index a96114b2da..41c23fc596 100644
> --- a/lib/cryptodev/rte_cryptodev.c
> +++ b/lib/cryptodev/rte_cryptodev.c
> @@ -346,6 +346,8 @@ rte_cryptodev_get_cipher_algo_enum(enum
> rte_crypto_cipher_algorithm *algo_enum,
>  	int ret = -1;	/* Invalid string */
> 
>  	for (i = 1; i < RTE_DIM(crypto_cipher_algorithm_strings); i++) {
> +		if (crypto_cipher_algorithm_strings[i] == NULL)
> +			continue;

crypto_cipher_algorithm_strings is a fixed size array with all non-NULL known values
and the for loop is iterating over it. So, this check does not make sense to me.


>  		if (strcmp(algo_string, crypto_cipher_algorithm_strings[i]) == 0)
> {
>  			*algo_enum = (enum rte_crypto_cipher_algorithm) i;
>  			ret = 0;
> @@ -366,6 +368,8 @@ rte_cryptodev_get_auth_algo_enum(enum
> rte_crypto_auth_algorithm *algo_enum,
>  	int ret = -1;	/* Invalid string */
> 
>  	for (i = 1; i < RTE_DIM(crypto_auth_algorithm_strings); i++) {
> +		if (crypto_auth_algorithm_strings[i] == NULL)
> +			continue;
>  		if (strcmp(algo_string, crypto_auth_algorithm_strings[i]) == 0) {
>  			*algo_enum = (enum rte_crypto_auth_algorithm) i;
>  			ret = 0;
> @@ -386,6 +390,8 @@ rte_cryptodev_get_aead_algo_enum(enum
> rte_crypto_aead_algorithm *algo_enum,
>  	int ret = -1;	/* Invalid string */
> 
>  	for (i = 1; i < RTE_DIM(crypto_aead_algorithm_strings); i++) {
> +		if (crypto_aead_algorithm_strings[i] == NULL)
> +			continue;
>  		if (strcmp(algo_string, crypto_aead_algorithm_strings[i]) == 0) {
>  			*algo_enum = (enum rte_crypto_aead_algorithm) i;
>  			ret = 0;
> @@ -406,6 +412,8 @@ rte_cryptodev_asym_get_xform_enum(enum
> rte_crypto_asym_xform_type *xform_enum,
>  	int ret = -1;	/* Invalid string */
> 
>  	for (i = 1; i < RTE_DIM(crypto_asym_xform_strings); i++) {
> +		if (crypto_asym_xform_strings[i] == NULL)
> +			continue;
>  		if (strcmp(xform_string,
>  			crypto_asym_xform_strings[i]) == 0) {
>  			*xform_enum = (enum rte_crypto_asym_xform_type) i;
> --
> 2.34.1
liu xixin June 9, 2023, 7 a.m. UTC | #2 
> Subject: [EXT] [PATCH v2] cryptodev: avoid algorithm strings null 
> pointers
> 
> The crypto algorithm strings identifiers that are Continuous may be 
> null, so there is needed to add null judgment.
> When testing with dpdk-test-crypto-perf and passing in the parameter 
> --auth-algo sm3-hmac, The program caused a segfault due to a null 
> pointer passed in by strcmp.
> Adding this patch can solve the segfault problem.

I believe this is a fix and you should add fixes tag for this and need to be backported.

> 
> Signed-off-by: xixin.liu <liuxixin2020@outlook.com>

Signoff format is not correct.
Please follow https://doc.dpdk.org/guides/contributing/patches.html


> ---
>  lib/cryptodev/rte_cryptodev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/lib/cryptodev/rte_cryptodev.c 
> b/lib/cryptodev/rte_cryptodev.c index a96114b2da..41c23fc596 100644
> --- a/lib/cryptodev/rte_cryptodev.c
> +++ b/lib/cryptodev/rte_cryptodev.c
> @@ -346,6 +346,8 @@ rte_cryptodev_get_cipher_algo_enum(enum
> rte_crypto_cipher_algorithm *algo_enum,
>  	int ret = -1;	/* Invalid string */
> 
>  	for (i = 1; i < RTE_DIM(crypto_cipher_algorithm_strings); i++) {
> +		if (crypto_cipher_algorithm_strings[i] == NULL)
> +			continue;

crypto_cipher_algorithm_strings is a fixed size array with all non-NULL known values and the for loop is iterating over it. So, this check does not make sense to me.
----> Not every element of the array is defined, eg. it is NULL that the first element [1],  if not check "strcmp(algo_string, crypto_cipher_algorithm_strings[i]" will fail

>  		if (strcmp(algo_string, crypto_cipher_algorithm_strings[i]) == 0) {
>  			*algo_enum = (enum rte_crypto_cipher_algorithm) i;
>  			ret = 0;
> @@ -366,6 +368,8 @@ rte_cryptodev_get_auth_algo_enum(enum
> rte_crypto_auth_algorithm *algo_enum,
>  	int ret = -1;	/* Invalid string */
> 
>  	for (i = 1; i < RTE_DIM(crypto_auth_algorithm_strings); i++) {
> +		if (crypto_auth_algorithm_strings[i] == NULL)
> +			continue;
>  		if (strcmp(algo_string, crypto_auth_algorithm_strings[i]) == 0) {
>  			*algo_enum = (enum rte_crypto_auth_algorithm) i;
>  			ret = 0;
> @@ -386,6 +390,8 @@ rte_cryptodev_get_aead_algo_enum(enum
> rte_crypto_aead_algorithm *algo_enum,
>  	int ret = -1;	/* Invalid string */
> 
>  	for (i = 1; i < RTE_DIM(crypto_aead_algorithm_strings); i++) {
> +		if (crypto_aead_algorithm_strings[i] == NULL)
> +			continue;
>  		if (strcmp(algo_string, crypto_aead_algorithm_strings[i]) == 0) {
>  			*algo_enum = (enum rte_crypto_aead_algorithm) i;
>  			ret = 0;
> @@ -406,6 +412,8 @@ rte_cryptodev_asym_get_xform_enum(enum
> rte_crypto_asym_xform_type *xform_enum,
>  	int ret = -1;	/* Invalid string */
> 
>  	for (i = 1; i < RTE_DIM(crypto_asym_xform_strings); i++) {
> +		if (crypto_asym_xform_strings[i] == NULL)
> +			continue;
>  		if (strcmp(xform_string,
>  			crypto_asym_xform_strings[i]) == 0) {
>  			*xform_enum = (enum rte_crypto_asym_xform_type) i;
> --
> 2.34.1
Akhil Goyal June 9, 2023, 8:06 a.m. UTC | #3 
> -----Original Message-----
> From: liu xixin <liuxixin2020@outlook.com>
> Sent: Friday, June 9, 2023 12:31 PM
> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org
> Subject: 答复: [EXT] [PATCH v2] cryptodev: avoid algorithm strings null pointers
> 
> > Subject: [EXT] [PATCH v2] cryptodev: avoid algorithm strings null
> > pointers
> >
> > The crypto algorithm strings identifiers that are Continuous may be
> > null, so there is needed to add null judgment.
> > When testing with dpdk-test-crypto-perf and passing in the parameter
> > --auth-algo sm3-hmac, The program caused a segfault due to a null
> > pointer passed in by strcmp.
> > Adding this patch can solve the segfault problem.
> 
> I believe this is a fix and you should add fixes tag for this and need to be
> backported.
> 
> >
> > Signed-off-by: xixin.liu <liuxixin2020@outlook.com>
> 
> Signoff format is not correct.
> Please follow https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__doc.dpdk.org_guides_contributing_patches.html&d=DwIGoQ&c=nKjWec2b
> 6R0mOyPaz7xtfQ&r=DnL7Si2wl_PRwpZ9TWey3eu68gBzn7DkPwuqhd6WNyo&m
> =6mzvm07Go5-
> S_2jVk9KRF1mOl0Juay2Sa2WI2XiNgTbg6ZhMcm75GNceSVhe0Doj&s=tvqhxvElc
> m_8h3e7YIBym6IAHk6BxUAFx2RKmjJ6Ibw&e=
> 
> 
> > ---
> >  lib/cryptodev/rte_cryptodev.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/lib/cryptodev/rte_cryptodev.c
> > b/lib/cryptodev/rte_cryptodev.c index a96114b2da..41c23fc596 100644
> > --- a/lib/cryptodev/rte_cryptodev.c
> > +++ b/lib/cryptodev/rte_cryptodev.c
> > @@ -346,6 +346,8 @@ rte_cryptodev_get_cipher_algo_enum(enum
> > rte_crypto_cipher_algorithm *algo_enum,
> >  	int ret = -1;	/* Invalid string */
> >
> >  	for (i = 1; i < RTE_DIM(crypto_cipher_algorithm_strings); i++) {
> > +		if (crypto_cipher_algorithm_strings[i] == NULL)
> > +			continue;
> 
> crypto_cipher_algorithm_strings is a fixed size array with all non-NULL known
> values and the for loop is iterating over it. So, this check does not make sense to
> me.
> ----> Not every element of the array is defined, eg. it is NULL that the first
> element [1],  if not check "strcmp(algo_string,
> crypto_cipher_algorithm_strings[i]" will fail

The loop starts from 1 so 0th element = NULL will not matter.
And for all other values it is not null. If something is missing,
then it can be added.
But this check is not needed.
Akhil Goyal June 9, 2023, 8:30 a.m. UTC | #4 
Please reply only in plain text format.

All the values are added in the array but are jumbled up.
But still it will point to correct position in the array as we are using enum value.
Eg. RTE_CRYPTO_CIPHER_NULL = 1,
And in the array it is there as highlighted below.
Hence there is no hole present in the array.


static const char *
crypto_cipher_algorithm_strings[] = {
        [RTE_CRYPTO_CIPHER_3DES_CBC]    = "3des-cbc",
        [RTE_CRYPTO_CIPHER_3DES_ECB]    = "3des-ecb",
        [RTE_CRYPTO_CIPHER_3DES_CTR]    = "3des-ctr",

        [RTE_CRYPTO_CIPHER_AES_CBC]     = "aes-cbc",
        [RTE_CRYPTO_CIPHER_AES_CTR]     = "aes-ctr",
        [RTE_CRYPTO_CIPHER_AES_DOCSISBPI]       = "aes-docsisbpi",
        [RTE_CRYPTO_CIPHER_AES_ECB]     = "aes-ecb",
        [RTE_CRYPTO_CIPHER_AES_F8]      = "aes-f8",
        [RTE_CRYPTO_CIPHER_AES_XTS]     = "aes-xts",

        [RTE_CRYPTO_CIPHER_ARC4]        = "arc4",

        [RTE_CRYPTO_CIPHER_DES_CBC]     = "des-cbc",
        [RTE_CRYPTO_CIPHER_DES_DOCSISBPI]       = "des-docsisbpi",

        [RTE_CRYPTO_CIPHER_NULL]        = "null",             ----------------------------------------->

        [RTE_CRYPTO_CIPHER_KASUMI_F8]   = "kasumi-f8",
        [RTE_CRYPTO_CIPHER_SNOW3G_UEA2] = "snow3g-uea2",
        [RTE_CRYPTO_CIPHER_ZUC_EEA3]    = "zuc-eea3",
        [RTE_CRYPTO_CIPHER_SM4_ECB]     = "sm4-ecb",
        [RTE_CRYPTO_CIPHER_SM4_CBC]     = "sm4-cbc",
        [RTE_CRYPTO_CIPHER_SM4_CTR]     = "sm4-ctr",
        [RTE_CRYPTO_CIPHER_SM4_CFB]     = "sm4-cfb",
        [RTE_CRYPTO_CIPHER_SM4_OFB]     = "sm4-ofb"
};


From: liu xixin <liuxixin2020@outlook.com>
Sent: Friday, June 9, 2023 1:45 PM
To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org
Subject: 回复: [EXT] [PATCH v2] cryptodev: avoid algorithm strings null pointers

> -----Original Message-----
> From: liu xixin <liuxixin2020@outlook.com<mailto:liuxixin2020@outlook.com>>
> Sent: Friday, June 9, 2023 12:31 PM
> To: Akhil Goyal <gakhil@marvell.com<mailto:gakhil@marvell.com>>; dev@dpdk.org<mailto:dev@dpdk.org>
> Subject: 答复: [EXT] [PATCH v2] cryptodev: avoid algorithm strings null pointers
>
> > Subject: [EXT] [PATCH v2] cryptodev: avoid algorithm strings null
> > pointers
> >
> > The crypto algorithm strings identifiers that are Continuous may be
> > null, so there is needed to add null judgment.
> > When testing with dpdk-test-crypto-perf and passing in the parameter
> > --auth-algo sm3-hmac, The program caused a segfault due to a null
> > pointer passed in by strcmp.
> > Adding this patch can solve the segfault problem.
>
> I believe this is a fix and you should add fixes tag for this and need to be
> backported.
>
> >
> > Signed-off-by: xixin.liu <liuxixin2020@outlook.com<mailto:liuxixin2020@outlook.com>>
>
> Signoff format is not correct.
> Please follow https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__doc.dpdk.org_guides_contributing_patches.html&d=DwIGoQ&c=nKjWec2b
> 6R0mOyPaz7xtfQ&r=DnL7Si2wl_PRwpZ9TWey3eu68gBzn7DkPwuqhd6WNyo&m
> =6mzvm07Go5-
> S_2jVk9KRF1mOl0Juay2Sa2WI2XiNgTbg6ZhMcm75GNceSVhe0Doj&s=tvqhxvElc
> m_8h3e7YIBym6IAHk6BxUAFx2RKmjJ6Ibw&e=
>
>
> > ---
> >  lib/cryptodev/rte_cryptodev.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/lib/cryptodev/rte_cryptodev.c
> > b/lib/cryptodev/rte_cryptodev.c index a96114b2da..41c23fc596 100644
> > --- a/lib/cryptodev/rte_cryptodev.c
> > +++ b/lib/cryptodev/rte_cryptodev.c
> > @@ -346,6 +346,8 @@ rte_cryptodev_get_cipher_algo_enum(enum
> > rte_crypto_cipher_algorithm *algo_enum,
> >      int ret = -1;   /* Invalid string */
> >
> >      for (i = 1; i < RTE_DIM(crypto_cipher_algorithm_strings); i++) {
> > +           if (crypto_cipher_algorithm_strings[i] == NULL)
> > +                   continue;
>
> crypto_cipher_algorithm_strings is a fixed size array with all non-NULL known
> values and the for loop is iterating over it. So, this check does not make sense to
> me.
> ----> Not every element of the array is defined, eg. it is NULL that the first
> element [1],  if not check "strcmp(algo_string,
> crypto_cipher_algorithm_strings[i]" will fail

The loop starts from 1 so 0th element = NULL will not matter.

And for all other values it is not null. If something is missing,
then it can be added.
But this check is not needed.
---> [0] is no need to check 。However, the values of all subsequent elements are checked to be null
static const char *
crypto_cipher_algorithm_strings[] = {
    [RTE_CRYPTO_CIPHER_3DES_CBC]    = "3des-cbc",  //this RTE_CRYPTO_CIPHER_3DES_CBC == 2，so 【1】already is NULL
    [RTE_CRYPTO_CIPHER_3DES_ECB]    = "3des-ecb", // same as RTE_CRYPTO_CIPHER_3DES_ECB == 4 ，and now 【1】【3】are all NULL
    [RTE_CRYPTO_CIPHER_3DES_CTR]    = "3des-ctr",  // and so on
...
};
diff mbox series

Patch

diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
index a96114b2da..41c23fc596 100644
--- a/lib/cryptodev/rte_cryptodev.c
+++ b/lib/cryptodev/rte_cryptodev.c
@@ -346,6 +346,8 @@  rte_cryptodev_get_cipher_algo_enum(enum rte_crypto_cipher_algorithm *algo_enum,
 	int ret = -1;	/* Invalid string */
 
 	for (i = 1; i < RTE_DIM(crypto_cipher_algorithm_strings); i++) {
+		if (crypto_cipher_algorithm_strings[i] == NULL)
+			continue;
 		if (strcmp(algo_string, crypto_cipher_algorithm_strings[i]) == 0) {
 			*algo_enum = (enum rte_crypto_cipher_algorithm) i;
 			ret = 0;
@@ -366,6 +368,8 @@  rte_cryptodev_get_auth_algo_enum(enum rte_crypto_auth_algorithm *algo_enum,
 	int ret = -1;	/* Invalid string */
 
 	for (i = 1; i < RTE_DIM(crypto_auth_algorithm_strings); i++) {
+		if (crypto_auth_algorithm_strings[i] == NULL)
+			continue;
 		if (strcmp(algo_string, crypto_auth_algorithm_strings[i]) == 0) {
 			*algo_enum = (enum rte_crypto_auth_algorithm) i;
 			ret = 0;
@@ -386,6 +390,8 @@  rte_cryptodev_get_aead_algo_enum(enum rte_crypto_aead_algorithm *algo_enum,
 	int ret = -1;	/* Invalid string */
 
 	for (i = 1; i < RTE_DIM(crypto_aead_algorithm_strings); i++) {
+		if (crypto_aead_algorithm_strings[i] == NULL)
+			continue;
 		if (strcmp(algo_string, crypto_aead_algorithm_strings[i]) == 0) {
 			*algo_enum = (enum rte_crypto_aead_algorithm) i;
 			ret = 0;
@@ -406,6 +412,8 @@  rte_cryptodev_asym_get_xform_enum(enum rte_crypto_asym_xform_type *xform_enum,
 	int ret = -1;	/* Invalid string */
 
 	for (i = 1; i < RTE_DIM(crypto_asym_xform_strings); i++) {
+		if (crypto_asym_xform_strings[i] == NULL)
+			continue;
 		if (strcmp(xform_string,
 			crypto_asym_xform_strings[i]) == 0) {
 			*xform_enum = (enum rte_crypto_asym_xform_type) i;