[PATCH 0/3] Fix three coverity issues of bond PMD

[PATCH 0/3] Fix three coverity issues of bond PMD

[Chaoyong He]

This patch series fix three coverity issues about bond PMD and test
case, 403097, 403099 and 403101.

Long Wu (3):
  app/test: fix control flow issue
  net/bonding: fix illegal memory accesses
  app/test: fix checking return value

 app/test/test_link_bonding.c              | 3 ++-
 app/test/test_link_bonding_mode4.c        | 3 +--
 drivers/net/bonding/rte_eth_bond_8023ad.c | 4 ++++
 3 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.39.1

[PATCH 1/3] app/test: fix control flow issue

[Chaoyong He]

From: Long Wu <long.wu@corigine.com>

CI found that execution cannot reach the expression "-1"
inside this statement.

Coverity issue: 403097
Fixes: 5e41ab250dfa ("app/test: unit tests for bonding mode 4")
Cc: tomaszx.kulasek@intel.com
Cc: stable@dpdk.org

Signed-off-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 app/test/test_link_bonding_mode4.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index 645fc1e0d4..ff13dbed93 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -641,8 +641,7 @@ bond_handshake(void)
 	/* If response didn't send - report failure */
 	TEST_ASSERT_EQUAL(all_members_done, 1, "Bond handshake failed\n");
 
-	/* If flags doesn't match - report failure */
-	return all_members_done == 1 ? TEST_SUCCESS : TEST_FAILED;
+	return TEST_SUCCESS;
 }
 
 #define TEST_LACP_MEMBER_COUT RTE_DIM(test_params.member_ports)
-- 
2.39.1

Re: [PATCH 1/3] app/test: fix control flow issue

[Ferruh Yigit]

On 10/10/2023 7:23 AM, Chaoyong He wrote:
> From: Long Wu <long.wu@corigine.com>
> 
> CI found that execution cannot reach the expression "-1"
> inside this statement.
> 
> Coverity issue: 403097
> Fixes: 5e41ab250dfa ("app/test: unit tests for bonding mode 4")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.

[PATCH 2/3] net/bonding: fix illegal memory accesses

[Chaoyong He]

From: Long Wu <long.wu@corigine.com>

CI found that overrunning array of 32 2-byte elements at
element index 65535 (byte offset 131071) by dereferencing
pointer "members + agg_new_idx".

Coverity issue: 403099
Fixes: 6d72657ce379 ("net/bonding: add other aggregator modes")
Cc: danielx.t.mrzyglod@intel.com
Cc: stable@dpdk.org

