Re: [PATCH v2 11/12] mac80211: drop data frames without key on encrypted links

Re: [PATCH v2 11/12] mac80211: drop data frames without key on encrypted links

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.5.11, v5.4.27, v4.19.112, v4.14.174, v4.9.217, v4.4.217.

v5.5.11: Build OK!
v5.4.27: Build OK!
v4.19.112: Failed to apply! Possible dependencies:
    03512ceb60ae ("ieee80211: remove redundant leading zeroes")
    09b4a4faf9d0 ("mac80211: introduce capability flags for VHT EXT NSS support")
    0eeb2b674f05 ("mac80211: add an option for station management TXQ")
    1c9559734eca ("mac80211: remove unnecessary key condition")
    57a3a454f303 ("iwlwifi: split HE capabilities between AP and STA")
    62872a9b9a10 ("mac80211: Fix PTK rekey freezes and clear text leak")
    77f7ffdc335d ("mac80211: minstrel_ht: add flag to indicate missing/inaccurate tx A-MPDU length")
    77ff2c6b4984 ("mac80211: update HE IEs to D3.3")
    80aaa9c16415 ("mac80211: Add he_capa debugfs entry")
    96fc6efb9ad9 ("mac80211: IEEE 802.11 Extended Key ID support")
    add7453ad62f ("wireless: align to draft 11ax D3.0")
    adf8ed01e4fd ("mac80211: add an optional TXQ for other PS-buffered frames")
    caf56338c22f ("mac80211: indicate support for multiple BSSID")
    daa5b83513a7 ("mac80211: update HE operation fields to D3.0")

v4.14.174: Failed to apply! Possible dependencies:
    110b32f065f3 ("iwlwifi: mvm: rs: add basic implementation of the new RS API handlers")
    1c73acf58bd6 ("iwlwifi: acpi: move ACPI method definitions to acpi.h")
    28e9c00fe1f0 ("iwlwifi: remove upper case letters in sku_capa_band_*_enable")
    4ae80f6c8d86 ("iwlwifi: support api ver2 of NVM_GET_INFO resp")
    4b82455ca51e ("iwlwifi: use flags to denote modifiers for the channel maps")
    4c625c564ba2 ("iwlwifi: get rid of fw/nvm.c")
    514c30696fbc ("iwlwifi: add support for IEEE802.11ax")
    57a3a454f303 ("iwlwifi: split HE capabilities between AP and STA")
    77ff2c6b4984 ("mac80211: update HE IEs to D3.3")
    813df5cef3bb ("iwlwifi: acpi: add common code to read from ACPI")
    8a6171a7b601 ("iwlwifi: fw: add FW APIs for HE")
    9c4f7d512740 ("iwlwifi: move all NVM parsing code to the common files")
    9f66a397c877 ("iwlwifi: mvm: rs: add ops for the new rate scaling in the FW")
    e7a3b8d87910 ("iwlwifi: acpi: move ACPI-related definitions to acpi.h")

v4.9.217: Failed to apply! Possible dependencies:
    01796ff2fa6e ("iwlwifi: mvm: always free inactive queue when moving ownership")
    0aaece81114e ("iwlwifi: split firmware API from iwl-trans.h")
    1ea423b0e047 ("iwlwifi: remove unnecessary dev_cmd_headroom parameter")
    310181ec34e2 ("iwlwifi: move to TVQM mode")
    5594d80e9bf4 ("iwlwifi: support two phys for a000 devices")
    623e7766be90 ("iwlwifi: pcie: introduce split point to a000 devices")
    65e254821cee ("iwlwifi: mvm: use firmware station PM notification for AP_LINK_PS")
    6b35ff91572f ("iwlwifi: pcie: introduce a000 TX queues management")
    727c02dfb848 ("iwlwifi: pcie: cleanup rfkill checks")
    77ff2c6b4984 ("mac80211: update HE IEs to D3.3")
    8236f7db2724 ("iwlwifi: mvm: assign cab queue to the correct station")
    87d0e1af9db3 ("iwlwifi: mvm: separate queue mapping from queue enablement")
    bb49701b41de ("iwlwifi: mvm: support a000 SCD queue configuration")
    cf90da352a32 ("iwlwifi: mvm: use mvm_disable_queue instead of sharing logic")
    d172a5eff629 ("iwlwifi: reorganize firmware API")
    df88c08d5c7e ("iwlwifi: mvm: release static queues on bcast release")
    eda50cde58de ("iwlwifi: pcie: add context information support")

