DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EAFFC20892
Subject: Re: [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key
 ID
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
References: <20181111110235.14213-1-alexander@wetzel-home.de>
 <20181111110235.14213-2-alexander@wetzel-home.de>
 <471c7131e4b81a06f8c514652038d69819ae66ea.camel@sipsolutions.net>
 <a2d2f2ff-f776-93e8-579a-fbc05f236d76@wetzel-home.de>
 <0d60ddc1367ab8e82811898b13e4d990794c9118.camel@sipsolutions.net>
From:   Alexander Wetzel <alexander@wetzel-home.de>
Message-ID: <f70db2ec-0d14-3ee6-a8e8-9a54c4b1abb6@wetzel-home.de>
Date:   Thu, 6 Dec 2018 17:21:36 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.3.1
MIME-Version: 1.0
In-Reply-To: <0d60ddc1367ab8e82811898b13e4d990794c9118.camel@sipsolutions.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

> 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.