Signed-off-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index 677067870f..0be33f61e3 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -732,10 +732,14 @@ selection_logic(struct bond_dev_private *internals, uint16_t member_id)
 	switch (internals->mode4.agg_selection) {
 	case AGG_COUNT:
 		agg_new_idx = max_index(agg_count, members_count);
+		if (agg_new_idx >= members_count)
+			agg_new_idx = default_member;
 		new_agg_id = members[agg_new_idx];
 		break;
 	case AGG_BANDWIDTH:
 		agg_new_idx = max_index(agg_bandwidth, members_count);
+		if (agg_new_idx >= members_count)
+			agg_new_idx = default_member;
 		new_agg_id = members[agg_new_idx];
 		break;
 	case AGG_STABLE:
-- 
2.39.1

Re: [PATCH 2/3] net/bonding: fix illegal memory accesses

[Ferruh Yigit]

On 10/10/2023 7:23 AM, Chaoyong He wrote:
> From: Long Wu <long.wu@corigine.com>
> 
> CI found that overrunning array of 32 2-byte elements at
> element index 65535 (byte offset 131071) by dereferencing
> pointer "members + agg_new_idx".
> 
> Coverity issue: 403099
> Fixes: 6d72657ce379 ("net/bonding: add other aggregator modes")
> Cc: danielx.t.mrzyglod@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> ---
>  drivers/net/bonding/rte_eth_bond_8023ad.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index 677067870f..0be33f61e3 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -732,10 +732,14 @@ selection_logic(struct bond_dev_private *internals, uint16_t member_id)
>  	switch (internals->mode4.agg_selection) {
>  	case AGG_COUNT:
>  		agg_new_idx = max_index(agg_count, members_count);
> +		if (agg_new_idx >= members_count)
> +			agg_new_idx = default_member;
>  		new_agg_id = members[agg_new_idx];
>

Overrun may happen when 'max_index()' returns error, '-1', which becomes
'UINT16_MAX' as function returns 'uint16_t'.

And 'max_index()' returns error only if "members_count <= 0", but as far
as I can see 'members_count' can't be "<= 0" anyway.

What do you think to remove check in the 'max_index()', or add a check
in 'selection_logic()' for 'members_count == 0', but not sure what to do
'max_index()'in this case, so updating 'max_index()' is simpler.

[PATCH v2] net/bonding: fix illegal memory accesses

[Chaoyong He]

From: Long Wu <long.wu@corigine.com>

CI found that overrunning array of 32 2-byte elements at
element index 65535 (byte offset 131071) by dereferencing
pointer "members + agg_new_idx".

Coverity issue: 403099
Fixes: 6d72657 ("net/bonding: add other aggregator modes")
Cc: danielx.t.mrzyglod@intel.com
Cc: stable@dpdk.org

Signed-off-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>

---
v2:
* Modify the logic of 'max_index()'.
---
 drivers/net/bonding/rte_eth_bond_8023ad.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index 677067870f..79f1b3f1a0 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -654,12 +654,9 @@ tx_machine(struct bond_dev_private *internals, uint16_t member_id)
 }
 
 static uint16_t
-max_index(uint64_t *a, int n)
+max_index(uint64_t *a, uint16_t n)
 {
-	if (n <= 0)
-		return -1;
-
-	int i, max_i = 0;
+	uint16_t i, max_i = 0;
 	uint64_t max = a[0];
 
 	for (i = 1; i < n; ++i) {
-- 
2.39.1

Re: [PATCH v2] net/bonding: fix illegal memory accesses

[Ferruh Yigit]

On 11/1/2023 2:19 AM, Chaoyong He wrote:
> From: Long Wu <long.wu@corigine.com>
> 
> CI found that overrunning array of 32 2-byte elements at
> element index 65535 (byte offset 131071) by dereferencing
> pointer "members + agg_new_idx".
> 
> Coverity issue: 403099
> Fixes: 6d72657 ("net/bonding: add other aggregator modes")
> Cc: danielx.t.mrzyglod@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.

[PATCH 3/3] app/test: fix checking return value

[Chaoyong He]

From: Long Wu <long.wu@corigine.com>

CI found the function without checking return value in this place.

Coverity issue: 403101
Fixes: 92073ef961ee ("bond: unit tests")
Cc: declan.doherty@intel.com
Cc: stable@dpdk.org

Signed-off-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 app/test/test_link_bonding.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
index 4d715c4465..4d54706c21 100644
--- a/app/test/test_link_bonding.c
+++ b/app/test/test_link_bonding.c
@@ -449,7 +449,8 @@ test_add_already_bonding_member_to_bonding_device(void)
 	uint16_t members[RTE_MAX_ETHPORTS];
 	char pmd_name[RTE_ETH_NAME_MAX_LEN];
 
-	test_add_member_to_bonding_device();
+	TEST_ASSERT_SUCCESS(test_add_member_to_bonding_device(),
+			"Failed to add member to bonding device");
 
 	current_member_count = rte_eth_bond_members_get(test_params->bonding_port_id,
 			members, RTE_MAX_ETHPORTS);
-- 
2.39.1

Re: [PATCH 3/3] app/test: fix checking return value

[Ferruh Yigit]

On 10/10/2023 7:23 AM, Chaoyong He wrote:
> From: Long Wu <long.wu@corigine.com>
> 
> CI found the function without checking return value in this place.
> 
> Coverity issue: 403101
> Fixes: 92073ef961ee ("bond: unit tests")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.

Re: [PATCH 0/3] Fix three coverity issues of bond PMD

[Ferruh Yigit]

On 10/10/2023 7:23 AM, Chaoyong He wrote:
> This patch series fix three coverity issues about bond PMD and test
> case, 403097, 403099 and 403101.
> 
> Long Wu (3):
>   app/test: fix control flow issue
>   net/bonding: fix illegal memory accesses
>   app/test: fix checking return value
> 

Except from 2/3,
Series applied to dpdk-next-net/main, thanks.