v4.4.217: Failed to apply! Possible dependencies:
    0aaece81114e ("iwlwifi: split firmware API from iwl-trans.h")
    13555e8ba2f4 ("iwlwifi: mvm: add 9000-series RX API")
    1a616dd2f171 ("iwlwifi: dump prph registers in a common place for all transports")
    1e0bbebaae66 ("mac80211: enable starting BA session with custom timeout")
    2f89a5d7d377 ("iwlwifi: mvm: move fw-dbg code to separate file")
    39bdb17ebb5b ("iwlwifi: update host command messages to new format")
    41837ca962ec ("iwlwifi: pcie: allow to pretend to have Tx CSUM for debug")
    4707fde5cdef ("iwlwifi: mvm: use build-time assertion for fw trigger ID")
    5b88792cd850 ("iwlwifi: move to wide ID for all commands")
    6c4fbcbc1c95 ("iwlwifi: add support for 12K Receive Buffers")
    77ff2c6b4984 ("mac80211: update HE IEs to D3.3")
    92fe83430b89 ("iwlwifi: uninline iwl_trans_send_cmd")
    d172a5eff629 ("iwlwifi: reorganize firmware API")
    dcbb4746286a ("iwlwifi: trans: support a callback for ASYNC commands")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [PATCH v2 11/12] mac80211: drop data frames without key on encrypted links

[Pali Rohár]

On Friday 27 March 2020 15:03:41 Sasha Levin wrote:
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
> 
> The bot has tested the following trees: v5.5.11, v5.4.27, v4.19.112, v4.14.174, v4.9.217, v4.4.217.
> 
> v5.5.11: Build OK!
> v5.4.27: Build OK!
> v4.19.112: Failed to apply! Possible dependencies:
...
> v4.14.174: Failed to apply! Possible dependencies:
...
> v4.9.217: Failed to apply! Possible dependencies:
...
> v4.4.217: Failed to apply! Possible dependencies:
...
> 
> How should we proceed with this patch?

Hello! I have looked at this patch and backported it into 4.19 and older
versions. But as this patch is security related and backporting needed
some code changes, it is required to review this patch prior including
it into any stable branch. Patch is below.

The main change in backported patch is in ieee80211_key_replace()
function.

So could you please review this patch if it is correct and if it is
suitable for backporting it into stable kernels 4.19 in this (or other)
form?

=======================================================================

From 189da5743e28d0c5d211b70b4cb06ce3aff77d86 Mon Sep 17 00:00:00 2001
From: Johannes Berg <johannes.berg@intel.com>
Date: Thu, 26 Mar 2020 15:09:42 +0200
Subject: [PATCH] mac80211: drop data frames without key on encrypted links
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

commit a0761a301746ec2d92d7fcb82af69c0a6a4339aa upstream.

If we know that we have an encrypted link (based on having had
a key configured for TX in the past) then drop all data frames
in the key selection handler if there's no key anymore.

This fixes an issue with mac80211 internal TXQs - there we can
buffer frames for an encrypted link, but then if the key is no
longer there when they're dequeued, the frames are sent without
encryption. This happens if a station is disconnected while the
frames are still on the TXQ.

Detecting that a link should be encrypted based on a first key
having been configured for TX is fine as there are no use cases
for a connection going from with encryption to no encryption.
With extended key IDs, however, there is a case of having a key
configured for only decryption, so we can't just trigger this
behaviour on a key being configured.

