[PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[Dinghao Liu]

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

Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[Greg Kroah-Hartman]

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

Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[Larry Finger]

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

Re: Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[dinghao.liu]

> 
> 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

Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[Dan Carpenter]

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

Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[Dan Carpenter]

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

Re: Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode

[dinghao.liu]

> 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