[PATCH] iwlwifi: Extended Key ID support for mvm and dvm

[PATCH] iwlwifi: Extended Key ID support for mvm and dvm

[Alexander Wetzel]

All iwlwifi cards below the 22000 series are able to handle multiple
keyids per STA and allow the selection of the encryption key per MPDU.

These are therefore fully compatible with the Extended Key ID support
implementation in mac80211.

Enable Extended Key ID support for all dvm cards and the mvm cards not
using the incompatible new Tx API introduced for the 22000 series.

Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---

This is basically the V3 of the patch, but the other patches were part
of series and this here is the first standalone version.

V1: https://patchwork.kernel.org/patch/10931879/
V2: https://patchwork.kernel.org/patch/11024137/

V1 become deprecated due to redesigning the Extended Key ID support API.
V2 became deprecated due to the discovery that the 22000 is not (yet)
able to support Extended Key ID.

The patch is still super trivial, but I cross checked that Extended Key
ID support is enabled with my old 3168 card and disabled with my new
AX200 card.

 drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c | 1 +
 drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c b/drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c
index 6c170636110a..ac88c19f4f18 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c
@@ -200,6 +200,7 @@ int iwlagn_mac_setup_register(struct iwl_priv *priv,
 	iwl_leds_init(priv);
 
 	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);
+	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_EXT_KEY_ID);
 
 	ret = ieee80211_register_hw(priv->hw);
 	if (ret) {
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
index b74bd58f3f45..034bf959153b 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
@@ -617,6 +617,14 @@ int iwl_mvm_mac_setup_register(struct iwl_mvm *mvm)
 
 	hw->wiphy->flags |= WIPHY_FLAG_IBSS_RSN;
 	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_VHT_IBSS);
+
+	/* The new Tx API does not allow to pass the key or keyid of a MPDU to
+	 * the hw, preventing us to control which key(id) to use per MPDU.
+	 * Till that's fixed we can't use Extended Key ID for the newer cards.
+	 */
+	if (!iwl_mvm_has_new_tx_api(mvm))
+		wiphy_ext_feature_set(hw->wiphy,
+				      NL80211_EXT_FEATURE_EXT_KEY_ID);
 	hw->wiphy->features |= NL80211_FEATURE_HT_IBSS;
 
 	hw->wiphy->regulatory_flags |= REGULATORY_ENABLE_RELAX_NO_IR;
-- 
2.22.0

Re: [PATCH] iwlwifi: Extended Key ID support for mvm and dvm

[Johannes Berg]

On Mon, 2019-08-19 at 20:05 +0200, Alexander Wetzel wrote:
> All iwlwifi cards below the 22000 series are able to handle multiple
> keyids per STA and allow the selection of the encryption key per MPDU.
> 
> These are therefore fully compatible with the Extended Key ID support
> implementation in mac80211.
> 
> Enable Extended Key ID support for all dvm cards and the mvm cards not
> using the incompatible new Tx API introduced for the 22000 series.
> 
> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>

LGTM.