Cc: stable@vger.kernel.org
Reported-by: Jouni Malinen <j@w1.fi>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
Link: https://lore.kernel.org/r/iwlwifi.20200326150855.6865c7f28a14.I9fb1d911b064262d33e33dfba730cdeef83926ca@changeid
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
[pali: Backported to 4.19 and older versions]
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 net/mac80211/debugfs_sta.c |  1 +
 net/mac80211/key.c         |  7 +++++--
 net/mac80211/sta_info.h    |  1 +
 net/mac80211/tx.c          | 12 +++++++++---
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 4105081dc1df..6f390c2e4c8e 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -80,6 +80,7 @@ static const char * const sta_flag_names[] = {
 	FLAG(MPSP_OWNER),
 	FLAG(MPSP_RECIPIENT),
 	FLAG(PS_DELIVER),
+	FLAG(USES_ENCRYPTION),
 #undef FLAG
 };
 
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index f20bb39f492d..217db25a1afa 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -341,8 +341,11 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
 	if (sta) {
 		if (pairwise) {
 			rcu_assign_pointer(sta->ptk[idx], new);
-			sta->ptk_idx = idx;
-			ieee80211_check_fast_xmit(sta);
+			if (new) {
+				set_sta_flag(new->sta, WLAN_STA_USES_ENCRYPTION);
+				new->sta->ptk_idx = new->conf.keyidx;
+				ieee80211_check_fast_xmit(new->sta);
+			}
 		} else {
 			rcu_assign_pointer(sta->gtk[idx], new);
 		}
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 9a04327d71d1..075609c4571d 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -101,6 +101,7 @@ enum ieee80211_sta_info_flags {
 	WLAN_STA_MPSP_OWNER,
 	WLAN_STA_MPSP_RECIPIENT,
 	WLAN_STA_PS_DELIVER,
+	WLAN_STA_USES_ENCRYPTION,
 
 	NUM_WLAN_STA_FLAGS,
 };
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 98d048630ad2..3530d1a5fc98 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -593,10 +593,13 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
 
-	if (unlikely(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT))
+	if (unlikely(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)) {
 		tx->key = NULL;
-	else if (tx->sta &&
-		 (key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx])))
+		return TX_CONTINUE;
+	}
+
+	if (tx->sta &&
+	    (key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx])))
 		tx->key = key;
 	else if (ieee80211_is_group_privacy_action(tx->skb) &&
 		(key = rcu_dereference(tx->sdata->default_multicast_key)))
@@ -657,6 +660,9 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 		if (!skip_hw && tx->key &&
 		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)
 			info->control.hw_key = &tx->key->conf;
+	} else if (!ieee80211_is_mgmt(hdr->frame_control) && tx->sta &&
+		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
+		return TX_DROP;
 	}
 
 	return TX_CONTINUE;
-- 
2.20.1

Re: [PATCH v2 11/12] mac80211: drop data frames without key on encrypted links

[Pali Rohár]

On Friday 11 June 2021 12:10:46 Pali Rohár wrote:
> On Friday 27 March 2020 15:03:41 Sasha Levin wrote:
> > This commit has been processed because it contains a -stable tag.
> > The stable tag indicates that it's relevant for the following trees: all
> > 
> > The bot has tested the following trees: v5.5.11, v5.4.27, v4.19.112, v4.14.174, v4.9.217, v4.4.217.
> > 
> > v5.5.11: Build OK!
> > v5.4.27: Build OK!
> > v4.19.112: Failed to apply! Possible dependencies:
> ...
> > v4.14.174: Failed to apply! Possible dependencies:
> ...
> > v4.9.217: Failed to apply! Possible dependencies:
> ...
> > v4.4.217: Failed to apply! Possible dependencies:
> ...
> > 
> > How should we proceed with this patch?
> 
> Hello! I have looked at this patch and backported it into 4.19 and older
> versions. But as this patch is security related and backporting needed
> some code changes, it is required to review this patch prior including
> it into any stable branch. Patch is below.

Hello Sasha and Greg!

Do you have any opinion how do you want to process this patch? I would
like to know if something else is needed from my side.

> The main change in backported patch is in ieee80211_key_replace()
> function.
> 
> So could you please review this patch if it is correct and if it is
> suitable for backporting it into stable kernels 4.19 in this (or other)
> form?

Johannes, you are the original author of this patch, what do you think,
would you be able to find some time and review at this backported patch?

