[PATCH 1/6] cfg80211: disallow shared key authentication with key index 4

[PATCH 1/6] cfg80211: disallow shared key authentication with key index 4

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Key index 4 can only be used for an IGTK, so the range checks
for shared key authentication should treat 4 as an error, fix
that in the code.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/mlme.c    | 2 +-
 net/wireless/nl80211.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index c284d883c349..d6abb0704db5 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -222,7 +222,7 @@ int cfg80211_mlme_auth(struct cfg80211_registered_device *rdev,
 	ASSERT_WDEV_LOCK(wdev);
 
 	if (auth_type == NL80211_AUTHTYPE_SHARED_KEY)
-		if (!key || !key_len || key_idx < 0 || key_idx > 4)
+		if (!key || !key_len || key_idx < 0 || key_idx > 3)
 			return -EINVAL;
 
 	if (wdev->current_bss &&
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 83c6445ebf33..c96e22b906af 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -7388,7 +7388,7 @@ static int nl80211_authenticate(struct sk_buff *skb, struct genl_info *info)
 		    (key.p.cipher != WLAN_CIPHER_SUITE_WEP104 ||
 		     key.p.key_len != WLAN_KEY_LEN_WEP104))
 			return -EINVAL;
-		if (key.idx > 4)
+		if (key.idx > 3)
 			return -EINVAL;
 	} else {
 		key.p.key_len = 0;
-- 
2.8.1

[PATCH 2/6] nl80211: fix connect keys range check

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Only key index 0-3 should be accepted, 4/5 are for IGTKs and
cannot be used as connect keys. Fix the range checking to not
allow such erroneous configurations.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/nl80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index c96e22b906af..6fe14b5d1af3 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -866,7 +866,7 @@ nl80211_parse_connkeys(struct cfg80211_registered_device *rdev,
 		err = -EINVAL;
 		if (!parse.p.key)
 			goto error;
-		if (parse.idx < 0 || parse.idx > 4)
+		if (parse.idx < 0 || parse.idx > 3)
 			goto error;
 		if (parse.def) {
 			if (def)
-- 
2.8.1

[PATCH 3/6] nl80211: only allow WEP keys during connect command

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This was already documented that way in nl80211.h, but the
parsing code still accepted other key types. Change it to
really only accept WEP keys as documented.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/nl80211.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 6fe14b5d1af3..739d0a780d83 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -881,16 +881,19 @@ nl80211_parse_connkeys(struct cfg80211_registered_device *rdev,
 						     parse.idx, false, NULL);
 		if (err)
 			goto error;
+		if (parse.p.cipher != WLAN_CIPHER_SUITE_WEP40 &&
+		    parse.p.cipher != WLAN_CIPHER_SUITE_WEP104) {
+			err = -EINVAL;
+			goto error;
+		}
 		result->params[parse.idx].cipher = parse.p.cipher;
 		result->params[parse.idx].key_len = parse.p.key_len;
 		result->params[parse.idx].key = result->data[parse.idx];
 		memcpy(result->data[parse.idx], parse.p.key, parse.p.key_len);
 
-		if (parse.p.cipher == WLAN_CIPHER_SUITE_WEP40 ||
-		    parse.p.cipher == WLAN_CIPHER_SUITE_WEP104) {
-			if (no_ht)
-				*no_ht = true;
-		}
+		/* must be WEP key if we got here */
+		if (no_ht)
+			*no_ht = true;
 	}
 
 	return result;
-- 
2.8.1

[PATCH 4/6] cfg80211: wext: only allow WEP keys to be configured before connected

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When not connected, anything but WEP keys shouldn't be allowed to be
configured for later - only static WEP keys make sense at this point.
Change wext to reject anything else just like nl80211 does.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/wext-compat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c
index 9f27221c8913..e45a76449b43 100644
--- a/net/wireless/wext-compat.c
+++ b/net/wireless/wext-compat.c
@@ -487,6 +487,9 @@ static int __cfg80211_set_encryption(struct cfg80211_registered_device *rdev,
 	err = 0;
 	if (wdev->current_bss)
 		err = rdev_add_key(rdev, dev, idx, pairwise, addr, params);
+	else if (params->cipher != WLAN_CIPHER_SUITE_WEP40 &&
+		 params->cipher != WLAN_CIPHER_SUITE_WEP104)
+		return -EINVAL;
 	if (err)
 		return err;
 
-- 
2.8.1

[PATCH 5/6] cfg80211: validate key index better

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Don't accept it if a key_idx < 0 snuck through, reject WEP keys with
key index 4 and 5 (which are used for IGTKs) and don't allow IGTKs
with key indices other than 4 and 5. This makes the key data match
expectations better.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/util.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index 0675f513e7b9..81fa16b36d30 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -218,7 +218,7 @@ int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
 				   struct key_params *params, int key_idx,
 				   bool pairwise, const u8 *mac_addr)
 {
-	if (key_idx > 5)
+	if (key_idx < 0 || key_idx > 5)
 		return -EINVAL;
 
 	if (!pairwise && mac_addr && !(rdev->wiphy.flags & WIPHY_FLAG_IBSS_RSN))
@@ -249,7 +249,13 @@ int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
 		/* Disallow BIP (group-only) cipher as pairwise cipher */
 		if (pairwise)
 			return -EINVAL;
+		if (key_idx < 4)
+			return -EINVAL;
 		break;
+	case WLAN_CIPHER_SUITE_WEP40:
+	case WLAN_CIPHER_SUITE_WEP104:
+		if (key_idx < 0 || key_idx > 3)
+			return -EINVAL;
 	default:
 		break;
 	}
-- 
2.8.1

[PATCH 6/6] cfg80211: reduce connect key caching struct size

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

After the previous patches, connect keys can only (correctly)
be used for storing static WEP keys. Therefore, remove all the
data for dealing with key index 4/5 and reduce the size of the
key material to the maximum for WEP keys.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/core.h        | 6 +++---
 net/wireless/ibss.c        | 6 ++----
 net/wireless/nl80211.c     | 1 -
 net/wireless/util.c        | 5 +----
 net/wireless/wext-compat.c | 6 +++---
 net/wireless/wext-sme.c    | 3 +--
 6 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/net/wireless/core.h b/net/wireless/core.h
index eee91443924d..5555e3c13ae9 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -249,9 +249,9 @@ struct cfg80211_event {
 };
 
 struct cfg80211_cached_keys {
-	struct key_params params[6];
-	u8 data[6][WLAN_MAX_KEY_LEN];
-	int def, defmgmt;
+	struct key_params params[4];
+	u8 data[4][WLAN_KEY_LEN_WEP104];
+	int def;
 };
 
 enum cfg80211_chan_mode {
diff --git a/net/wireless/ibss.c b/net/wireless/ibss.c
index 4a4dda53bdf1..896cbb20b6e1 100644
--- a/net/wireless/ibss.c
+++ b/net/wireless/ibss.c
@@ -284,10 +284,8 @@ int cfg80211_ibss_wext_join(struct cfg80211_registered_device *rdev,
 	if (!netif_running(wdev->netdev))
 		return 0;
 
-	if (wdev->wext.keys) {
+	if (wdev->wext.keys)
 		wdev->wext.keys->def = wdev->wext.default_key;
-		wdev->wext.keys->defmgmt = wdev->wext.default_mgmt_key;
-	}
 
 	wdev->wext.ibss.privacy = wdev->wext.default_key != -1;
 
@@ -295,7 +293,7 @@ int cfg80211_ibss_wext_join(struct cfg80211_registered_device *rdev,
 		ck = kmemdup(wdev->wext.keys, sizeof(*ck), GFP_KERNEL);
 		if (!ck)
 			return -ENOMEM;
-		for (i = 0; i < 6; i++)
+		for (i = 0; i < 4; i++)
 			ck->params[i].key = ck->data[i];
 	}
 	err = __cfg80211_join_ibss(rdev, wdev->netdev,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 739d0a780d83..8c6cbaac7d89 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -854,7 +854,6 @@ nl80211_parse_connkeys(struct cfg80211_registered_device *rdev,
 		return ERR_PTR(-ENOMEM);
 
 	result->def = -1;
-	result->defmgmt = -1;
 
 	nla_for_each_nested(key, keys, rem) {
 		memset(&parse, 0, sizeof(parse));
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 81fa16b36d30..dea2398329c4 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -912,7 +912,7 @@ void cfg80211_upload_connect_keys(struct wireless_dev *wdev)
 	if (!wdev->connect_keys)
 		return;
 
-	for (i = 0; i < 6; i++) {
+	for (i = 0; i < 4; i++) {
 		if (!wdev->connect_keys->params[i].cipher)
 			continue;
 		if (rdev_add_key(rdev, dev, i, false, NULL,
@@ -925,9 +925,6 @@ void cfg80211_upload_connect_keys(struct wireless_dev *wdev)
 				netdev_err(dev, "failed to set defkey %d\n", i);
 				continue;
 			}
-		if (wdev->connect_keys->defmgmt == i)
-			if (rdev_set_default_mgmt_key(rdev, dev, i))
-				netdev_err(dev, "failed to set mgtdef %d\n", i);
 	}
 
 	kzfree(wdev->connect_keys);
diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c
index e45a76449b43..7b97d43b27e1 100644
--- a/net/wireless/wext-compat.c
+++ b/net/wireless/wext-compat.c
@@ -408,10 +408,10 @@ static int __cfg80211_set_encryption(struct cfg80211_registered_device *rdev,
 
 	if (!wdev->wext.keys) {
 		wdev->wext.keys = kzalloc(sizeof(*wdev->wext.keys),
-					      GFP_KERNEL);
+					  GFP_KERNEL);
 		if (!wdev->wext.keys)
 			return -ENOMEM;
-		for (i = 0; i < 6; i++)
+		for (i = 0; i < 4; i++)
 			wdev->wext.keys->params[i].key =
 				wdev->wext.keys->data[i];
 	}
@@ -460,7 +460,7 @@ static int __cfg80211_set_encryption(struct cfg80211_registered_device *rdev,
 		if (err == -ENOENT)
 			err = 0;
 		if (!err) {
-			if (!addr) {
+			if (!addr && idx < 4) {
 				memset(wdev->wext.keys->data[idx], 0,
 				       sizeof(wdev->wext.keys->data[idx]));
 				wdev->wext.keys->params[idx].key_len = 0;
diff --git a/net/wireless/wext-sme.c b/net/wireless/wext-sme.c
index a4e8af3321d2..f6523a4387cc 100644
--- a/net/wireless/wext-sme.c
+++ b/net/wireless/wext-sme.c
@@ -35,7 +35,6 @@ int cfg80211_mgd_wext_connect(struct cfg80211_registered_device *rdev,
 
 	if (wdev->wext.keys) {
 		wdev->wext.keys->def = wdev->wext.default_key;
-		wdev->wext.keys->defmgmt = wdev->wext.default_mgmt_key;
 		if (wdev->wext.default_key != -1)
 			wdev->wext.connect.privacy = true;
 	}
@@ -47,7 +46,7 @@ int cfg80211_mgd_wext_connect(struct cfg80211_registered_device *rdev,
 		ck = kmemdup(wdev->wext.keys, sizeof(*ck), GFP_KERNEL);
 		if (!ck)
 			return -ENOMEM;
-		for (i = 0; i < 6; i++)
+		for (i = 0; i < 4; i++)
 			ck->params[i].key = ck->data[i];
 	}
 
-- 
2.8.1

Re: [PATCH 6/6] cfg80211: reduce connect key caching struct size

[Jouni Malinen]

On Tue, Sep 13, 2016 at 04:44:28PM +0200, Johannes Berg wrote:
> After the previous patches, connect keys can only (correctly)
> be used for storing static WEP keys. Therefore, remove all the
> data for dealing with key index 4/5 and reduce the size of the
> key material to the maximum for WEP keys.

> diff --git a/net/wireless/core.h b/net/wireless/core.h

>  struct cfg80211_cached_keys {
> -	struct key_params params[6];
> -	u8 data[6][WLAN_MAX_KEY_LEN];
> -	int def, defmgmt;
> +	struct key_params params[4];
> +	u8 data[4][WLAN_KEY_LEN_WEP104];
> +	int def;
>  };

As noted in our irc discussion, this is not really a good thing to do.
WEXT compat code uses this structure for all ciphers, not just static
WEP keys. BIP configuration can use key index 4-5 and the key lengths
can go up to 32 bytes instead of WLAN_KEY_LEN_WEP104. In other words,
this patch should be dropped or reverted since it causes kernel panics
due to memory corruption when writing beyond this reduced size
structure.

This was found with hwsim tests and after full day of running full test
runs, a compressed form for easy triggering of the issue was found:

hostap/tests/hwsim/vm$ ./vm-run.sh wext_pmf wext_pmf wext_pmf wext_pmf wext_pmf wext_pmf
Starting test run in a virtual machine
./run-all.sh: passing the following args to run-tests.py: wext_pmf wext_pmf wext_pmf wext_pmf wext_pmf wext_pmf 
START wext_pmf 1/6
PASS wext_pmf 7.384815 2016-09-28 20:36:15.644646
START wext_pmf 2/6
qemu-system-x86_64: 9pfs:virtfs_reset: One or more uncluncked fids found during reset
qemu-system-x86_64: 9pfs:virtfs_reset: One or more uncluncked fids found during reset
KERNEL CRASHED!

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 6/6] cfg80211: reduce connect key caching struct size

[Johannes Berg]

> >  struct cfg80211_cached_keys {
> > -	struct key_params params[6];
> > -	u8 data[6][WLAN_MAX_KEY_LEN];
> > -	int def, defmgmt;
> > +	struct key_params params[4];
> > +	u8 data[4][WLAN_KEY_LEN_WEP104];
> > +	int def;
> >  };
> 
> As noted in our irc discussion, this is not really a good thing to
> do.
> WEXT compat code uses this structure for all ciphers, not just static
> WEP keys. BIP configuration can use key index 4-5 and the key lengths
> can go up to 32 bytes instead of WLAN_KEY_LEN_WEP104. In other words,
> this patch should be dropped or reverted since it causes kernel
> panics due to memory corruption when writing beyond this reduced size
> structure.

Yeah, this was obviously a mistake - and smatch even pointed it out to
me, but I *still* couldn't find it.

I've just sent a fix to *really* only store the WEP keys, which fixes
the issue (after I could reproduce it) for me.

johannes