[PATCH] mac80211: allow AP_VLAN operation on crypto controlled devices

[PATCH] mac80211: allow AP_VLAN operation on crypto controlled devices

[mpubbise]

From: Manikanta Pubbisetty <mpubbise@codeaurora.org>

In the current implementation, mac80211 advertises the support of
AP_VLANs based on the driver's support for AP mode; it also
blocks encrypted AP_VLAN operation on devices advertising
SW_CRYPTO_CONTROL.

The implementation seems weird in it's current form and could be
often confusing, this is because there can be drivers advertising
both SW_CRYPTO_CONTROL and AP mode support (ex: ath10k) in which case
AP_VLAN will still be supported but only in open BSS and not in
secured BSS.

When SW_CRYPTO_CONTROL is enabled, it makes more sense if the decision
to support AP_VLANs is left to the driver. Mac80211 can then allow
AP_VLAN operations depending on the driver support.

Signed-off-by: Manikanta Pubbisetty <mpubbise@codeaurora.org>
---
 net/mac80211/key.c  | 8 +++++---
 net/mac80211/main.c | 8 ++++++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index aee05ec..ee0d0cc 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -126,7 +126,7 @@ static void decrease_tailroom_need_count(struct ieee80211_sub_if_data *sdata,
 
 static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 {
-	struct ieee80211_sub_if_data *sdata;
+	struct ieee80211_sub_if_data *sdata = key->sdata;
 	struct sta_info *sta;
 	int ret = -EOPNOTSUPP;
 
@@ -162,7 +162,6 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 	if (sta && !sta->uploaded)
 		goto out_unsupported;
 
-	sdata = key->sdata;
 	if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) {
 		/*
 		 * The driver doesn't know anything about VLAN interfaces.
@@ -214,8 +213,11 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 		/* all of these we can do in software - if driver can */
 		if (ret == 1)
 			return 0;
-		if (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL))
+		if (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL)) {
+			if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
+				return 0;
 			return -EINVAL;
+		}
 		return 0;
 	default:
 		return -EINVAL;
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 0785d04..8d0333b 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -930,8 +930,12 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
 			             IEEE80211_HT_CAP_SM_PS_SHIFT;
 	}
 
-	/* if low-level driver supports AP, we also support VLAN */
-	if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_AP)) {
+	/* if low-level driver supports AP, we also support VLAN.
+	 * drivers advertising SW_CRYPTO_CONTROL should enable AP_VLAN
+	 * based on their support to transmit SW encrypted packets.
+	 */
+	if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_AP) &&
+	    !ieee80211_hw_check(&local->hw, SW_CRYPTO_CONTROL)) {
 		hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_AP_VLAN);
 		hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_AP_VLAN);
 	}
-- 
2.7.4

Re: [PATCH] mac80211: allow AP_VLAN operation on crypto controlled devices

[Alexander Wetzel]

Hello,

> From: Manikanta Pubbisetty <mpubbise@codeaurora.org>
> 
> In the current implementation, mac80211 advertises the support of
> AP_VLANs based on the driver's support for AP mode; it also
> blocks encrypted AP_VLAN operation on devices advertising
> SW_CRYPTO_CONTROL.
> 
> The implementation seems weird in it's current form and could be
> often confusing, this is because there can be drivers advertising
> both SW_CRYPTO_CONTROL and AP mode support (ex: ath10k) in which case
> AP_VLAN will still be supported but only in open BSS and not in
> secured BSS.
> 
> When SW_CRYPTO_CONTROL is enabled, it makes more sense if the decision
> to support AP_VLANs is left to the driver. Mac80211 can then allow
> AP_VLAN operations depending on the driver support.

This first part of the patch contradicts my current understanding of how 
Software crypto fallback can be triggered: We have a driver actively 
telling us to only fall back to sw crypto when it returns 1 on set_key, 
BUT we ignore that when the interface is set to @NL80211_IFTYPE_AP_VLAN 
and allow software encryption unconditionally?

Here the code:
         case WLAN_CIPHER_SUITE_GCMP:
         case WLAN_CIPHER_SUITE_GCMP_256:
                 /* all of these we can do in software - if driver can */
                 if (ret == 1)
                         return 0;
                 if (ieee80211_hw_check(&key->local->hw,
		                       SW_CRYPTO_CONTROL)) {
                         if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
                                 return 0;
                         return -EINVAL;
                 }
                 return 0;
         default:
                 return -EINVAL;
         }


Wouldn't it be preferable to just return "ret" or "-EINVAL" instead of 
"0" when the interface has @NL80211_IFTYPE_AP_VLAN set?
As it is this basically overrides SW_CRYPTO_CONTROL in AP Vlan mode!

For me it looks like the old behavior in this section was already fine 
and does not hurt the intention of this patch: A driver setting 
SW_CRYPTO_CONTROL won't get support for AP VLANs as long as the driver 
is not opting in to it.

Therefore I would like to undo this part of the patch again:

-		if (ieee80211_hw_check(&key->local->hw,
				       SW_CRYPTO_CONTROL))
+		if (ieee80211_hw_check(&key->local->hw,
				       SW_CRYPTO_CONTROL)) {
+			if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
+				return 0;
  			return -EINVAL;
+		}


Do I miss something here and would anyone have issues when I revert that 
in another patch?

Alexander

Re: [PATCH] mac80211: allow AP_VLAN operation on crypto controlled devices

[Sebastian Gottschall]

yes you just missed that ap_vlan is used for 4addr ap's / wds too so 
that might be related to the weired handling

Sebastian

Am 02.12.2018 um 14:02 schrieb Alexander Wetzel:
> Hello,
>
>> From: Manikanta Pubbisetty <mpubbise@codeaurora.org>
>>
>> In the current implementation, mac80211 advertises the support of
>> AP_VLANs based on the driver's support for AP mode; it also
>> blocks encrypted AP_VLAN operation on devices advertising
>> SW_CRYPTO_CONTROL.
>>
>> The implementation seems weird in it's current form and could be
>> often confusing, this is because there can be drivers advertising
>> both SW_CRYPTO_CONTROL and AP mode support (ex: ath10k) in which case
>> AP_VLAN will still be supported but only in open BSS and not in
>> secured BSS.
>>
>> When SW_CRYPTO_CONTROL is enabled, it makes more sense if the decision
>> to support AP_VLANs is left to the driver. Mac80211 can then allow
>> AP_VLAN operations depending on the driver support.
>
> This first part of the patch contradicts my current understanding of 
> how Software crypto fallback can be triggered: We have a driver 
> actively telling us to only fall back to sw crypto when it returns 1 
> on set_key, BUT we ignore that when the interface is set to 
> @NL80211_IFTYPE_AP_VLAN and allow software encryption unconditionally?
>
> Here the code:
>         case WLAN_CIPHER_SUITE_GCMP:
>         case WLAN_CIPHER_SUITE_GCMP_256:
>                 /* all of these we can do in software - if driver can */
>                 if (ret == 1)
>                         return 0;
>                 if (ieee80211_hw_check(&key->local->hw,
>                                SW_CRYPTO_CONTROL)) {
>                         if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
>                                 return 0;
>                         return -EINVAL;
>                 }
>                 return 0;
>         default:
>                 return -EINVAL;
>         }
>
>
> Wouldn't it be preferable to just return "ret" or "-EINVAL" instead of 
> "0" when the interface has @NL80211_IFTYPE_AP_VLAN set?
> As it is this basically overrides SW_CRYPTO_CONTROL in AP Vlan mode!
>
> For me it looks like the old behavior in this section was already fine 
> and does not hurt the intention of this patch: A driver setting 
> SW_CRYPTO_CONTROL won't get support for AP VLANs as long as the driver 
> is not opting in to it.
>
> Therefore I would like to undo this part of the patch again:
>
> -        if (ieee80211_hw_check(&key->local->hw,
>                        SW_CRYPTO_CONTROL))
> +        if (ieee80211_hw_check(&key->local->hw,
>                        SW_CRYPTO_CONTROL)) {
> +            if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
> +                return 0;
>              return -EINVAL;
> +        }
>
>
> Do I miss something here and would anyone have issues when I revert 
> that in another patch?
>
> Alexander
>

