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 <[email protected]>
---
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
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
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
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 <[email protected]>
This matches how rtl8723bs does it.
Fixes: 7b464c9fa5cc ("staging: r8188eu: Add files for new driver - part 4")
Reviewed-by: Dan Carpenter <[email protected]>
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
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
>
> 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
> 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