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

Date:   Wed, 20 Apr 2022 12:42:20 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Rebecca Mckeever <remckee0@...il.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

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?

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?

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.