Linux kernel staging patches
 help / color / Atom feed
* staging: rtl8723bs mask values in wpa_set_auth_algs
@ 2021-07-09 14:53 Colin Ian King
  2021-07-15 10:08 ` Fabio Aiuto
  2021-07-15 14:57 ` [PATCH] staging: rtl8723bs: fix wpa_set_auth_algs() function Fabio Aiuto
  0 siblings, 2 replies; 3+ messages in thread
From: Colin Ian King @ 2021-07-09 14:53 UTC (permalink / raw)
  To: Quytelda Kahja; +Cc: Greg Kroah-Hartman, linux-staging, linux-kernel

Hi,

Static analysis with Coverity has found an issue introduced by the
following commit:

commit 5befa937e8daaebcde81b9423eb93f3ff2e918f7
Author: Quytelda Kahja <quytelda@tamalin.org>
Date:   Tue Mar 27 01:41:02 2018 -0700

    staging: rtl8723bs: Fix IEEE80211 authentication algorithm constants.

The analysis is as follows:

347 static int wpa_set_auth_algs(struct net_device *dev, u32 value)
348 {
349        struct adapter *padapter = rtw_netdev_priv(dev);
350        int ret = 0;
351
352        if ((value & WLAN_AUTH_SHARED_KEY) && (value & WLAN_AUTH_OPEN)) {

Logically dead code (DEADCODE)

353                padapter->securitypriv.ndisencryptstatus =
Ndis802_11Encryption1Enabled;
354                padapter->securitypriv.ndisauthtype =
Ndis802_11AuthModeAutoSwitch;
355                padapter->securitypriv.dot11AuthAlgrthm =
dot11AuthAlgrthm_Auto;
356        } else if (value & WLAN_AUTH_SHARED_KEY)        {
357                padapter->securitypriv.ndisencryptstatus =
Ndis802_11Encryption1Enabled;
358
359                padapter->securitypriv.ndisauthtype =
Ndis802_11AuthModeShared;
360                padapter->securitypriv.dot11AuthAlgrthm =
dot11AuthAlgrthm_Shared;
   dead_error_condition: The condition value & 0U cannot be true.
361        } else if (value & WLAN_AUTH_OPEN) {
362                /* padapter->securitypriv.ndisencryptstatus =
Ndis802_11EncryptionDisabled; */

Logically dead code (DEADCODE)

363                if (padapter->securitypriv.ndisauthtype <
Ndis802_11AuthModeWPAPSK) {
364                        padapter->securitypriv.ndisauthtype =
Ndis802_11AuthModeOpen;
365                        padapter->securitypriv.dot11AuthAlgrthm =
dot11AuthAlgrthm_Open;
366                }
367        } else {
368                ret = -EINVAL;
369        }
370
371        return ret;
372
373 }

The deadcode warnings occur because the mask value of WLAN_AUTH_OPEN is
defined in /include/linux/ieee80211.h as:

#define WLAN_AUTO_OPEN 0

and so any value masked with 0 is 0 and hence that part of the if
statement is always false.

The original code was using the mask values:

AUTH_ALG_SHARED_KEY and also AUTH_ALG_OPEN_SYSTEM that are defined as:

#define AUTH_ALG_OPEN_SYSTEM                   0x1
#define AUTH_ALG_SHARED_KEY                    0x2

So this appears to be a regression. I'm not sure the best approach for a
suitable fix to use the intended macros in ieee80211.h

Colin




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

* Re: staging: rtl8723bs mask values in wpa_set_auth_algs
  2021-07-09 14:53 staging: rtl8723bs mask values in wpa_set_auth_algs Colin Ian King
@ 2021-07-15 10:08 ` Fabio Aiuto
  2021-07-15 14:57 ` [PATCH] staging: rtl8723bs: fix wpa_set_auth_algs() function Fabio Aiuto
  1 sibling, 0 replies; 3+ messages in thread
From: Fabio Aiuto @ 2021-07-15 10:08 UTC (permalink / raw)
  To: Colin Ian King
  Cc: Quytelda Kahja, Greg Kroah-Hartman, linux-staging, linux-kernel

Hello Colin,

