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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240613074857.66597de9@kernel.org>
Date: Thu, 13 Jun 2024 07:48:57 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Ghadi Elie Rahme <ghadi.rahme@...onical.com>
Cc: netdev@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2 net] bnx2x: Fix multiple UBSAN
 array-index-out-of-bounds

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@...onical.com>
> Cc: stable@...r.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