> +
> +	/* The new Tx API does not allow to pass the key or keyid of a MPDU to
> +	 * the hw, preventing us to control which key(id) to use per MPDU.
> +	 * Till that's fixed we can't use Extended Key ID for the newer cards.

Technically we still don't need per MPDU, we just need to switch which
one to use for TX after installing it for RX already.

johannes

Re: [PATCH] iwlwifi: Extended Key ID support for mvm and dvm

[Alexander Wetzel]

>> +
>> +	/* The new Tx API does not allow to pass the key or keyid of a MPDU to
>> +	 * the hw, preventing us to control which key(id) to use per MPDU.
>> +	 * Till that's fixed we can't use Extended Key ID for the newer cards.
> 
> Technically we still don't need per MPDU, we just need to switch which
> one to use for TX after installing it for RX already.

The Extended Key ID API we finally merged in mac80211 is not notifying 
the driver when to switch the key over to the other id.
The current API provides the key/keyid per MPDU and let's mac80211 have 
the full control what's the correct key for each frame.

That's especially critical for drivers setting 
IEEE80211_KEY_FLAG_GENERATE_IV and/or supporting A-MPDU's. Allowing the 
driver to override the mac80211 decision is only safe when the 
driver/card generates the PNs itself and also handles the A-MPDU key 
borders correctly.

While less desirable we still could get that working: The mvm driver 
would have to detect the key borders and then tell the firmware to 
switch over to the other key. But we would have to make sure to not 
re-enable A-MPDU aggregation till the card really has switched.

Alexander

Re: [PATCH] iwlwifi: Extended Key ID support for mvm and dvm

[Johannes Berg]

On Mon, 2019-08-19 at 21:57 +0200, Alexander Wetzel wrote:
> > > +
> > > +	/* The new Tx API does not allow to pass the key or keyid of a MPDU to
> > > +	 * the hw, preventing us to control which key(id) to use per MPDU.
> > > +	 * Till that's fixed we can't use Extended Key ID for the newer cards.
> > 
> > Technically we still don't need per MPDU, we just need to switch which
> > one to use for TX after installing it for RX already.
> 
> The Extended Key ID API we finally merged in mac80211 is not notifying 
> the driver when to switch the key over to the other id.

Oh, right, good point.

> The current API provides the key/keyid per MPDU and let's mac80211 have 
> the full control what's the correct key for each frame.

Yeah, but as you noticed we no longer have that control per MPDU with
the new TX API in iwlmvm.

> That's especially critical for drivers setting 
> IEEE80211_KEY_FLAG_GENERATE_IV and/or supporting A-MPDU's. Allowing the 
> driver to override the mac80211 decision is only safe when the 
> driver/card generates the PNs itself and also handles the A-MPDU key 
> borders correctly.

Sure, the device does generate the PN itself now with the new TX API
too. It doesn't care about A-MPDU key borders, but it probably could
when taught to care about extended key ID.

> While less desirable we still could get that working: The mvm driver 
> would have to detect the key borders and then tell the firmware to 
> switch over to the other key. But we would have to make sure to not 
> re-enable A-MPDU aggregation till the card really has switched.

I'm not entirely sure off the top of my head how it works, but it seems
possible that if we just assign a new PN to retransmits of the same
frame but in a new A-MPDU after key switching, it wouldn't actually
matter?

But then again maybe somewhere it's stated that we must use the same key
for all transmit attempts of a single frame? Not sure.

I'm also not sure if we could actually assign a PN from the new key for
the retransmit, the hardware has to store those back into memory
normally.

So probably you're right, and we'd have to disable A-MPDUs until we have
no outstanding old-key-retransmits, but that seems manageable.

johannes

Re: [PATCH] iwlwifi: Extended Key ID support for mvm and dvm

[Johannes Berg]

On Mon, 2019-08-19 at 22:03 +0200, Johannes Berg wrote:
> 
> > While less desirable we still could get that working: The mvm driver 
> > would have to detect the key borders and then tell the firmware to 
> > switch over to the other key. But we would have to make sure to not 
> > re-enable A-MPDU aggregation till the card really has switched.

> So probably you're right, and we'd have to disable A-MPDUs until we have
> no outstanding old-key-retransmits, but that seems manageable.

Actually, we probably have to even delay the key switch until there are
no more frames to retransmit, because the hardware is involved to some
extent and it won't know about two keys or something... Not really sure
how it all works though, off the top of my head.

johannes

Re: [PATCH] iwlwifi: Extended Key ID support for mvm and dvm

[Alexander Wetzel]

Am 19.08.19 um 22:09 schrieb Johannes Berg:
> On Mon, 2019-08-19 at 22:03 +0200, Johannes Berg wrote:
>>
>>> While less desirable we still could get that working: The mvm driver
>>> would have to detect the key borders and then tell the firmware to
>>> switch over to the other key. But we would have to make sure to not
>>> re-enable A-MPDU aggregation till the card really has switched.
> 
>> So probably you're right, and we'd have to disable A-MPDUs until we have
>> no outstanding old-key-retransmits, but that seems manageable.
> 
> Actually, we probably have to even delay the key switch until there are
> no more frames to retransmit, because the hardware is involved to some
> extent and it won't know about two keys or something... Not really sure
> how it all works though, off the top of my head.

This sounds like the card is not really able to handle two unicast key 
per STA, which would be a show stopper.
But not sure if I can believe that: After all the card is setting the 
correct keyid for the key and e.g. able to use keyid 1 for both send and 
receive, so it's not simply assuming unicast keys are always using keyid 0.

Honoring the keyid for that but then not be able to differentiate 
between the keyids for re-transmits is nothing I would have expected. So 
I still hope you are wrong here:-)

Alexander

Re: [PATCH] iwlwifi: Extended Key ID support for mvm and dvm

[Alexander Wetzel]

Am 19.08.19 um 22:03 schrieb Johannes Berg:
> On Mon, 2019-08-19 at 21:57 +0200, Alexander Wetzel wrote:
>>>> +
>>>> +	/* The new Tx API does not allow to pass the key or keyid of a MPDU to
>>>> +	 * the hw, preventing us to control which key(id) to use per MPDU.
>>>> +	 * Till that's fixed we can't use Extended Key ID for the newer cards.
>>>
>>> Technically we still don't need per MPDU, we just need to switch which
>>> one to use for TX after installing it for RX already.
>>
>> The Extended Key ID API we finally merged in mac80211 is not notifying
>> the driver when to switch the key over to the other id.
> 
> Oh, right, good point.
> 
>> The current API provides the key/keyid per MPDU and let's mac80211 have
>> the full control what's the correct key for each frame.
> 
> Yeah, but as you noticed we no longer have that control per MPDU with
> the new TX API in iwlmvm.
> 
>> That's especially critical for drivers setting
>> IEEE80211_KEY_FLAG_GENERATE_IV and/or supporting A-MPDU's. Allowing the
>> driver to override the mac80211 decision is only safe when the
>> driver/card generates the PNs itself and also handles the A-MPDU key
>> borders correctly.
> 
> Sure, the device does generate the PN itself now with the new TX API
> too. It doesn't care about A-MPDU key borders, but it probably could
> when taught to care about extended key ID.
> 
>> While less desirable we still could get that working: The mvm driver
>> would have to detect the key borders and then tell the firmware to
>> switch over to the other key. But we would have to make sure to not
>> re-enable A-MPDU aggregation till the card really has switched.
> 
> I'm not entirely sure off the top of my head how it works, but it seems
> possible that if we just assign a new PN to retransmits of the same
> frame but in a new A-MPDU after key switching, it wouldn't actually
> matter?

I was thinking about mac80211 re-enabling A-MPDU aggregation but the 
driver still using the old key for some frames and then switching to the 
new key within one A-MPDU, causing non-compliant A-MPDUs.

> But then again maybe somewhere it's stated that we must use the same key
> for all transmit attempts of a single frame? Not sure.

I know of course next to nothing how the hardware/firmware is handling 
that, but I'm surprised this could be a problem. I assumed the card 
would create a full MPDU (with the crypto headers) and use that for 
retransmit and not create the MPDU new... After all you still have to 
use the old PN and getting that right sounds tricky when you don't have 
the full MPDU cached somehow. But I guess the high bandwidth and the 
required buffer space forced you to extreme measures to conserve space...

> 
> I'm also not sure if we could actually assign a PN from the new key for
> the retransmit, the hardware has to store those back into memory
> normally.
> 
> So probably you're right, and we'd have to disable A-MPDUs until we have
> no outstanding old-key-retransmits, but that seems manageable.

Extending the A-MPDU block mac80211 is using to enforce the A-MPDU key 
borders should be not very hard. Figuring out the time when we safely 
can restart it - when re-transmits indeed are also an issue - I'm less 
sure about. I believe re-transmits are handled by the card and not the 
driver but so far had no reason to dig deeper into that topic. But I 
guess a call to switch over the keyid which blocks till that is done 
should take care of it.

Alexander

Alexander

Re: [PATCH] iwlwifi: Extended Key ID support for mvm and dvm

[Alexander Wetzel]

Am 19.08.19 um 20:05 schrieb Alexander Wetzel:
> All iwlwifi cards below the 22000 series are able to handle multiple
> keyids per STA and allow the selection of the encryption key per MPDU.
> 
> These are therefore fully compatible with the Extended Key ID support
> implementation in mac80211.
> 
> Enable Extended Key ID support for all dvm cards and the mvm cards not
> using the incompatible new Tx API introduced for the 22000 series.
> 
> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
> ---
> 
> This is basically the V3 of the patch, but the other patches were part
> of series and this here is the first standalone version.
> 
> V1: https://patchwork.kernel.org/patch/10931879/
> V2: https://patchwork.kernel.org/patch/11024137/
> 
> V1 become deprecated due to redesigning the Extended Key ID support API.
> V2 became deprecated due to the discovery that the 22000 is not (yet)
> able to support Extended Key ID.
> 
> The patch is still super trivial, but I cross checked that Extended Key
> ID support is enabled with my old 3168 card and disabled with my new
> AX200 card.

For what it's worth:

I just upgraded my test AP from the initial AC 3168 to an AC 9560 card 
after the AC 201 turned out to be incompatible (for now).

So I can now confirm that Extended Key ID is also working with this card 
and the patch here. (Tested against my "reference" system using an old 
Intel Ultimate-N 6300.)

Alexander