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 <[email protected]>
---
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
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
>> 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
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