On Wed, 2018-12-05 at 21:54 +0100, Alexander Wetzel wrote: > > It started out as a flag and I switched to enum later without updating > it. I'll chnage that to nl80211_key_install_mode, much much better... > No, all stations using Extended Key ID will always use RX_ONLY and > SET_TX for pairwise key installs. The AP will install the Key Rx only > prior to sending eapol #3, the sta prior to sending eapol #4. Actually ... let's see all the operations at nl80211 level. We have NEW_KEY and SET_KEY, right? SET_KEY basically already says to use a given key for TX from now on, IIRC? So realistically, don't we only need NEW_KEY (RX-only) as a new variant of NEW_KEY? > The PTK0 rekey patch added a new line in front of the description. The > next author did not notice that and added the description for > NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER at the wrong place, probably > assuming it was the end of the list. I've noticed that and fixed the > documentation order and the misleading empty line. > I'll break that out as a separate cleanup patch if nobody beats me to it:-) Oh. OK. It also doesn't really matter ;-) > > > k->def_multi = true; > > > > > > + k->rx_only = !!info->attrs[NL80211_ATTR_KEY_RXONLY]; > > > + k->set_tx = !!info->attrs[NL80211_ATTR_KEY_SETTX]; > > > > shouldn't this already be install_mode? > > > > Looks like switching to NL80211_ATTR_INSTALL_MODE will throw out that, > but with the code from this patch that's an interim layer for checks and > mapping it. So I'm not sure I understand that comment. Well, me neither. Sounds almost like I got ahead of myself. > > You need to check if extended key ID is supported > > Yes. I have added checks in cfg80211_validate_key_settings current > development version already, making sure only valid combinations can be > called and reach this section. Ok, great. > NL80211_CMD_SET_KEY is normally only used to set default keys for wep or > managment keys. That changes here. Right. > In this API draft NL80211_CMD_NEW_KEY is only used when installing a > Extended Key ID key RX only. The switch to TX is added to > NL80211_CMD_SET_KEY. The code has some sanity checks and then tells the > driver to switch the key to tx. Makes sense. > But that has been changed quite a bit, the procedure in this patch > turned out to be not so good and even had a locking issue, so it has > changed a bit. I guess we should shelf that till I get the new variant > working and then look at it again. Fair enough. > > > +++ b/net/wireless/util.c > > > @@ -236,13 +236,14 @@ int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev, > > > case WLAN_CIPHER_SUITE_CCMP_256: > > > case WLAN_CIPHER_SUITE_GCMP: > > > case WLAN_CIPHER_SUITE_GCMP_256: > > > - /* Disallow pairwise keys with non-zero index unless it's WEP > > > + /* IEEE802.11-2016 allows only 0 and 1 for pairwise keys. > > > + * Disallow pairwise keys with index above 1 unless it's WEP > > > * or a vendor specific cipher (because current deployments use > > > - * pairwise WEP keys with non-zero indices and for vendor > > > + * pairwise WEP keys with higher indices and for vendor > > > * specific ciphers this should be validated in the driver or > > > - * hardware level - but 802.11i clearly specifies to use zero) > > > + * hardware level. > > > */ > > > - if (pairwise && key_idx) > > > + if (pairwise && key_idx > 1) > > > return -EINVAL; > > > break; > > > > Again, only if driver support is advertised, and the comment should > > probably reference the feature bit from the spec. > > That is where I added most of the sanity checks in the meantime. > But what feature bit from the spec are you referring to? The RSN > Capability one? Well, I wasn't thinking that precisely. I just thought it should mention that it now says "IEEE802.11-2016 allows only 0 and 1 for pairwise keys" but doesn't clarify that this is only for stations that actually want to support it, so it could be read as being always that way. johannes
