[v2,1/1] ethdev: fix int overflow in descriptor count logic
Checks
Commit Message
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(-)
Comments
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.
@@ -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>
@@ -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