All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] staging: rtl8192u: compare strcmp result to zero
@ 2022-04-16 10:24 Rebecca Mckeever
  2022-04-20  9:42 ` Dan Carpenter
  0 siblings, 1 reply; 3+ messages in thread
From: Rebecca Mckeever @ 2022-04-16 10:24 UTC (permalink / raw)
  To: outreachy
  Cc: Greg Kroah-Hartman, linux-staging, linux-kernel, Rebecca Mckeever

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>
---
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;
2849 
2850         //set WEP40 first, it will be modified according to WEP104 or WEP40 at other place
2851         if (!strcmp(param->u.crypt.alg, "WEP"))
2852                 module = "ieee80211_crypt_wep";
2853         else if (!strcmp(param->u.crypt.alg, "TKIP"))
2854                 module = "ieee80211_crypt_tkip";
2855         else if (!strcmp(param->u.crypt.alg, "CCMP"))
2856                 module = "ieee80211_crypt_ccmp"; 

drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
index 9885917b9199..d6829cf6f7e3 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
@@ -688,9 +688,9 @@ int ieee80211_wx_get_encode_ext(struct ieee80211_device *ieee,
 	} else {
 		if (strcmp(crypt->ops->name, "WEP") == 0)
 			ext->alg = IW_ENCODE_ALG_WEP;
-		else if (strcmp(crypt->ops->name, "TKIP"))
+		else if (strcmp(crypt->ops->name, "TKIP") == 0)
 			ext->alg = IW_ENCODE_ALG_TKIP;
-		else if (strcmp(crypt->ops->name, "CCMP"))
+		else if (strcmp(crypt->ops->name, "CCMP") == 0)
 			ext->alg = IW_ENCODE_ALG_CCMP;
 		else
 			return -EINVAL;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] staging: rtl8192u: compare strcmp result to zero
  2022-04-16 10:24 [PATCH] staging: rtl8192u: compare strcmp result to zero Rebecca Mckeever
@ 2022-04-20  9:42 ` Dan Carpenter
  2022-04-21  1:16   ` Rebecca Mckeever
  0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-04-20  9:42 UTC (permalink / raw)
  To: Rebecca Mckeever
  Cc: outreachy, Greg Kroah-Hartman, linux-staging, linux-kernel

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?

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?

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.



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] staging: rtl8192u: compare strcmp result to zero
  2022-04-20  9:42 ` Dan Carpenter
@ 2022-04-21  1:16   ` Rebecca Mckeever
  0 siblings, 0 replies; 3+ messages in thread
From: Rebecca Mckeever @ 2022-04-21  1:16 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: outreachy, Greg Kroah-Hartman, linux-staging, linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-04-21  1:16 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-16 10:24 [PATCH] staging: rtl8192u: compare strcmp result to zero Rebecca Mckeever
2022-04-20  9:42 ` Dan Carpenter
2022-04-21  1:16   ` Rebecca Mckeever

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.