linux-kernel - Re: [PATCH] staging: rtl8192u: compare strcmp result to zero

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] [day] [month] [year] [list]

Message-ID: <20220421011614.GA5118@sophie>
Date:   Wed, 20 Apr 2022 20:16:14 -0500
From:   Rebecca Mckeever <remckee0@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     outreachy@...ts.linux.dev,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8192u: compare strcmp result to zero

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@...il.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@...cle.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