linux-kernel - Re: [PATCH] net: bnx2x: fix variable dereferenced before check

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: <YZYULWjK34xL6BeW@hovoldconsulting.com>
Date:   Thu, 18 Nov 2021 09:51:57 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Pavel Skripkin <paskripkin@...il.com>,
        Dan Carpenter <dan.carpenter@...cle.com>
Cc:     aelior@...vell.com, skalluru@...vell.com,
        GR-everest-linux-l2@...vell.com, davem@...emloft.net,
        kuba@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: bnx2x: fix variable dereferenced before check

[ Adding Dan. ]

On Sun, Nov 14, 2021 at 01:36:36AM +0300, Pavel Skripkin wrote:
> Smatch says:
> 	bnx2x_init_ops.h:640 bnx2x_ilt_client_mem_op()
> 	warn: variable dereferenced before check 'ilt' (see line 638)
> 
> Move ilt_cli variable initialization _after_ ilt validation, because
> it's unsafe to deref the pointer before validation check.

It seems smatch is confused here. There is no dereference happening
until after the check, we're just determining the address when
initialising ilt_cli.

I know this has been applied, and the change itself is fine, but the
patch description is wrong and the Fixes tag is unwarranted.
 
> Fixes: 523224a3b3cd ("bnx2x, cnic, bnx2i: use new FW/HSI")
> Signed-off-by: Pavel Skripkin <paskripkin@...il.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_init_ops.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_init_ops.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_init_ops.h
> index 1835d2e451c0..fc7fce642666 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_init_ops.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_init_ops.h
> @@ -635,11 +635,13 @@ static int bnx2x_ilt_client_mem_op(struct bnx2x *bp, int cli_num,
>  {
>  	int i, rc;
>  	struct bnx2x_ilt *ilt = BP_ILT(bp);
> -	struct ilt_client_info *ilt_cli = &ilt->clients[cli_num];
> +	struct ilt_client_info *ilt_cli;
>  
>  	if (!ilt || !ilt->lines)
>  		return -1;
>  
> +	ilt_cli = &ilt->clients[cli_num];
> +
>  	if (ilt_cli->flags & (ILT_CLIENT_SKIP_INIT | ILT_CLIENT_SKIP_MEM))
>  		return 0;

Johan