From: Alexander Wetzel <alexander@wetzel-home.de>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID
Date: Thu, 6 Dec 2018 17:21:36 +0100 [thread overview]
Message-ID: <f70db2ec-0d14-3ee6-a8e8-9a54c4b1abb6@wetzel-home.de> (raw)
In-Reply-To: <0d60ddc1367ab8e82811898b13e4d990794c9118.camel@sipsolutions.net>
> 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?
>
Yes, that should indeed work. I'll try that and see how it plays out.
>> 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.
>
Makes sense. I'll reword that a bit.
next prev parent reply other threads:[~2018-12-06 17:36 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-11 11:02 [RFC PATCH v2 0/2] Extended Key ID support for linux Alexander Wetzel
2018-11-11 11:02 ` [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID Alexander Wetzel
2018-12-05 14:51 ` Johannes Berg
2018-12-05 20:54 ` Alexander Wetzel
2018-12-06 7:25 ` Johannes Berg
2018-12-06 16:21 ` Alexander Wetzel [this message]
2018-11-11 11:02 ` [RFC PATCH v2 2/2] mac80211: " Alexander Wetzel
2018-12-05 14:58 ` Johannes Berg
2018-12-05 21:58 ` Alexander Wetzel
2018-12-06 7:32 ` Johannes Berg
2018-12-06 16:27 ` Alexander Wetzel
2018-12-05 14:42 ` [RFC PATCH v2 0/2] Extended Key ID support for linux Johannes Berg
2018-12-05 19:06 ` Alexander Wetzel
2018-12-07 10:01 ` Jouni Malinen
2018-12-08 13:58 ` Alexander Wetzel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f70db2ec-0d14-3ee6-a8e8-9a54c4b1abb6@wetzel-home.de \
--to=alexander@wetzel-home.de \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).