[PATCH 1/2] staging: rtl8723bs: Fix key-store index handling

[PATCH 1/2] staging: rtl8723bs: Fix key-store index handling

[Hans de Goede]

There are 2 issues with the key-store index handling

1. The non WEP key stores can store keys with indexes 0 - BIP_MAX_KEYID,
   this means that they should be an array with BIP_MAX_KEYID + 1
   entries. But some of the arrays where just BIP_MAX_KEYID entries
   big. While one other array was hardcoded to a size of 6 entries,
   instead of using the BIP_MAX_KEYID define.

2. The rtw_cfg80211_set_encryption() and wpa_set_encryption() functions
   index check where checking that the passed in key-index would fit
   inside both the WEP key store (which only has 4 entries) as well as
   in the non WEP key stores. This breaks any attempts to set non WEP
   keys with index 4 or 5.

Issue 2. specifically breaks wifi connection with some access points
which advertise PMF support. Without this fix connecting to these
access points fails with the following wpa_supplicant messages:

 nl80211: kernel reports: key addition failed
 wlan0: WPA: Failed to configure IGTK to the driver
 wlan0: RSN: Failed to configure IGTK
 wlan0: CTRL-EVENT-DISCONNECTED bssid=... reason=1 locally_generated=1

Fix 1. by using the right size for the key-stores. After this 2. can
safely be fixed by checking the right max-index value depending on the
used algorithm, fixing wifi not working with some PMF capable APs.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/rtl8723bs/include/rtw_security.h  |  8 ++---
 .../staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 26 ++++++++-------
 .../staging/rtl8723bs/os_dep/ioctl_linux.c    | 33 ++++++++++---------
 3 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/rtl8723bs/include/rtw_security.h b/drivers/staging/rtl8723bs/include/rtw_security.h
