[PATCH] mac80211: Honor SW_CRYPTO_CONTROL in AP VLAN mode

[PATCH] mac80211: Honor SW_CRYPTO_CONTROL in AP VLAN mode

[Alexander Wetzel]

Restore @SW_CRYPTO_CONTROL when interface is in AP Vlan mode and don't
override driver decision for unicast keys.

Fixes commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on crypto
controlled devices"), which should only have allow SW crypto fallback for
group keys.

Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---
 net/mac80211/key.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 4700718e010f..37e372896230 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -167,8 +167,10 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 		 * The driver doesn't know anything about VLAN interfaces.
 		 * Hence, don't send GTKs for VLAN interfaces to the driver.
 		 */
-		if (!(key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE))
+		if (!(key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE)) {
+			ret = 1;
 			goto out_unsupported;
+		}
 	}
 
 	ret = drv_set_key(key->local, SET_KEY, sdata,
@@ -213,11 +215,8 @@ 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 (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
-				return 0;
+		if (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL))
 			return -EINVAL;
-		}
 		return 0;
 	default:
 		return -EINVAL;
-- 
2.20.1

Re: [PATCH] mac80211: Honor SW_CRYPTO_CONTROL in AP VLAN mode

[Johannes Berg]

On Sat, 2019-02-09 at 15:01 +0100, Alexander Wetzel wrote:
> Restore @SW_CRYPTO_CONTROL when interface is in AP Vlan mode and don't
> override driver decision for unicast keys.
> 
> Fixes commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on crypto
> controlled devices"), which should only have allow SW crypto fallback for
> group keys.

This confuses me.

The driver doesn't really know about AP_VLAN, so the original commit
intentionally did this, I think?

There was some kind of other fix related to this though?

johannes

Re: [PATCH] mac80211: Honor SW_CRYPTO_CONTROL in AP VLAN mode

[Alexander Wetzel]

>> Restore @SW_CRYPTO_CONTROL when interface is in AP Vlan mode and don't
>> override driver decision for unicast keys.
>>
>> Fixes commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on crypto
>> controlled devices"), which should only have allow SW crypto fallback for
>> group keys.
> 
> This confuses me.
> 
> The driver doesn't really know about AP_VLAN, so the original commit
> intentionally did this, I think?
> 
> There was some kind of other fix related to this though?
Here how I understand the situation:

The intent of db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on crypto 
controlled devices") was, to allow devices setting SW_CRYPTO_CONTROL to 
still support AP_VLAN group keys. It unintentionally allowed more than 
it should for that...

Prior to the commit db3bdcb9c3ff installing a group key when in AP_VLAN 
mode could not work. Mac80211 was initializing ret to -EOPNOTSUPP and 
then jumped to "out_unsupported" when the interface was in AP_VLAN mode 
without trying to install the key and giving the driver a chance to 
allow SW crypto. Software fallback was then blocked by ret != 1 with 
SW_CRYPTO_CONTROL set by the driver.

But any driver setting NL80211_IFTYPE_AP_VLAN already has confirmed to 
be fine with SW crypto for group keys, since these CAN only be handled 
with SW crypto currently. The net effect was that ath10k - the only 
driver in tree setting SW_CRYPTO_CONTROL and also NL80211_IFTYPE_AP - 
was not able to send out frames encrypted with the group key at all.

Now the fix for that was commit db3bdcb9c3ff. Unfortunately this went 
too far:
We now not only allows fallback to SW crypto for group keys (which the 
driver allowed by setting NL80211_IFTYPE_AP_VLAN) but for ANY keys as 
long as the interface is in AP_VLAN mode.
So if for some reasons ath10k is not able to install a pairwise key 
mac80211 now incorrectly allows the fallback to SW crypto...

This patch here tries to fix that and excludes pairwise keys from the 
special AP_VLAN handling again (but keeps it for group keys).

This understanding is based on the result of a inquiry to linux-wireless 
(see https://patchwork.kernel.org/patch/10313127/) and a short 
out-of-list discussion with Christian Limpach who pointed me to 
http://lists.infradead.org/pipermail/ath10k/2018-November/012542.html
as the reason for the original fix in db3bdcb9c3ff.

Alexander

Re: [PATCH] mac80211: Honor SW_CRYPTO_CONTROL in AP VLAN mode

[Johannes Berg]

On Mon, 2019-04-08 at 21:58 +0200, Alexander Wetzel wrote:

> Here how I understand the situation:
> 
> The intent of db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on crypto 
> controlled devices") was, to allow devices setting SW_CRYPTO_CONTROL to 
> still support AP_VLAN group keys.

Yes, I think so.

> It unintentionally allowed more than it should for that...

I guess you're going to explain that in the text below :-)

> Prior to the commit db3bdcb9c3ff installing a group key when in AP_VLAN 
> mode could not work. Mac80211 was initializing ret to -EOPNOTSUPP and 
> then jumped to "out_unsupported" when the interface was in AP_VLAN mode 
> without trying to install the key and giving the driver a chance to 
> allow SW crypto. Software fallback was then blocked by ret != 1 with 
> SW_CRYPTO_CONTROL set by the driver.

Right.

> But any driver setting NL80211_IFTYPE_AP_VLAN already has confirmed to 
> be fine with SW crypto for group keys, since these CAN only be handled 
> with SW crypto currently. 

Well, NL80211_IFTYPE_AP_VLAN was set by mac80211 at the time still. Said
commit also changed that to only set it if SW_CRYPTO_CONTROL isn't set.

But yes, with the commit I agree - the driver is now setting
NL80211_IFTYPE_AP_VLAN so it's its own problem if something's wrong.

> The net effect was that ath10k - the only 
> driver in tree setting SW_CRYPTO_CONTROL and also NL80211_IFTYPE_AP - 
> was not able to send out frames encrypted with the group key at all.

I don't think I follow here. Before the commit in question, ath10k
would've rejected the key and not been able to. After the commit, and of
course also after 4920ce3bf7e0d ("ath10k: add dynamic vlan support")
(which you linked to below) ath10k again sets the NL80211_IFTYPE_AP_VLAN
bit under certain conditions, and thus *can* do software-crypto of such
frames.

> Now the fix for that was commit db3bdcb9c3ff. Unfortunately this went 
> too far:
> We now not only allows fallback to SW crypto for group keys (which the 
> driver allowed by setting NL80211_IFTYPE_AP_VLAN) but for ANY keys as 
> long as the interface is in AP_VLAN mode.

Aha, now I follow.

> So if for some reasons ath10k is not able to install a pairwise key 
> mac80211 now incorrectly allows the fallback to SW crypto...

Indeed, I agree.

> This patch here tries to fix that and excludes pairwise keys from the 
> special AP_VLAN handling again (but keeps it for group keys).

Right, ok, makes sense now. I'll probably edit the commit message, but
yeah, I can follow your patch now.

Thanks,
johannes