> =======================================================================
> 
> From 189da5743e28d0c5d211b70b4cb06ce3aff77d86 Mon Sep 17 00:00:00 2001
> From: Johannes Berg <johannes.berg@intel.com>
> Date: Thu, 26 Mar 2020 15:09:42 +0200
> Subject: [PATCH] mac80211: drop data frames without key on encrypted links
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> commit a0761a301746ec2d92d7fcb82af69c0a6a4339aa upstream.
> 
> If we know that we have an encrypted link (based on having had
> a key configured for TX in the past) then drop all data frames
> in the key selection handler if there's no key anymore.
> 
> This fixes an issue with mac80211 internal TXQs - there we can
> buffer frames for an encrypted link, but then if the key is no
> longer there when they're dequeued, the frames are sent without
> encryption. This happens if a station is disconnected while the
> frames are still on the TXQ.
> 
> Detecting that a link should be encrypted based on a first key
> having been configured for TX is fine as there are no use cases
> for a connection going from with encryption to no encryption.
> With extended key IDs, however, there is a case of having a key
> configured for only decryption, so we can't just trigger this
> behaviour on a key being configured.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Jouni Malinen <j@w1.fi>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> Link: https://lore.kernel.org/r/iwlwifi.20200326150855.6865c7f28a14.I9fb1d911b064262d33e33dfba730cdeef83926ca@changeid
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> [pali: Backported to 4.19 and older versions]
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  net/mac80211/debugfs_sta.c |  1 +
>  net/mac80211/key.c         |  7 +++++--
>  net/mac80211/sta_info.h    |  1 +
>  net/mac80211/tx.c          | 12 +++++++++---
>  4 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
> index 4105081dc1df..6f390c2e4c8e 100644
> --- a/net/mac80211/debugfs_sta.c
> +++ b/net/mac80211/debugfs_sta.c
> @@ -80,6 +80,7 @@ static const char * const sta_flag_names[] = {
>  	FLAG(MPSP_OWNER),
>  	FLAG(MPSP_RECIPIENT),
>  	FLAG(PS_DELIVER),
> +	FLAG(USES_ENCRYPTION),
>  #undef FLAG
>  };
>  
> diff --git a/net/mac80211/key.c b/net/mac80211/key.c
> index f20bb39f492d..217db25a1afa 100644
> --- a/net/mac80211/key.c
> +++ b/net/mac80211/key.c
> @@ -341,8 +341,11 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
>  	if (sta) {
>  		if (pairwise) {
>  			rcu_assign_pointer(sta->ptk[idx], new);
> -			sta->ptk_idx = idx;
> -			ieee80211_check_fast_xmit(sta);
> +			if (new) {
> +				set_sta_flag(new->sta, WLAN_STA_USES_ENCRYPTION);
> +				new->sta->ptk_idx = new->conf.keyidx;
> +				ieee80211_check_fast_xmit(new->sta);
> +			}
>  		} else {
>  			rcu_assign_pointer(sta->gtk[idx], new);
>  		}
> diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
> index 9a04327d71d1..075609c4571d 100644
> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -101,6 +101,7 @@ enum ieee80211_sta_info_flags {
>  	WLAN_STA_MPSP_OWNER,
>  	WLAN_STA_MPSP_RECIPIENT,
>  	WLAN_STA_PS_DELIVER,
> +	WLAN_STA_USES_ENCRYPTION,
>  
>  	NUM_WLAN_STA_FLAGS,
>  };
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 98d048630ad2..3530d1a5fc98 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -593,10 +593,13 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
>  	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
>  
> -	if (unlikely(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT))
> +	if (unlikely(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)) {
>  		tx->key = NULL;
> -	else if (tx->sta &&
> -		 (key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx])))
> +		return TX_CONTINUE;
> +	}
> +
> +	if (tx->sta &&
> +	    (key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx])))
>  		tx->key = key;
>  	else if (ieee80211_is_group_privacy_action(tx->skb) &&
>  		(key = rcu_dereference(tx->sdata->default_multicast_key)))
> @@ -657,6 +660,9 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
>  		if (!skip_hw && tx->key &&
>  		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)
>  			info->control.hw_key = &tx->key->conf;
> +	} else if (!ieee80211_is_mgmt(hdr->frame_control) && tx->sta &&
> +		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
> +		return TX_DROP;
>  	}
>  
>  	return TX_CONTINUE;
> -- 
> 2.20.1