index a68b73858462..7587fa888527 100644
--- a/drivers/staging/rtl8723bs/include/rtw_security.h
+++ b/drivers/staging/rtl8723bs/include/rtw_security.h
@@ -107,13 +107,13 @@ struct security_priv {
 
 	u32 dot118021XGrpPrivacy;	/*  This specify the privacy algthm. used for Grp key */
 	u32 dot118021XGrpKeyid;		/*  key id used for Grp Key (tx key index) */
-	union Keytype	dot118021XGrpKey[BIP_MAX_KEYID];	/*  802.1x Group Key, for inx0 and inx1 */
-	union Keytype	dot118021XGrptxmickey[BIP_MAX_KEYID];
-	union Keytype	dot118021XGrprxmickey[BIP_MAX_KEYID];
+	union Keytype	dot118021XGrpKey[BIP_MAX_KEYID + 1];	/*  802.1x Group Key, for inx0 and inx1 */
+	union Keytype	dot118021XGrptxmickey[BIP_MAX_KEYID + 1];
+	union Keytype	dot118021XGrprxmickey[BIP_MAX_KEYID + 1];
 	union pn48		dot11Grptxpn;			/*  PN48 used for Grp Key xmit. */
 	union pn48		dot11Grprxpn;			/*  PN48 used for Grp Key recv. */
 	u32 dot11wBIPKeyid;						/*  key id used for BIP Key (tx key index) */
-	union Keytype	dot11wBIPKey[6];		/*  BIP Key, for index4 and index5 */
+	union Keytype	dot11wBIPKey[BIP_MAX_KEYID + 1];	/*  BIP Key, for index4 and index5 */
 	union pn48		dot11wBIPtxpn;			/*  PN48 used for Grp Key xmit. */
 	union pn48		dot11wBIPrxpn;			/*  PN48 used for Grp Key recv. */
 
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 54004f846cf0..3aba4e6eec8a 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -711,6 +711,7 @@ static int rtw_cfg80211_ap_set_encryption(struct net_device *dev, struct ieee_pa
 static int rtw_cfg80211_set_encryption(struct net_device *dev, struct ieee_param *param, u32 param_len)
 {
 	int ret = 0;
+	u8 max_idx;
 	u32 wep_key_idx, wep_key_len;
 	struct adapter *padapter = rtw_netdev_priv(dev);
 	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
@@ -724,26 +725,29 @@ static int rtw_cfg80211_set_encryption(struct net_device *dev, struct ieee_param
 		goto exit;
 	}
 
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff) {
-		if (param->u.crypt.idx >= WEP_KEYS
-			|| param->u.crypt.idx >= BIP_MAX_KEYID) {
-			ret = -EINVAL;
-			goto exit;
-		}
-	} else {
-		{
+	if (param->sta_addr[0] != 0xff || param->sta_addr[1] != 0xff ||
+	    param->sta_addr[2] != 0xff || param->sta_addr[3] != 0xff ||
+	    param->sta_addr[4] != 0xff || param->sta_addr[5] != 0xff) {
 		ret = -EINVAL;
 		goto exit;
 	}
+
+	if (strcmp(param->u.crypt.alg, "WEP") == 0)
+		max_idx = WEP_KEYS - 1;
+	else
+		max_idx = BIP_MAX_KEYID;
+
+	if (param->u.crypt.idx > max_idx) {
+		netdev_err(dev, "Error crypt.idx %d > %d\n", param->u.crypt.idx, max_idx);
+		ret = -EINVAL;
+		goto exit;
 	}
 
 	if (strcmp(param->u.crypt.alg, "WEP") == 0) {
 		wep_key_idx = param->u.crypt.idx;
 		wep_key_len = param->u.crypt.key_len;
 
-		if ((wep_key_idx >= WEP_KEYS) || (wep_key_len <= 0)) {
+		if (wep_key_len <= 0) {
 			ret = -EINVAL;
 			goto exit;
 		}
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
index 30374a820496..40a3157fb735 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
@@ -46,6 +46,7 @@ static int wpa_set_auth_algs(struct net_device *dev, u32 value)
 static int wpa_set_encryption(struct net_device *dev, struct ieee_param *param, u32 param_len)
 {
 	int ret = 0;
+	u8 max_idx;
 	u32 wep_key_idx, wep_key_len, wep_total_len;
 	struct ndis_802_11_wep	 *pwep = NULL;
 	struct adapter *padapter = rtw_netdev_priv(dev);
@@ -60,19 +61,22 @@ static int wpa_set_encryption(struct net_device *dev, struct ieee_param *param,
 		goto exit;
 	}
 
-	if (param->sta_addr[0] == 0xff && param->sta_addr[1] == 0xff &&
-	    param->sta_addr[2] == 0xff && param->sta_addr[3] == 0xff &&
-	    param->sta_addr[4] == 0xff && param->sta_addr[5] == 0xff) {
-		if (param->u.crypt.idx >= WEP_KEYS ||
-		    param->u.crypt.idx >= BIP_MAX_KEYID) {
-			ret = -EINVAL;
-			goto exit;
-		}
-	} else {
-		{
-			ret = -EINVAL;
-			goto exit;
-		}
+	if (param->sta_addr[0] != 0xff || param->sta_addr[1] != 0xff ||
+	    param->sta_addr[2] != 0xff || param->sta_addr[3] != 0xff ||
+	    param->sta_addr[4] != 0xff || param->sta_addr[5] != 0xff) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	if (strcmp(param->u.crypt.alg, "WEP") == 0)
+		max_idx = WEP_KEYS - 1;
+	else
+		max_idx = BIP_MAX_KEYID;
+
+	if (param->u.crypt.idx > max_idx) {
+		netdev_err(dev, "Error crypt.idx %d > %d\n", param->u.crypt.idx, max_idx);
+		ret = -EINVAL;
+		goto exit;
 	}
 
 	if (strcmp(param->u.crypt.alg, "WEP") == 0) {
@@ -84,9 +88,6 @@ static int wpa_set_encryption(struct net_device *dev, struct ieee_param *param,
 		wep_key_idx = param->u.crypt.idx;
 		wep_key_len = param->u.crypt.key_len;
 
-		if (wep_key_idx > WEP_KEYS)
-			return -EINVAL;
-
 		if (wep_key_len > 0) {
 			wep_key_len = wep_key_len <= 5 ? 5 : 13;
 			wep_total_len = wep_key_len + FIELD_OFFSET(struct ndis_802_11_wep, key_material);
-- 
2.39.1

[PATCH 2/2] staging: rtl8723bs: Pass correct parameters to cfg80211_get_bss()

[Hans de Goede]

To last 2 parameters to cfg80211_get_bss() should be of
the enum ieee80211_bss_type resp. enum ieee80211_privacy types,
which WLAN_CAPABILITY_ESS very much is not.

Fix both cfg80211_get_bss() calls in ioctl_cfg80211.c to pass
the right parameters.

Note that the second call was already somewhat fixed by commenting
out WLAN_CAPABILITY_ESS and passing in 0 instead. This was still
not entirely correct though since that would limit returned
BSS-es to ESS type BSS-es with privacy on.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 3aba4e6eec8a..84a9f4dd8f95 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -350,7 +350,7 @@ int rtw_cfg80211_check_bss(struct adapter *padapter)
 	bss = cfg80211_get_bss(padapter->rtw_wdev->wiphy, notify_channel,
 			pnetwork->mac_address, pnetwork->ssid.ssid,
 			pnetwork->ssid.ssid_length,
-			WLAN_CAPABILITY_ESS, WLAN_CAPABILITY_ESS);
+			IEEE80211_BSS_TYPE_ANY, IEEE80211_PRIVACY_ANY);
 
 	cfg80211_put_bss(padapter->rtw_wdev->wiphy, bss);
 
@@ -1139,8 +1139,8 @@ void rtw_cfg80211_unlink_bss(struct adapter *padapter, struct wlan_network *pnet
 
 	bss = cfg80211_get_bss(wiphy, NULL/*notify_channel*/,
 		select_network->mac_address, select_network->ssid.ssid,
-		select_network->ssid.ssid_length, 0/*WLAN_CAPABILITY_ESS*/,
-		0/*WLAN_CAPABILITY_ESS*/);
+		select_network->ssid.ssid_length, IEEE80211_BSS_TYPE_ANY,
+		IEEE80211_PRIVACY_ANY);
 
 	if (bss) {
 		cfg80211_unlink_bss(wiphy, bss);
-- 
2.39.1

Re: [PATCH 1/2] staging: rtl8723bs: Fix key-store index handling

[Bastien Nocera]

On Mon, 2023-03-06 at 16:35 +0100, Hans de Goede wrote:
> There are 2 issues with the key-store index handling
> 
> 1. The non WEP key stores can store keys with indexes 0 -
> BIP_MAX_KEYID,
>    this means that they should be an array with BIP_MAX_KEYID + 1
>    entries. But some of the arrays where just BIP_MAX_KEYID entries
>    big. While one other array was hardcoded to a size of 6 entries,
>    instead of using the BIP_MAX_KEYID define.
> 
> 2. The rtw_cfg80211_set_encryption() and wpa_set_encryption()
> functions
>    index check where checking that the passed in key-index would fit
>    inside both the WEP key store (which only has 4 entries) as well
> as
>    in the non WEP key stores. This breaks any attempts to set non WEP
>    keys with index 4 or 5.
> 
> Issue 2. specifically breaks wifi connection with some access points
> which advertise PMF support. Without this fix connecting to these
> access points fails with the following wpa_supplicant messages:
> 
>  nl80211: kernel reports: key addition failed
>  wlan0: WPA: Failed to configure IGTK to the driver
>  wlan0: RSN: Failed to configure IGTK
>  wlan0: CTRL-EVENT-DISCONNECTED bssid=... reason=1
> locally_generated=1
> 
> Fix 1. by using the right size for the key-stores. After this 2. can
> safely be fixed by checking the right max-index value depending on
> the
> used algorithm, fixing wifi not working with some PMF capable APs.

Good job on both those patches.

Can you please also CC: the maintainer of r8188eu which looks like it
has similar code?

Cheers

Re: [PATCH 1/2] staging: rtl8723bs: Fix key-store index handling

[Hans de Goede]

Hi,

On 3/6/23 16:40, Bastien Nocera wrote:
> On Mon, 2023-03-06 at 16:35 +0100, Hans de Goede wrote:
>> There are 2 issues with the key-store index handling
>>
>> 1. The non WEP key stores can store keys with indexes 0 -
>> BIP_MAX_KEYID,
>>    this means that they should be an array with BIP_MAX_KEYID + 1
>>    entries. But some of the arrays where just BIP_MAX_KEYID entries
>>    big. While one other array was hardcoded to a size of 6 entries,
>>    instead of using the BIP_MAX_KEYID define.
>>
>> 2. The rtw_cfg80211_set_encryption() and wpa_set_encryption()
>> functions
>>    index check where checking that the passed in key-index would fit
>>    inside both the WEP key store (which only has 4 entries) as well
>> as
>>    in the non WEP key stores. This breaks any attempts to set non WEP
>>    keys with index 4 or 5.
>>
>> Issue 2. specifically breaks wifi connection with some access points
>> which advertise PMF support. Without this fix connecting to these
>> access points fails with the following wpa_supplicant messages:
>>
>>  nl80211: kernel reports: key addition failed
>>  wlan0: WPA: Failed to configure IGTK to the driver
>>  wlan0: RSN: Failed to configure IGTK
>>  wlan0: CTRL-EVENT-DISCONNECTED bssid=... reason=1
>> locally_generated=1
>>
>> Fix 1. by using the right size for the key-stores. After this 2. can
>> safely be fixed by checking the right max-index value depending on
>> the
>> used algorithm, fixing wifi not working with some PMF capable APs.
> 
> Good job on both those patches.
> 
> Can you please also CC: the maintainer of r8188eu which looks like it
> has similar code?

Done (added to the To: of this reply).

Note I have heard that the r8188eu is now (starting with 6.2 ?) supported
by one of the non staging realtek wifi drivers. So I think that maybe it
can just be removed from staging altogether ?

Regards,

Hans

the future of r8188eu (was: Re: [PATCH 1/2] staging: rtl8723bs: Fix key-store index handling)

[Martin Kaiser]

Hi all,

Thus wrote Hans de Goede (hdegoede@redhat.com):

> Note I have heard that the r8188eu is now (starting with 6.2 ?) supported
> by one of the non staging realtek wifi drivers. So I think that maybe it
> can just be removed from staging altogether ?

thanks for bringing this up.

Indeed, the r8xxxu driver does now support the rtl8188eu chipset. The
code for this chip has landed in Linus' tree for 6.3-rc1.

The r8xxxu driver uses mac80211 and seems to work well (at least for
me). The Kconfig entry says that features like power management are
missing, this is supported by r8188eu.

So this raises the question about the future of r8188eu. It might have
more features, which are probably not well tested, but its integration
into the rest of the kernel is far behind rtl8xxxu. A lot of people (myself
included) have submitted cleanups for r8188eu and improved it. Still,
when 6.3 is released, it's likely that the users of rtl8188eu-based
dongles will switch to rtl8xxxu.

I guess we might have to bite the bullet and give up on r8188eu...

What do you think?

Thanks,
Martin

Re: the future of r8188eu (was: Re: [PATCH 1/2] staging: rtl8723bs: Fix key-store index handling)

[Greg Kroah-Hartman]

On Tue, Mar 07, 2023 at 09:40:10PM +0100, Martin Kaiser wrote:
> Hi all,
> 
> Thus wrote Hans de Goede (hdegoede@redhat.com):
> 
> > Note I have heard that the r8188eu is now (starting with 6.2 ?) supported
> > by one of the non staging realtek wifi drivers. So I think that maybe it
> > can just be removed from staging altogether ?
> 
> thanks for bringing this up.
> 
> Indeed, the r8xxxu driver does now support the rtl8188eu chipset. The
> code for this chip has landed in Linus' tree for 6.3-rc1.
> 
> The r8xxxu driver uses mac80211 and seems to work well (at least for
> me). The Kconfig entry says that features like power management are
> missing, this is supported by r8188eu.
> 
> So this raises the question about the future of r8188eu. It might have
> more features, which are probably not well tested, but its integration
> into the rest of the kernel is far behind rtl8xxxu. A lot of people (myself
> included) have submitted cleanups for r8188eu and improved it. Still,
> when 6.3 is released, it's likely that the users of rtl8188eu-based
> dongles will switch to rtl8xxxu.
> 
> I guess we might have to bite the bullet and give up on r8188eu...
> 
> What do you think?

Yes, we need to drop the staging driver if there is a "real" kernel
driver that supports the same hardware.  We can't have multiple drivers
in the kernel that try to bind to the same device, we've done that in
the past and it causes nothing but problems.

I'll go make up a patch now to drop the staging driver, thanks for
letting me know.

greg k-h

Re: the future of r8188eu (was: Re: [PATCH 1/2] staging: rtl8723bs: Fix key-store index handling)

[Greg Kroah-Hartman]

On Wed, Mar 08, 2023 at 07:32:51AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 07, 2023 at 09:40:10PM +0100, Martin Kaiser wrote:
> > Hi all,
> > 
> > Thus wrote Hans de Goede (hdegoede@redhat.com):
> > 
> > > Note I have heard that the r8188eu is now (starting with 6.2 ?) supported
> > > by one of the non staging realtek wifi drivers. So I think that maybe it
> > > can just be removed from staging altogether ?
> > 
> > thanks for bringing this up.
> > 
> > Indeed, the r8xxxu driver does now support the rtl8188eu chipset. The
> > code for this chip has landed in Linus' tree for 6.3-rc1.
> > 
> > The r8xxxu driver uses mac80211 and seems to work well (at least for
> > me). The Kconfig entry says that features like power management are
> > missing, this is supported by r8188eu.
> > 
> > So this raises the question about the future of r8188eu. It might have
> > more features, which are probably not well tested, but its integration
> > into the rest of the kernel is far behind rtl8xxxu. A lot of people (myself
> > included) have submitted cleanups for r8188eu and improved it. Still,
> > when 6.3 is released, it's likely that the users of rtl8188eu-based
> > dongles will switch to rtl8xxxu.
> > 
> > I guess we might have to bite the bullet and give up on r8188eu...
> > 
> > What do you think?
> 
> Yes, we need to drop the staging driver if there is a "real" kernel
> driver that supports the same hardware.  We can't have multiple drivers
> in the kernel that try to bind to the same device, we've done that in
> the past and it causes nothing but problems.
> 
> I'll go make up a patch now to drop the staging driver, thanks for
> letting me know.

For those that didn't see it, the patch that removes the staging driver
is here:
	https://lore.kernel.org/r/20230308131934.380395-1-gregkh@linuxfoundation.org