[PATCH] nl80211: Expand max value of NL80211_MESHCONF_HT_OPMODE command

[PATCH] nl80211: Expand max value of NL80211_MESHCONF_HT_OPMODE command

[Masashi Honma]

Previously, the max value of NL80211_MESHCONF_HT_OPMODE was 16.
But it causes EINVAL when IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED
and IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT bit is enabled.
So this patch expands the max value.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 net/wireless/nl80211.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 46417f9..8a00e50 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5471,7 +5471,10 @@ do {									    \
 	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, rssi_threshold, -255, 0,
 				  mask, NL80211_MESHCONF_RSSI_THRESHOLD,
 				  nl80211_check_s32);
-	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0, 16,
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0,
+				  IEEE80211_HT_OP_MODE_PROTECTION |
+				  IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT |
+				  IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT,
 				  mask, NL80211_MESHCONF_HT_OPMODE,
 				  nl80211_check_u16);
 	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, dot11MeshHWMPactivePathToRootTimeout,
-- 
2.7.4

Re: [PATCH] nl80211: Expand max value of NL80211_MESHCONF_HT_OPMODE command

[Johannes Berg]

On Tue, 2016-07-19 at 20:25 +0900, Masashi Honma wrote:
> Previously, the max value of NL80211_MESHCONF_HT_OPMODE was 16.
> But it causes EINVAL when IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED
> and IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT bit is enabled.
> So this patch expands the max value.
> 
> Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
> ---
>  net/wireless/nl80211.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 46417f9..8a00e50 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -5471,7 +5471,10 @@ do {						
> 			    \
>  	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, rssi_threshold, -255, 0,
>  				  mask,
> NL80211_MESHCONF_RSSI_THRESHOLD,
>  				  nl80211_check_s32);
> -	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0, 16,
> +	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0,
> +				  IEEE80211_HT_OP_MODE_PROTECTION |
> +				  IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT |
> +				  IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT,
>  				  mask, NL80211_MESHCONF_HT_OPMODE,
>  				  nl80211_check_u16);
> 
Hmm. So first of all, it looks like the old value was wrong because 16
would have been an allowed value, which doesn't make sense - 15 was
likely the maximum that should've been allowed?

You're now changing it to 23, which makes some sense, but is kinda
strange. It allows setting the unused bit 0x8 by itself or in
combination with any protection and/or the NON_GF bit, but not in
combination with the NON_HT bit.

It seems that perhaps this should rather check the value against a
bitmap of allowed bits, which would be exactly the ones you add in this
patch?

johannes

Re: [PATCH] nl80211: Expand max value of NL80211_MESHCONF_HT_OPMODE command

[Masashi Honma]

On 2016年08月02日 16:43, Johannes Berg wrote:
> You're now changing it to 23, which makes some sense, but is kinda
> strange. It allows setting the unused bit 0x8 by itself or in
> combination with any protection and/or the NON_GF bit, but not in
> combination with the NON_HT bit.

Thanks. This is reasonable. I will implement bitwise checks.

Masashi Honma.

[PATCH v2] nl80211: Receive correct value for NL80211_MESHCONF_HT_OPMODE command

[Masashi Honma]

Previously, NL80211_MESHCONF_HT_OPMODE rejected correct flag
combination, ex) IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED |
IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT.

This was caused by simple comparison with value 16. This causes setting
non-existent flag (like 0x08) and invalid flag combinations. So this
commit implements some checks based on IEEE 802.11 2012 8.4.2.59 HT
Operation element.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 net/wireless/nl80211.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 46417f9..b2af600 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5471,9 +5471,49 @@ do {									    \
 	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, rssi_threshold, -255, 0,
 				  mask, NL80211_MESHCONF_RSSI_THRESHOLD,
 				  nl80211_check_s32);
-	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0, 16,
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0,
+				  IEEE80211_HT_OP_MODE_PROTECTION |
+				  IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT |
+				  IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT,
 				  mask, NL80211_MESHCONF_HT_OPMODE,
 				  nl80211_check_u16);
