Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Apr 2022 20:16:14 -0500
From:   Rebecca Mckeever <remckee0@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     outreachy@lists.linux.dev,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: rtl8192u: compare strcmp result to zero
Message-ID: <20220421011614.GA5118@sophie>
References: <20220416102434.97567-1-remckee0@gmail.com>
 <20220420094220.GD2951@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220420094220.GD2951@kadam>
Precedence: bulk

Hi Dan,

On Wed, Apr 20, 2022 at 12:42:20PM +0300, Dan Carpenter wrote:
> On Sat, Apr 16, 2022 at 05:24:34AM -0500, Rebecca Mckeever wrote:
> > Add " == 0" to the condition in both else if branches to address a
> > possible bug. strcmp returns 0 when its arguments are equal, which
> > evaluates to false, often leading to errors when used in if statements.
> > 
> > Currently, the statement in the first else if branch does not execute
> > when its arguments are equal, but it does execute when crypt->ops->name
> > equals any string other than "WEP" or "TKIP".
> > 
> > Similarly, the second else if branch does not execute when its arguments
> > are equal, and it only executes when crypt->ops->name equals "TKIP".
> > The else branch never executes.
> > 
> > It is unlikely that this is working as intended.
> > 
> > Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> > ---
> 
> Looks good.  How did you find this bug?

I noticed it when I was trying to understand the surrounding code when
preparing another patch.

> 
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> > There is a similiar issue in
> > drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > but I'm not sure if it's incorrect. The strcmp on line 2847 isn't
> > negated, but the ones on lines 2851, 2853, and 2855 are.
> > 
> > 2845         /* IPW HW cannot build TKIP MIC, host decryption still needed. */
> > 2846         if (!(ieee->host_encrypt || ieee->host_decrypt) &&
> > 2847             strcmp(param->u.crypt.alg, "TKIP"))
> > 2848                 goto skip_host_crypt;
> 
> You're right, but also I suspect this whole if statement is wrong.
> 
> The if statement is only triggered if both ->host_encrypt and
> ->host_decrypt are disabled.  (Too many negatives).  I think both are
> set in alloc_ieee80211() and rtl8192_init_priv_variable() so both are
> always true and the if statement is dead code.
> 
> How does the code match with the comment?
> 
> Fixing this probably requires testing.  Maybe we could add this to the
> TODO list or maybe add a comment?

There isn't currently a TODO file for this driver. Adding a TODO file
would probably be more effective than a comment?

> 
> regards,
> dan carpenter
> 
> Ps:  When you have a !(foo || bar) then it's often more readable to
> write it as !foo && !bar, but in this case it doesn't really answer any
> of the core questions so don't bother.
 
Yeah, I think you're right, !foo && !bar makes more sense to me.

Thanks,
Rebecca