[PATCH v2 1/1] ethdev: fix int overflow in descriptor count logic

[PATCH v2 1/1] ethdev: fix int overflow in descriptor count logic

[Niall Meade]

Addressed a specific overflow issue in the eth_dev_adjust_nb_desc()
function where the uint16_t variable nb_desc would overflow when its
value was greater than (2^16 - nb_align). This overflow caused nb_desc
to incorrectly wrap around between 0 and nb_align-1, leading to the
function setting nb_desc to nb_min instead of the expected nb_max.

To give an example, let nb_desc=UINT16_MAX, nb_align=32, nb_max=4096 and
nb_min=64. RTE_ALIGN_CEIL(nb_desc, nb_align) calls
RTE_ALIGN_FLOOR(nb_desc + nb_align - 1, nb_align). This results in an
overflow of nb_desc, leading to nb_desc being set to 30 and then 0 when
the macros return. As a result of this, nb_desc is then set to nb_min
later on.

The resolution involves upcasting nb_desc to a uint32_t before the
RTE_ALIGN_CEIL macro is applied. This change ensures that the subsequent
call to RTE_ALIGN_FLOOR(nb_desc + (nb_align - 1), nb_align) does not
result in an overflow, as it would when nb_desc is a uint16_t. By using
a uint32_t for these operations, the correct behavior is maintained
without the risk of overflow.

Fixes: 0f67fc3baeb9 ("ethdev: add function to adjust number of descriptors")

Signed-off-by: Niall Meade <niall.meade@intel.com>

---
v2:
* add example of issue to commit message
---
 .mailmap                |  1 +
 lib/ethdev/rte_ethdev.c | 12 +++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/.mailmap b/.mailmap
index 4a508bafad..c1941e78bb 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1053,6 +1053,7 @@ Nelson Escobar <neescoba@cisco.com>
 Nemanja Marjanovic <nemanja.marjanovic@intel.com>
 Netanel Belgazal <netanel@amazon.com>
 Netanel Gonen <netanelg@mellanox.com>
+Niall Meade <niall.meade@intel.com>
 Niall Power <niall.power@intel.com>
 Nicholas Pratte <npratte@iol.unh.edu>
 Nick Connolly <nick.connolly@arm.com> <nick.connolly@mayadata.io>
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index f1c658f49e..f978283edf 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -6577,13 +6577,19 @@ static void
 eth_dev_adjust_nb_desc(uint16_t *nb_desc,
 		const struct rte_eth_desc_lim *desc_lim)
 {
+	/* Upcast to uint32 to avoid potential overflow with RTE_ALIGN_CEIL(). */
+	uint32_t nb_desc_32 = *nb_desc;
+
 	if (desc_lim->nb_align != 0)
-		*nb_desc = RTE_ALIGN_CEIL(*nb_desc, desc_lim->nb_align);
+		nb_desc_32 = RTE_ALIGN_CEIL(nb_desc_32, desc_lim->nb_align);
 
 	if (desc_lim->nb_max != 0)
-		*nb_desc = RTE_MIN(*nb_desc, desc_lim->nb_max);
+		nb_desc_32 = RTE_MIN(nb_desc_32, desc_lim->nb_max);
+
+	nb_desc_32 = RTE_MAX(nb_desc_32, desc_lim->nb_min);
 
-	*nb_desc = RTE_MAX(*nb_desc, desc_lim->nb_min);
+	/* Assign clipped u32 back to u16. */
+	*nb_desc = nb_desc_32;
 }
 
 int
-- 
2.34.1

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

Re: [PATCH v2 1/1] ethdev: fix int overflow in descriptor count logic

[Ferruh Yigit]

