diff mbox series

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

Message ID 20231010062304.205933-3-chaoyong.he@corigine.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series Fix three coverity issues of bond PMD |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chaoyong He Oct. 10, 2023, 6:23 a.m. UTC 
  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(+)

Comments

Ferruh Yigit Oct. 31, 2023, 2:51 p.m. UTC | #1 
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.
diff mbox series

Patch

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: