DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode
@ 2020-07-24 12:29 Dinghao Liu
  2020-07-24 13:28 ` Greg Kroah-Hartman
  2020-07-27 11:44 ` Dan Carpenter
  0 siblings, 2 replies; 7+ messages in thread
From: Dinghao Liu @ 2020-07-24 12:29 UTC (permalink / raw)
  To: dinghao.liu, kjlu
  Cc: devel, Greg Kroah-Hartman, linux-kernel, Julia Lawall,
	Stefano Brivio, Shreeya Patel, Larry Finger

The variable authmode will keep uninitialized if neither if
statements used to initialize this variable are not triggered.
Then authmode may contain a garbage value and influence the
execution flow of this function.

Fix this by initializing it to zero.

Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
---
 drivers/staging/rtl8188eu/core/rtw_mlme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme.c b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 9de2d421f6b1..716f8d8a5c13 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme.c
@@ -1711,7 +1711,7 @@ static int rtw_append_pmkid(struct adapter *Adapter, int iEntry, u8 *ie, uint ie
 
 int rtw_restruct_sec_ie(struct adapter *adapter, u8 *in_ie, u8 *out_ie, uint in_len)
 {
-	u8 authmode;
+	u8 authmode = 0;
 	uint ielength;
 	int iEntry;
 	struct mlme_priv *pmlmepriv = &adapter->mlmepriv;
-- 
2.17.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

* Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode
  2020-07-24 12:29 [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode Dinghao Liu
@ 2020-07-24 13:28 ` Greg Kroah-Hartman
  2020-07-24 17:56   ` Larry Finger
  2020-07-27 11:44 ` Dan Carpenter
  1 sibling, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-24 13:28 UTC (permalink / raw)
  To: Dinghao Liu
  Cc: devel, kjlu, linux-kernel, Julia Lawall, Stefano Brivio,
	Shreeya Patel, Larry Finger

On Fri, Jul 24, 2020 at 08:29:55PM +0800, Dinghao Liu wrote:
> The variable authmode will keep uninitialized if neither if
> statements used to initialize this variable are not triggered.
> Then authmode may contain a garbage value and influence the
> execution flow of this function.
> 
> Fix this by initializing it to zero.

That does not fix anything, you just now are potentially setting a value
you really do not have.

Are you sure that this variable really will never be set?  If so, please
fix it up with a "real" value that the code can handle properly.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

* Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode
  2020-07-24 13:28 ` Greg Kroah-Hartman
@ 2020-07-24 17:56   ` Larry Finger
  2020-07-27 13:39     ` dinghao.liu
  0 siblings, 1 reply; 7+ messages in thread
From: Larry Finger @ 2020-07-24 17:56 UTC (permalink / raw)
  To: Dinghao Liu
  Cc: devel, Greg Kroah-Hartman, kjlu, linux-kernel, Julia Lawall,
	Stefano Brivio, Shreeya Patel

On 7/24/20 8:28 AM, Dinghao Liu wrote:
> The variable authmode will keep uninitialized if neither if
> statements used to initialize this variable are not triggered.

Besides Greg's comment, you need to re-parse this sentence. I realize that 
English is probably not your first language, but this one is not what you meant.

You likely meant "The variable authmode will remain uninitialized if all 
statements used to initialize this variable are not triggered."

A possible (line-wrapped) patch to quiet the tools would be:

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme.c 
b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 9de2d421f6b1..9e4d78bc9a2e 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme.c
@@ -1729,9 +1729,11 @@ int rtw_restruct_sec_ie(struct adapter *adapter, u8 
*in_ie, u8 *out_ie, uint in_
         if ((ndisauthmode == Ndis802_11AuthModeWPA) ||
             (ndisauthmode == Ndis802_11AuthModeWPAPSK))
                 authmode = _WPA_IE_ID_;
-       if ((ndisauthmode == Ndis802_11AuthModeWPA2) ||
-           (ndisauthmode == Ndis802_11AuthModeWPA2PSK))
+       else if ((ndisauthmode == Ndis802_11AuthModeWPA2) ||
+                (ndisauthmode == Ndis802_11AuthModeWPA2PSK))
                 authmode = _WPA2_IE_ID_;
+       else
+               authmode = 0;

         if (check_fwstate(pmlmepriv, WIFI_UNDER_WPS)) {
                 memcpy(out_ie + ielength, psecuritypriv->wps_ie, 
psecuritypriv->wps_ie_len);


Yes, in this routine, it would be possible for authmode to not be set; however, 
later code only compares it to either _WPA_IE_ID_ or _WPA2_IE_ID_. It is never 
used in a way that an unset value could make the program flow be different by 
arbitrarily setting the value to zero. Thus your statement "Then authmode may 
contain a garbage value and influence the execution flow of this function." is 
false.

Larry
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

* Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode
  2020-07-24 12:29 [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode Dinghao Liu
  2020-07-24 13:28 ` Greg Kroah-Hartman
@ 2020-07-27 11:44 ` Dan Carpenter
  2020-07-27 13:23   ` Dan Carpenter
  1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-07-27 11:44 UTC (permalink / raw)
  To: Dinghao Liu
  Cc: devel, Greg Kroah-Hartman, kjlu, linux-kernel, Julia Lawall,
	Stefano Brivio, Shreeya Patel, Larry Finger

On Fri, Jul 24, 2020 at 08:29:55PM +0800, Dinghao Liu wrote:
> The variable authmode will keep uninitialized if neither if
> statements used to initialize this variable are not triggered.
> Then authmode may contain a garbage value and influence the
> execution flow of this function.
> 
> Fix this by initializing it to zero.
> 
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>

This matches how rtl8723bs does it.

Fixes: 7b464c9fa5cc ("staging: r8188eu: Add files for new driver - part 4")
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

There is a quirk/bug in GCC where it's initializing stuff to zero
sometimes instead of printing a compile warning so this probably doesn't
change run time very much.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

* Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode
  2020-07-27 11:44 ` Dan Carpenter
@ 2020-07-27 13:23   ` Dan Carpenter
  2020-07-27 14:45     ` dinghao.liu
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-07-27 13:23 UTC (permalink / raw)
  To: Dinghao Liu
  Cc: devel, Greg Kroah-Hartman, kjlu, linux-kernel, Julia Lawall,
	Stefano Brivio, Shreeya Patel, Larry Finger

I review things in the order that they appear in my inbox so I hadn't
seen Greg and Larry's comments.  You've now stumbled into an area of
politics where you have conflicting reviews...  :P  Fortunately, we're
all of us reasonable people.

I think your patch is correct in that it is what the original coder
intended.  You just need to sell the patch correctly in the commit
message.  The real danger of the original code would be if "authmode" is
accidentally 0x30 or 0xdd just because it was uninitialized.  Setting it
to zero ensures that it is not and it also matches how this is handled
in the rtl8723bs driver.  This matches the original author's intention.

So:

1) Re-write the commit message.

    The variable authmode can be uninitialized.  The danger would be
    if it equals _WPA_IE_ID_ (0xdd) or _WPA2_IE_ID_ (0x33).  We can
    avoid this by setting it to zero instead.  This is the approach that
    was used in the rtl8723bs driver.

2) Add a fixes tag.
   Fixes: 7b464c9fa5cc ("staging: r8188eu: Add files for new driver - part 4")

3) Change the commit to Larry's style with the "else if" and "else".
   Sometimes people just disagree about style so it's easiest to go with
   whatever the maintainer (Larry) wants.  It's not worth debating one
   way or the other so just redo it.

Then resend.  Google for "how to send a v2 patch" to get the right
format.

regards,
dan carpenter


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

* Re: Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode
  2020-07-24 17:56   ` Larry Finger
@ 2020-07-27 13:39     ` dinghao.liu
  0 siblings, 0 replies; 7+ messages in thread
From: dinghao.liu @ 2020-07-27 13:39 UTC (permalink / raw)
  To: Larry Finger
  Cc: devel, Greg Kroah-Hartman, kjlu, linux-kernel, Julia Lawall,
	Stefano Brivio, Shreeya Patel

> 
> Yes, in this routine, it would be possible for authmode to not be set; however, 
> later code only compares it to either _WPA_IE_ID_ or _WPA2_IE_ID_. It is never 
> used in a way that an unset value could make the program flow be different by 
> arbitrarily setting the value to zero. Thus your statement "Then authmode may 
> contain a garbage value and influence the execution flow of this function." is 
> false.
> 

It's clear to me. Thank you for your advice.

Regards,
Dinghao
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

* Re: Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode
  2020-07-27 13:23   ` Dan Carpenter
@ 2020-07-27 14:45     ` dinghao.liu
  0 siblings, 0 replies; 7+ messages in thread
From: dinghao.liu @ 2020-07-27 14:45 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: devel, Greg Kroah-Hartman, kjlu, linux-kernel, Julia Lawall,
	Stefano Brivio, Shreeya Patel, Larry Finger

> I review things in the order that they appear in my inbox so I hadn't
> seen Greg and Larry's comments.  You've now stumbled into an area of
> politics where you have conflicting reviews...  :P  Fortunately, we're
> all of us reasonable people.
> 
> I think your patch is correct in that it is what the original coder
> intended.  You just need to sell the patch correctly in the commit
> message.  The real danger of the original code would be if "authmode" is
> accidentally 0x30 or 0xdd just because it was uninitialized.  Setting it
> to zero ensures that it is not and it also matches how this is handled
> in the rtl8723bs driver.  This matches the original author's intention.
> 
> So:
> 
> 1) Re-write the commit message.
> 
>     The variable authmode can be uninitialized.  The danger would be
>     if it equals _WPA_IE_ID_ (0xdd) or _WPA2_IE_ID_ (0x33).  We can
>     avoid this by setting it to zero instead.  This is the approach that
>     was used in the rtl8723bs driver.
> 
> 2) Add a fixes tag.
>    Fixes: 7b464c9fa5cc ("staging: r8188eu: Add files for new driver - part 4")
> 
> 3) Change the commit to Larry's style with the "else if" and "else".
>    Sometimes people just disagree about style so it's easiest to go with
>    whatever the maintainer (Larry) wants.  It's not worth debating one
>    way or the other so just redo it.
> 
> Then resend.  Google for "how to send a v2 patch" to get the right
> format.
> 
> regards,
> dan carpenter
> 

Your advice is very helpful to me, thanks! I will prepare v2 patch and
resend it soon.

Regards,
Dinghao

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 12:29 [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode Dinghao Liu
2020-07-24 13:28 ` Greg Kroah-Hartman
2020-07-24 17:56   ` Larry Finger
2020-07-27 13:39     ` dinghao.liu
2020-07-27 11:44 ` Dan Carpenter
2020-07-27 13:23   ` Dan Carpenter
2020-07-27 14:45     ` dinghao.liu

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git