Re: [PATCH v2 net] bnx2x: Fix multiple UBSAN array-index-out-of-bounds

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 12 Jun 2024 18:44:49 +0300 Ghadi Elie Rahme wrote:
> Fix UBSAN warnings that occur when using a system with 32 physical
> cpu cores or more, or when the user defines a number of Ethernet
> queues greater than or equal to FP_SB_MAX_E1x using the num_queues
> module parameter.
> 
> The value of the maximum number of Ethernet queues should be limited
> to FP_SB_MAX_E1x in case FCOE is disabled or to [FP_SB_MAX_E1x-1] if
> enabled to avoid out of bounds reads and writes.

You're just describing what the code does, not providing extra
context...

> Fixes: 7d0445d66a76 ("bnx2x: clamp num_queues to prevent passing a negative value")

Sure this is not more recent, netif_get_num_default_rss_queues()
used to always return 8.

> Signed-off-by: Ghadi Elie Rahme <ghadi.rahme@canonical.com>
> Cc: stable@vger.kernel.org

>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> index a8e07e51418f..c895dd680cf8 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -66,7 +66,12 @@ static int bnx2x_calc_num_queues(struct bnx2x *bp)
>  	if (is_kdump_kernel())
>  		nq = 1;
>  
> -	nq = clamp(nq, 1, BNX2X_MAX_QUEUES(bp));
> +	int max_nq = FP_SB_MAX_E1x - 1;

please don't mix declarations and code

> +	if (NO_FCOE(bp))
> +		max_nq = FP_SB_MAX_E1x;

you really need to explain somewhere why you're hardcoding E1x
constants while at a glance the driver also supports E2.
Also why is BNX2X_MAX_QUEUES() higher than the number of queues?
Isn't that the bug?
-- 
pw-bot: cr