+	if (tb[NL80211_MESHCONF_HT_OPMODE]) {
+		/*
+		 * Check HT operation mode based on IEEE 802.11 2012 8.4.2.59
+		 * HT Operation element.
+		 */
+		if (cfg->ht_opmode & (~(IEEE80211_HT_OP_MODE_PROTECTION |
+		    IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT |
+		    IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT)))
+			return -EINVAL;
+
+		if ((cfg->ht_opmode & IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT) &&
+		    (cfg->ht_opmode & IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT))
+			return -EINVAL;
+
+		switch (cfg->ht_opmode & IEEE80211_HT_OP_MODE_PROTECTION) {
+		case IEEE80211_HT_OP_MODE_PROTECTION_NONE:
+			if (cfg->ht_opmode &
+			    IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT)
+				return -EINVAL;
+			break;
+		case IEEE80211_HT_OP_MODE_PROTECTION_NONMEMBER:
+			if (!(cfg->ht_opmode &
+			    IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT))
+				return -EINVAL;
+			break;
+		case IEEE80211_HT_OP_MODE_PROTECTION_20MHZ:
+			if (cfg->ht_opmode &
+			    IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT)
+				return -EINVAL;
+			break;
+		case IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED:
+			if (!(cfg->ht_opmode &
+			    IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT))
+				return -EINVAL;
+			break;
+		}
+	}
 	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, dot11MeshHWMPactivePathToRootTimeout,
 				  1, 65535, mask,
 				  NL80211_MESHCONF_HWMP_PATH_TO_ROOT_TIMEOUT,
-- 
2.7.4

Re: [PATCH v2] nl80211: Receive correct value for NL80211_MESHCONF_HT_OPMODE command

[Masashi Honma]

On 2016年08月02日 20:41, Masashi Honma wrote:
> -	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0, 16,
> +	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0,
> +				  IEEE80211_HT_OP_MODE_PROTECTION |
> +				  IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT |
> +				  IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT,
>  				  mask, NL80211_MESHCONF_HT_OPMODE,
>  				  nl80211_check_u16);

This patch could over write cfg->ht_opmode even though EINVAL.
I will modify this.

Masashi Honma.

[PATCH v3] nl80211: Receive correct value for NL80211_MESHCONF_HT_OPMODE command

[Masashi Honma]

Previously, NL80211_MESHCONF_HT_OPMODE rejected correct flag
combination, ex) IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED |
IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT.

This was caused by simple comparison with value 16. This causes setting
non-existent flag (like 0x08) and invalid flag combinations. So this
commit implements some checks based on IEEE 802.11 2012 8.4.2.59 HT
Operation element.