On Fri, Jul 09, 2021 at 03:53:39PM +0100, Colin Ian King wrote:
> Hi,
> 
> Static analysis with Coverity has found an issue introduced by the
> following commit:
> 
> commit 5befa937e8daaebcde81b9423eb93f3ff2e918f7
> Author: Quytelda Kahja <quytelda@tamalin.org>
> Date:   Tue Mar 27 01:41:02 2018 -0700
> 
>     staging: rtl8723bs: Fix IEEE80211 authentication algorithm constants.
> 
> The analysis is as follows:
> 
> 347 static int wpa_set_auth_algs(struct net_device *dev, u32 value)
> 348 {
> 349        struct adapter *padapter = rtw_netdev_priv(dev);
> 350        int ret = 0;
> 351
> 352        if ((value & WLAN_AUTH_SHARED_KEY) && (value & WLAN_AUTH_OPEN)) {
> 
> Logically dead code (DEADCODE)
> 
> 353                padapter->securitypriv.ndisencryptstatus =
> Ndis802_11Encryption1Enabled;
> 354                padapter->securitypriv.ndisauthtype =
> Ndis802_11AuthModeAutoSwitch;
> 355                padapter->securitypriv.dot11AuthAlgrthm =
> dot11AuthAlgrthm_Auto;
> 356        } else if (value & WLAN_AUTH_SHARED_KEY)        {
> 357                padapter->securitypriv.ndisencryptstatus =
> Ndis802_11Encryption1Enabled;
> 358
> 359                padapter->securitypriv.ndisauthtype =
> Ndis802_11AuthModeShared;
> 360                padapter->securitypriv.dot11AuthAlgrthm =
> dot11AuthAlgrthm_Shared;
>    dead_error_condition: The condition value & 0U cannot be true.
> 361        } else if (value & WLAN_AUTH_OPEN) {
> 362                /* padapter->securitypriv.ndisencryptstatus =
> Ndis802_11EncryptionDisabled; */
> 
> Logically dead code (DEADCODE)
> 
> 363                if (padapter->securitypriv.ndisauthtype <
> Ndis802_11AuthModeWPAPSK) {
> 364                        padapter->securitypriv.ndisauthtype =
> Ndis802_11AuthModeOpen;
> 365                        padapter->securitypriv.dot11AuthAlgrthm =
> dot11AuthAlgrthm_Open;
> 366                }
> 367        } else {
> 368                ret = -EINVAL;
> 369        }
> 370
> 371        return ret;
> 372
> 373 }
> 
> The deadcode warnings occur because the mask value of WLAN_AUTH_OPEN is
> defined in /include/linux/ieee80211.h as:
> 
> #define WLAN_AUTO_OPEN 0
> 
> and so any value masked with 0 is 0 and hence that part of the if
> statement is always false.
> 
> The original code was using the mask values:
> 
> AUTH_ALG_SHARED_KEY and also AUTH_ALG_OPEN_SYSTEM that are defined as:
> 
> #define AUTH_ALG_OPEN_SYSTEM                   0x1
> #define AUTH_ALG_SHARED_KEY                    0x2

I think that the correct macros are:

/* IW_AUTH_80211_AUTH_ALG values (bit field) */
#define IW_AUTH_ALG_OPEN_SYSTEM 0x00000001
#define IW_AUTH_ALG_SHARED_KEY  0x00000002
#define IW_AUTH_ALG_LEAP        0x00000004

defined in include/uapi/linux/wireless.h

> 
> So this appears to be a regression. I'm not sure the best approach for a
> suitable fix to use the intended macros in ieee80211.h
> 
> Colin
> 
> 
> 
> 

thank you,

fabio

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

* [PATCH] staging: rtl8723bs: fix wpa_set_auth_algs() function
  2021-07-09 14:53 staging: rtl8723bs mask values in wpa_set_auth_algs Colin Ian King
  2021-07-15 10:08 ` Fabio Aiuto
@ 2021-07-15 14:57 ` Fabio Aiuto
  1 sibling, 0 replies; 3+ messages in thread
From: Fabio Aiuto @ 2021-07-15 14:57 UTC (permalink / raw)
  To: gregkh
  Cc: hdegoede, Larry.Finger, linux-staging, linux-kernel, Colin Ian King

fix authentication algorithm constants.
wpa_set_auth_algs() function contains some conditional
statements masking the checked value with the wrong
constants. This produces some unintentional dead code.
Mask the value with the right macros.

Fixes: 5befa937e8da ("staging: rtl8723bs: Fix IEEE80211 authentication algorithm constants.")
Reported-by: Colin Ian King <colin.king@canonical.com>
Tested-on: Lenovo Ideapad MiiX 300-10IBY
Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
---
 drivers/staging/rtl8723bs/os_dep/ioctl_linux.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
index f95000df8942..965558516cbd 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
@@ -349,16 +349,16 @@ static int wpa_set_auth_algs(struct net_device *dev, u32 value)
 	struct adapter *padapter = rtw_netdev_priv(dev);
 	int ret = 0;
 
-	if ((value & WLAN_AUTH_SHARED_KEY) && (value & WLAN_AUTH_OPEN)) {
+	if ((value & IW_AUTH_ALG_SHARED_KEY) && (value & IW_AUTH_ALG_OPEN_SYSTEM)) {
 		padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption1Enabled;
 		padapter->securitypriv.ndisauthtype = Ndis802_11AuthModeAutoSwitch;
 		padapter->securitypriv.dot11AuthAlgrthm = dot11AuthAlgrthm_Auto;
-	} else if (value & WLAN_AUTH_SHARED_KEY)	{
+	} else if (value & IW_AUTH_ALG_SHARED_KEY)	{
 		padapter->securitypriv.ndisencryptstatus = Ndis802_11Encryption1Enabled;
 
 		padapter->securitypriv.ndisauthtype = Ndis802_11AuthModeShared;
 		padapter->securitypriv.dot11AuthAlgrthm = dot11AuthAlgrthm_Shared;
-	} else if (value & WLAN_AUTH_OPEN) {
+	} else if (value & IW_AUTH_ALG_OPEN_SYSTEM) {
 		/* padapter->securitypriv.ndisencryptstatus = Ndis802_11EncryptionDisabled; */
 		if (padapter->securitypriv.ndisauthtype < Ndis802_11AuthModeWPAPSK) {
 			padapter->securitypriv.ndisauthtype = Ndis802_11AuthModeOpen;
-- 
2.20.1


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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-09 14:53 staging: rtl8723bs mask values in wpa_set_auth_algs Colin Ian King
2021-07-15 10:08 ` Fabio Aiuto
2021-07-15 14:57 ` [PATCH] staging: rtl8723bs: fix wpa_set_auth_algs() function Fabio Aiuto

Linux kernel staging patches

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-staging/0 linux-staging/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 linux-staging linux-staging/ https://lore.kernel.org/linux-staging \
		linux-staging@lists.linux.dev
	public-inbox-index linux-staging

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/dev.linux.lists.linux-staging


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