Re: [PATCH v2 11/12] mac80211: drop data frames without key on encrypted links

[Johannes Berg]

On Fri, 2021-06-11 at 12:10 +0200, Pali Rohár wrote:
> 
> @@ -341,8 +341,11 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
>  	if (sta) {
>  		if (pairwise) {
>  			rcu_assign_pointer(sta->ptk[idx], new);
> -			sta->ptk_idx = idx;
> -			ieee80211_check_fast_xmit(sta);
> +			if (new) {
> +				set_sta_flag(new->sta, WLAN_STA_USES_ENCRYPTION);
> +				new->sta->ptk_idx = new->conf.keyidx;

I'm not entirely sure moving that assignment under the guard is correct.

> +				ieee80211_check_fast_xmit(new->sta);

and I'm pretty sure that moving call under the guard is incorrect,
although in the end it probably doesn't even matter if we will drop all
frames anyway (due to this patch).

So all you need under the assignment is the flag, but also only
theoretically, because the function cannot be called with old==NULL &&
new==NULL, the first time around it's called we must have old==NULL (no
key was ever installed), and so the first time it's called it must be
old==NULL && new!=NULL, and then the flag gets set and we never want to
clear it again, so I believe you don't need the "if (new)" condition at
all.

In the code as it was in (and before) my patch the condition is
necessary because we use 'new' to obtain the 'sta' and 'local' pointers,
but otherwise we don't really need it even in the current version.

johannes

Re: [PATCH v2 11/12] mac80211: drop data frames without key on encrypted links

[Greg KH]

On Wed, Jun 23, 2021 at 01:15:11AM +0200, Pali Rohár wrote:
> On Friday 11 June 2021 12:10:46 Pali Rohár wrote:
> > On Friday 27 March 2020 15:03:41 Sasha Levin wrote:
> > > This commit has been processed because it contains a -stable tag.
> > > The stable tag indicates that it's relevant for the following trees: all
> > > 
> > > The bot has tested the following trees: v5.5.11, v5.4.27, v4.19.112, v4.14.174, v4.9.217, v4.4.217.
> > > 
> > > v5.5.11: Build OK!
> > > v5.4.27: Build OK!
> > > v4.19.112: Failed to apply! Possible dependencies:
> > ...
> > > v4.14.174: Failed to apply! Possible dependencies:
> > ...
> > > v4.9.217: Failed to apply! Possible dependencies:
> > ...
> > > v4.4.217: Failed to apply! Possible dependencies:
> > ...
> > > 
> > > How should we proceed with this patch?
> > 
> > Hello! I have looked at this patch and backported it into 4.19 and older
> > versions. But as this patch is security related and backporting needed
> > some code changes, it is required to review this patch prior including
> > it into any stable branch. Patch is below.
> 
> Hello Sasha and Greg!
> 
> Do you have any opinion how do you want to process this patch? I would
> like to know if something else is needed from my side.

If it is not applied, please resend it, it's not in any queue that I can
see...

thanks,

greg k-h

Re: [PATCH v2 11/12] mac80211: drop data frames without key on encrypted links

[Pali Rohár]

On Wednesday 23 June 2021 14:16:12 Johannes Berg wrote:
> On Fri, 2021-06-11 at 12:10 +0200, Pali Rohár wrote:
> > 
> > @@ -341,8 +341,11 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
> >  	if (sta) {
> >  		if (pairwise) {
> >  			rcu_assign_pointer(sta->ptk[idx], new);
> > -			sta->ptk_idx = idx;
> > -			ieee80211_check_fast_xmit(sta);
> > +			if (new) {
> > +				set_sta_flag(new->sta, WLAN_STA_USES_ENCRYPTION);
> > +				new->sta->ptk_idx = new->conf.keyidx;
> 
> I'm not entirely sure moving that assignment under the guard is correct.
> 
> > +				ieee80211_check_fast_xmit(new->sta);
> 
> and I'm pretty sure that moving call under the guard is incorrect,
> although in the end it probably doesn't even matter if we will drop all
> frames anyway (due to this patch).
> 
> So all you need under the assignment is the flag, but also only
> theoretically, because the function cannot be called with old==NULL &&
> new==NULL, the first time around it's called we must have old==NULL (no
> key was ever installed), and so the first time it's called it must be
> old==NULL && new!=NULL, and then the flag gets set and we never want to
> clear it again, so I believe you don't need the "if (new)" condition at
> all.
> 
> In the code as it was in (and before) my patch the condition is
> necessary because we use 'new' to obtain the 'sta' and 'local' pointers,
> but otherwise we don't really need it even in the current version.
> 
> johannes

Now I see, thank you for explanation. So the code should be like this:

 		if (pairwise) {
 			rcu_assign_pointer(sta->ptk[idx], new);
+			set_sta_flag(sta, WLAN_STA_USES_ENCRYPTION);
 			sta->ptk_idx = idx;
 			ieee80211_check_fast_xmit(sta);
 		} else {

Right?

Re: [PATCH v2 11/12] mac80211: drop data frames without key on encrypted links

[Johannes Berg]

On Tue, 2021-06-29 at 23:32 +0200, Pali Rohár wrote:
> On Wednesday 23 June 2021 14:16:12 Johannes Berg wrote:
> > On Fri, 2021-06-11 at 12:10 +0200, Pali Rohár wrote:
> > > 
> > > @@ -341,8 +341,11 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
> > >  	if (sta) {
> > >  		if (pairwise) {
> > >  			rcu_assign_pointer(sta->ptk[idx], new);
> > > -			sta->ptk_idx = idx;
> > > -			ieee80211_check_fast_xmit(sta);
> > > +			if (new) {
> > > +				set_sta_flag(new->sta, WLAN_STA_USES_ENCRYPTION);
> > > +				new->sta->ptk_idx = new->conf.keyidx;
> > 
> > I'm not entirely sure moving that assignment under the guard is correct.
> > 
> > > +				ieee80211_check_fast_xmit(new->sta);
> > 
> > and I'm pretty sure that moving call under the guard is incorrect,
> > although in the end it probably doesn't even matter if we will drop all
> > frames anyway (due to this patch).
> > 
> > So all you need under the assignment is the flag, but also only
> > theoretically, because the function cannot be called with old==NULL &&
> > new==NULL, the first time around it's called we must have old==NULL (no
> > key was ever installed), and so the first time it's called it must be
> > old==NULL && new!=NULL, and then the flag gets set and we never want to
> > clear it again, so I believe you don't need the "if (new)" condition at
> > all.
> > 
> > In the code as it was in (and before) my patch the condition is
> > necessary because we use 'new' to obtain the 'sta' and 'local' pointers,
> > but otherwise we don't really need it even in the current version.
> > 
> > johannes
> 
> Now I see, thank you for explanation. So the code should be like this:
> 
>  		if (pairwise) {
>  			rcu_assign_pointer(sta->ptk[idx], new);
> +			set_sta_flag(sta, WLAN_STA_USES_ENCRYPTION);
>  			sta->ptk_idx = idx;
>  			ieee80211_check_fast_xmit(sta);
>  		} else {
> 
> Right?

Yes, I think so.

johannes

[PATCH] mac80211: drop data frames without key on encrypted links

[Pali Rohár]

From: Johannes Berg <johannes.berg@intel.com>

commit a0761a301746ec2d92d7fcb82af69c0a6a4339aa upstream.

If we know that we have an encrypted link (based on having had
a key configured for TX in the past) then drop all data frames
in the key selection handler if there's no key anymore.

This fixes an issue with mac80211 internal TXQs - there we can
buffer frames for an encrypted link, but then if the key is no
longer there when they're dequeued, the frames are sent without
encryption. This happens if a station is disconnected while the
frames are still on the TXQ.

Detecting that a link should be encrypted based on a first key
having been configured for TX is fine as there are no use cases
for a connection going from with encryption to no encryption.
With extended key IDs, however, there is a case of having a key
configured for only decryption, so we can't just trigger this
behaviour on a key being configured.

Cc: stable@vger.kernel.org
Reported-by: Jouni Malinen <j@w1.fi>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
Link: https://lore.kernel.org/r/iwlwifi.20200326150855.6865c7f28a14.I9fb1d911b064262d33e33dfba730cdeef83926ca@changeid
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
[pali: Backported to 4.19 and older versions]
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 net/mac80211/debugfs_sta.c |  1 +
 net/mac80211/key.c         |  1 +
 net/mac80211/sta_info.h    |  1 +
 net/mac80211/tx.c          | 12 +++++++++---
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 4105081dc1df..6f390c2e4c8e 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -80,6 +80,7 @@ static const char * const sta_flag_names[] = {
 	FLAG(MPSP_OWNER),
 	FLAG(MPSP_RECIPIENT),
 	FLAG(PS_DELIVER),
+	FLAG(USES_ENCRYPTION),
 #undef FLAG
 };
 
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 6775d6cb7d3d..7fc55177db84 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -341,6 +341,7 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
 	if (sta) {
 		if (pairwise) {
 			rcu_assign_pointer(sta->ptk[idx], new);
+			set_sta_flag(sta, WLAN_STA_USES_ENCRYPTION);
 			sta->ptk_idx = idx;
 			ieee80211_check_fast_xmit(sta);
 		} else {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index c33bc5fc0f2d..75d982ff7f3d 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -102,6 +102,7 @@ enum ieee80211_sta_info_flags {
 	WLAN_STA_MPSP_OWNER,
 	WLAN_STA_MPSP_RECIPIENT,
 	WLAN_STA_PS_DELIVER,
+	WLAN_STA_USES_ENCRYPTION,
 
 	NUM_WLAN_STA_FLAGS,
 };
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 98d048630ad2..3530d1a5fc98 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -593,10 +593,13 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
 
-	if (unlikely(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT))
+	if (unlikely(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)) {
 		tx->key = NULL;
-	else if (tx->sta &&
-		 (key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx])))
+		return TX_CONTINUE;
+	}
+
+	if (tx->sta &&
+	    (key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx])))
 		tx->key = key;
 	else if (ieee80211_is_group_privacy_action(tx->skb) &&
 		(key = rcu_dereference(tx->sdata->default_multicast_key)))
@@ -657,6 +660,9 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 		if (!skip_hw && tx->key &&
 		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)
 			info->control.hw_key = &tx->key->conf;
+	} else if (!ieee80211_is_mgmt(hdr->frame_control) && tx->sta &&
+		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
+		return TX_DROP;
 	}
 
 	return TX_CONTINUE;
-- 
2.20.1

Re: [PATCH] mac80211: drop data frames without key on encrypted links

[Greg KH]

On Mon, Aug 16, 2021 at 03:44:24PM +0200, Pali Rohár wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> commit a0761a301746ec2d92d7fcb82af69c0a6a4339aa upstream.
> 
> If we know that we have an encrypted link (based on having had
> a key configured for TX in the past) then drop all data frames
> in the key selection handler if there's no key anymore.
> 
> This fixes an issue with mac80211 internal TXQs - there we can
> buffer frames for an encrypted link, but then if the key is no
> longer there when they're dequeued, the frames are sent without
> encryption. This happens if a station is disconnected while the
> frames are still on the TXQ.
> 
> Detecting that a link should be encrypted based on a first key
> having been configured for TX is fine as there are no use cases
> for a connection going from with encryption to no encryption.
> With extended key IDs, however, there is a case of having a key
> configured for only decryption, so we can't just trigger this
> behaviour on a key being configured.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Jouni Malinen <j@w1.fi>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> Link: https://lore.kernel.org/r/iwlwifi.20200326150855.6865c7f28a14.I9fb1d911b064262d33e33dfba730cdeef83926ca@changeid
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> [pali: Backported to 4.19 and older versions]
> Signed-off-by: Pali Rohár <pali@kernel.org>

Now queued up, thanks!

Did not apply to 4.4.y, don't know if you want it there or not...

thanks,

greg k-h