On 9/27/2024 2:23 PM, Niall Meade wrote:
> Addressed a specific overflow issue in the eth_dev_adjust_nb_desc()
> function where the uint16_t variable nb_desc would overflow when its
> value was greater than (2^16 - nb_align). This overflow caused nb_desc
> to incorrectly wrap around between 0 and nb_align-1, leading to the
> function setting nb_desc to nb_min instead of the expected nb_max.
> 
> To give an example, let nb_desc=UINT16_MAX, nb_align=32, nb_max=4096 and
> nb_min=64. RTE_ALIGN_CEIL(nb_desc, nb_align) calls
> RTE_ALIGN_FLOOR(nb_desc + nb_align - 1, nb_align). This results in an
> overflow of nb_desc, leading to nb_desc being set to 30 and then 0 when
> the macros return. As a result of this, nb_desc is then set to nb_min
> later on.
> 
> The resolution involves upcasting nb_desc to a uint32_t before the
> RTE_ALIGN_CEIL macro is applied. This change ensures that the subsequent
> call to RTE_ALIGN_FLOOR(nb_desc + (nb_align - 1), nb_align) does not
> result in an overflow, as it would when nb_desc is a uint16_t. By using
> a uint32_t for these operations, the correct behavior is maintained
> without the risk of overflow.
> 
> Fixes: 0f67fc3baeb9 ("ethdev: add function to adjust number of descriptors")
> 
> Signed-off-by: Niall Meade <niall.meade@intel.com>
> 
> ---
> v2:
> * add example of issue to commit message
> ---
>  .mailmap                |  1 +
>  lib/ethdev/rte_ethdev.c | 12 +++++++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/.mailmap b/.mailmap
> index 4a508bafad..c1941e78bb 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1053,6 +1053,7 @@ Nelson Escobar <neescoba@cisco.com>
>  Nemanja Marjanovic <nemanja.marjanovic@intel.com>
>  Netanel Belgazal <netanel@amazon.com>
>  Netanel Gonen <netanelg@mellanox.com>
> +Niall Meade <niall.meade@intel.com>
>  Niall Power <niall.power@intel.com>
>  Nicholas Pratte <npratte@iol.unh.edu>
>  Nick Connolly <nick.connolly@arm.com> <nick.connolly@mayadata.io>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index f1c658f49e..f978283edf 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6577,13 +6577,19 @@ static void
>  eth_dev_adjust_nb_desc(uint16_t *nb_desc,
>  		const struct rte_eth_desc_lim *desc_lim)
>  {
> +	/* Upcast to uint32 to avoid potential overflow with RTE_ALIGN_CEIL(). */
> +	uint32_t nb_desc_32 = *nb_desc;
> +
>  	if (desc_lim->nb_align != 0)
> -		*nb_desc = RTE_ALIGN_CEIL(*nb_desc, desc_lim->nb_align);
> +		nb_desc_32 = RTE_ALIGN_CEIL(nb_desc_32, desc_lim->nb_align);
>  
>  	if (desc_lim->nb_max != 0)
> -		*nb_desc = RTE_MIN(*nb_desc, desc_lim->nb_max);
> +		nb_desc_32 = RTE_MIN(nb_desc_32, desc_lim->nb_max);
> +
> +	nb_desc_32 = RTE_MAX(nb_desc_32, desc_lim->nb_min);
>  
> -	*nb_desc = RTE_MAX(*nb_desc, desc_lim->nb_min);
> +	/* Assign clipped u32 back to u16. */
> +	*nb_desc = nb_desc_32;
>

Better to add explicit cast here, in case compiler has '-Wconversion'
flag etc..

Except from above, lgtm.

[PATCH v3 1/1] ethdev: fix int overflow in descriptor count logic

[Niall Meade]

Addressed a specific overflow issue in the eth_dev_adjust_nb_desc()
function where the uint16_t variable nb_desc would overflow when its
value was greater than (2^16 - nb_align). This overflow caused nb_desc
to incorrectly wrap around between 0 and nb_align-1, leading to the
function setting nb_desc to nb_min instead of the expected nb_max.

To give an example, let nb_desc=UINT16_MAX, nb_align=32, nb_max=4096 and
nb_min=64. RTE_ALIGN_CEIL(nb_desc, nb_align) calls
RTE_ALIGN_FLOOR(nb_desc + nb_align - 1, nb_align). This results in an
overflow of nb_desc, leading to nb_desc being set to 30 and then 0 when
the macros return. As a result of this, nb_desc is then set to nb_min
later on.

The resolution involves upcasting nb_desc to a uint32_t before the
RTE_ALIGN_CEIL macro is applied. This change ensures that the subsequent
call to RTE_ALIGN_FLOOR(nb_desc + (nb_align - 1), nb_align) does not
result in an overflow, as it would when nb_desc is a uint16_t. By using
a uint32_t for these operations, the correct behavior is maintained
without the risk of overflow.

Fixes: 0f67fc3baeb9 ("ethdev: add function to adjust number of descriptors")

Signed-off-by: Niall Meade <niall.meade@intel.com>

---
v3:
* changed to explicit cast
v2:
* add example of issue to commit message
---
 .mailmap                |  1 +
 lib/ethdev/rte_ethdev.c | 12 +++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/.mailmap b/.mailmap
index 4a508bafad..c1941e78bb 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1053,6 +1053,7 @@ Nelson Escobar <neescoba@cisco.com>
 Nemanja Marjanovic <nemanja.marjanovic@intel.com>
 Netanel Belgazal <netanel@amazon.com>
 Netanel Gonen <netanelg@mellanox.com>
+Niall Meade <niall.meade@intel.com>
 Niall Power <niall.power@intel.com>
 Nicholas Pratte <npratte@iol.unh.edu>
 Nick Connolly <nick.connolly@arm.com> <nick.connolly@mayadata.io>
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index f1c658f49e..da1b831ff0 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -6577,13 +6577,19 @@ static void
 eth_dev_adjust_nb_desc(uint16_t *nb_desc,
 		const struct rte_eth_desc_lim *desc_lim)
 {
+	/* Upcast to uint32 to avoid potential overflow with RTE_ALIGN_CEIL(). */
+	uint32_t nb_desc_32 = (uint32_t)*nb_desc;
+
 	if (desc_lim->nb_align != 0)
-		*nb_desc = RTE_ALIGN_CEIL(*nb_desc, desc_lim->nb_align);
+		nb_desc_32 = RTE_ALIGN_CEIL(nb_desc_32, desc_lim->nb_align);
 
 	if (desc_lim->nb_max != 0)
-		*nb_desc = RTE_MIN(*nb_desc, desc_lim->nb_max);
+		nb_desc_32 = RTE_MIN(nb_desc_32, desc_lim->nb_max);
+
+	nb_desc_32 = RTE_MAX(nb_desc_32, desc_lim->nb_min);
 
-	*nb_desc = RTE_MAX(*nb_desc, desc_lim->nb_min);
+	/* Assign clipped u32 back to u16. */
+	*nb_desc = (uint16_t)nb_desc_32;
 }
 
 int
-- 
2.34.1

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

Re: [PATCH v3 1/1] ethdev: fix int overflow in descriptor count logic

[Ferruh Yigit]

On 9/30/2024 2:40 PM, Niall Meade wrote:
> Addressed a specific overflow issue in the eth_dev_adjust_nb_desc()
> function where the uint16_t variable nb_desc would overflow when its
> value was greater than (2^16 - nb_align). This overflow caused nb_desc
> to incorrectly wrap around between 0 and nb_align-1, leading to the
> function setting nb_desc to nb_min instead of the expected nb_max.
> 
> To give an example, let nb_desc=UINT16_MAX, nb_align=32, nb_max=4096 and
> nb_min=64. RTE_ALIGN_CEIL(nb_desc, nb_align) calls
> RTE_ALIGN_FLOOR(nb_desc + nb_align - 1, nb_align). This results in an
> overflow of nb_desc, leading to nb_desc being set to 30 and then 0 when
> the macros return. As a result of this, nb_desc is then set to nb_min
> later on.
> 
> The resolution involves upcasting nb_desc to a uint32_t before the
> RTE_ALIGN_CEIL macro is applied. This change ensures that the subsequent
> call to RTE_ALIGN_FLOOR(nb_desc + (nb_align - 1), nb_align) does not
> result in an overflow, as it would when nb_desc is a uint16_t. By using
> a uint32_t for these operations, the correct behavior is maintained
> without the risk of overflow.
> 
> Fixes: 0f67fc3baeb9 ("ethdev: add function to adjust number of descriptors")
> 
> Signed-off-by: Niall Meade <niall.meade@intel.com>
>

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

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