| Message ID | 20190209140138.16692-1-alexander@wetzel-home.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: Honor SW_CRYPTO_CONTROL in AP VLAN mode | expand |
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
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;
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(-)