Signed-off-by: Masashi Honma <masashi.honma@gmail.com>
---
 net/wireless/nl80211.c | 42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 46417f9..7b7530d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5380,6 +5380,7 @@ static int nl80211_parse_mesh_config(struct genl_info *info,
 {
 	struct nlattr *tb[NL80211_MESHCONF_ATTR_MAX + 1];
 	u32 mask = 0;
+	u16 ht_opmode;
 
 #define FILL_IN_MESH_PARAM_IF_SET(tb, cfg, param, min, max, mask, attr, fn) \
 do {									    \
@@ -5471,9 +5472,44 @@ do {									    \
 	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, rssi_threshold, -255, 0,
 				  mask, NL80211_MESHCONF_RSSI_THRESHOLD,
 				  nl80211_check_s32);
-	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, ht_opmode, 0, 16,
-				  mask, NL80211_MESHCONF_HT_OPMODE,
-				  nl80211_check_u16);
+	/*
+	 * Check HT operation mode based on
+	 * IEEE 802.11 2012 8.4.2.59 HT Operation element.
+	 */
+	if (tb[NL80211_MESHCONF_HT_OPMODE]) {
+		ht_opmode = nla_get_u16(tb[NL80211_MESHCONF_HT_OPMODE]);
+
+		if (ht_opmode & (~(IEEE80211_HT_OP_MODE_PROTECTION |
+		    IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT |
+		    IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT)))
+			return -EINVAL;
+
+		if ((ht_opmode & IEEE80211_HT_OP_MODE_NON_GF_STA_PRSNT) &&
+		    (ht_opmode & IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT))
+			return -EINVAL;
+
+		switch (ht_opmode & IEEE80211_HT_OP_MODE_PROTECTION) {
+		case IEEE80211_HT_OP_MODE_PROTECTION_NONE:
+			if (ht_opmode & IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT)
+				return -EINVAL;
+			break;
+		case IEEE80211_HT_OP_MODE_PROTECTION_NONMEMBER:
+			if (!(ht_opmode &
+			    IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT))
+				return -EINVAL;
+			break;
+		case IEEE80211_HT_OP_MODE_PROTECTION_20MHZ:
+			if (ht_opmode & IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT)
+				return -EINVAL;
+			break;
+		case IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED:
+			if (!(ht_opmode &
+			    IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT))
+				return -EINVAL;
+			break;
+		}
+		cfg->ht_opmode = ht_opmode;
+	}
 	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, dot11MeshHWMPactivePathToRootTimeout,
 				  1, 65535, mask,
 				  NL80211_MESHCONF_HWMP_PATH_TO_ROOT_TIMEOUT,
-- 
2.7.4

Re: [PATCH v2] nl80211: Receive correct value for NL80211_MESHCONF_HT_OPMODE command

[Johannes Berg]

> This patch could over write cfg->ht_opmode even though EINVAL.
> I will modify this.
> 

Don't think that actually matters since then it shouldn't be used, but
the v3 patch looks good.

I'm not sure we should bother to do cross-setting validation? Like, I
mean, validating that non-GF and non-HT aren't set together, etc. Those
are somewhat nonsense configurations, but we can't prevent them all.

I'm actually half thinking that we could just remove all restrictions
on this and allow any u16 value of this field, and rely on
wpa_supplicant to do the right thing... Then we don't have to update
this if we ever want to do something new either.

What do you think? What does the validation actually help us with?

johannes

Re: [PATCH v2] nl80211: Receive correct value for NL80211_MESHCONF_HT_OPMODE command

[Masashi Honma]

On 2016年08月03日 15:52, Johannes Berg wrote:
> I'm actually half thinking that we could just remove all restrictions
> on this and allow any u16 value of this field, and rely on
> wpa_supplicant to do the right thing... Then we don't have to update
> this if we ever want to do something new either.
>
> What do you think? What does the validation actually help us with?

I think checking the bits here is better than allowing all values.

Because if we allow any values for ht_opmode, kernel developer needs to 
care about any bit combination working well. For example, kernel 
developer should test there is not any unexpected thing when non-GF and 
non-HT both flags are enabled. If we check invalid bit at the entrance,
we don't need to care anymore about invalid combination. In any case we
need to care about combination. Then, it is more easy to do it near the
entrance.

And I think checking only in wpa_supplicant is not good idea. Because 
other user application can access to the kernel API. If invalid flag
combination causes kernel panic, it could be kernel vulnerability.

Masashi Honma.

Re: [PATCH v2] nl80211: Receive correct value for NL80211_MESHCONF_HT_OPMODE command

[Johannes Berg]

> And I think checking only in wpa_supplicant is not good idea. Because
> other user application can access to the kernel API. If invalid flag
> combination causes kernel panic, it could be kernel vulnerability.
> 

I don't really see how that should confuse a driver into a panic, but
fair enough.

johannes

Re: [PATCH v3] nl80211: Receive correct value for NL80211_MESHCONF_HT_OPMODE command

[Johannes Berg]

On Wed, 2016-08-03 at 10:07 +0900, Masashi Honma wrote:
> Previously, NL80211_MESHCONF_HT_OPMODE rejected correct flag
> combination, ex) IEEE80211_HT_OP_MODE_PROTECTION_NONHT_MIXED |
> IEEE80211_HT_OP_MODE_NON_HT_STA_PRSNT.
> 
> This was caused by simple comparison with value 16. This causes
> setting
> non-existent flag (like 0x08) and invalid flag combinations. So this
> commit implements some checks based on IEEE 802.11 2012 8.4.2.59 HT
> Operation element.
> 
Applied, with some minor changes.

johannes