Re: [PATCH] mac80211: allow AP_VLAN operation on crypto controlled devices

[Alexander Wetzel]

> yes you just missed that ap_vlan is used for 4addr ap's / wds too so 
> that might be related to the weired handling
> 
For my understanding ath10k is supporting 4addr mode with HW ciphers 
only. (As long as you do now switch it to raw mode manually and forcing 
SW crypto only.)

The code in question seems to be only reachable if:
1) set_key returned something other than 0 or 1
2) The driver has set SW_CRYPTO_CONTROL

Once triggered it will overwrite the information from the driver, that 
it's not able to handle this key with either hardware or software 
encryption and try SW encryption anyway.
The packets should then be dropped/scrambled by the card and not be 
useful in any way.

Or the other way round:
4addr frames are still QoS data frames, correct?
So why should SW crypto working after the driver told us it will not?


Alexander

> Sebastian
> 
> Am 02.12.2018 um 14:02 schrieb Alexander Wetzel:
>> Hello,
>>
>>> From: Manikanta Pubbisetty <mpubbise@codeaurora.org>
>>>
>>> In the current implementation, mac80211 advertises the support of
>>> AP_VLANs based on the driver's support for AP mode; it also
>>> blocks encrypted AP_VLAN operation on devices advertising
>>> SW_CRYPTO_CONTROL.
>>>
>>> The implementation seems weird in it's current form and could be
>>> often confusing, this is because there can be drivers advertising
>>> both SW_CRYPTO_CONTROL and AP mode support (ex: ath10k) in which case
>>> AP_VLAN will still be supported but only in open BSS and not in
>>> secured BSS.
>>>
>>> When SW_CRYPTO_CONTROL is enabled, it makes more sense if the decision
>>> to support AP_VLANs is left to the driver. Mac80211 can then allow
>>> AP_VLAN operations depending on the driver support.
>>
>> This first part of the patch contradicts my current understanding of 
>> how Software crypto fallback can be triggered: We have a driver 
>> actively telling us to only fall back to sw crypto when it returns 1 
>> on set_key, BUT we ignore that when the interface is set to 
>> @NL80211_IFTYPE_AP_VLAN and allow software encryption unconditionally?
>>
>> Here the code:
>>         case WLAN_CIPHER_SUITE_GCMP:
>>         case WLAN_CIPHER_SUITE_GCMP_256:
>>                 /* all of these we can do in software - if driver can */
>>                 if (ret == 1)
>>                         return 0;
>>                 if (ieee80211_hw_check(&key->local->hw,
>>                                SW_CRYPTO_CONTROL)) {
>>                         if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
>>                                 return 0;
>>                         return -EINVAL;
>>                 }
>>                 return 0;
>>         default:
>>                 return -EINVAL;
>>         }
>>
>>
>> Wouldn't it be preferable to just return "ret" or "-EINVAL" instead of 
>> "0" when the interface has @NL80211_IFTYPE_AP_VLAN set?
>> As it is this basically overrides SW_CRYPTO_CONTROL in AP Vlan mode!
>>
>> For me it looks like the old behavior in this section was already fine 
>> and does not hurt the intention of this patch: A driver setting 
>> SW_CRYPTO_CONTROL won't get support for AP VLANs as long as the driver 
>> is not opting in to it.
>>
>> Therefore I would like to undo this part of the patch again:
>>
>> -        if (ieee80211_hw_check(&key->local->hw,
>>                        SW_CRYPTO_CONTROL))
>> +        if (ieee80211_hw_check(&key->local->hw,
>>                        SW_CRYPTO_CONTROL)) {
>> +            if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
>> +                return 0;
>>              return -EINVAL;
>> +        }
>>
>>
>> Do I miss something here and would anyone have issues when I revert 
>> that in another patch?
>>
>> Alexander
>>