diff mbox series

[V2] rsi: fix AP mode with WPA failure due to encrypted EAPOL

Message ID 1622564459-24430-1-git-send-email-martin.fuzzey@flowbird.group
State Accepted
Commit 314538041b5632ffaf64798faaeabaf2793fe029
Delegated to: Kalle Valo
Headers show
Series [V2] rsi: fix AP mode with WPA failure due to encrypted EAPOL | expand

Commit Message

Martin Fuzzey June 1, 2021, 4:19 p.m. UTC
In AP mode WPA2-PSK connections were not established.

The reason was that the AP was sending the first message
of the 4 way handshake encrypted, even though no pairwise
key had (correctly) yet been set.

Encryption was enabled if the "security_enable" driver flag
was set and encryption was not explicitly disabled by
IEEE80211_TX_INTFL_DONT_ENCRYPT.

However security_enable was set when *any* key, including
the AP GTK key, had been set which was causing unwanted
encryption even if no key was avaialble for the unicast
packet to be sent.

Fix this by adding a check that we have a key and drop
the old security_enable driver flag which is insufficient
and redundant.

The Redpine downstream out of tree driver does it this way too.

Regarding the Fixes tag the actual code being modified was
introduced earlier, with the original driver submission, in
dad0d04fa7ba ("rsi: Add RS9113 wireless driver"), however
at that time AP mode was not yet supported so there was
no bug at that point.

So I have tagged the introduction of AP support instead
which was part of the patch set "rsi: support for AP mode" [1]

It is not clear whether AP WPA has ever worked, I can see nothing
on the kernel side that broke it afterwards yet the AP support
patch series says "Tests are performed to confirm aggregation,
connections in WEP and WPA/WPA2 security."

One possibility is that the initial tests were done with a modified
userspace (hostapd).

[1] https://www.spinics.net/lists/linux-wireless/msg165302.html

Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
Fixes: 38ef62353acb ("rsi: security enhancements for AP mode")
CC: stable@vger.kernel.org
---

V2:
        Remove security_enable driver flag
        Remove unnecessary parantheses
        Improve $SUBJECT

 drivers/net/wireless/rsi/rsi_91x_hal.c      | 2 +-
 drivers/net/wireless/rsi/rsi_91x_mac80211.c | 3 ---
 drivers/net/wireless/rsi/rsi_91x_mgmt.c     | 3 +--
 drivers/net/wireless/rsi/rsi_main.h         | 1 -
 4 files changed, 2 insertions(+), 7 deletions(-)

Comments

Kalle Valo June 15, 2021, 1:42 p.m. UTC | #1
Martin Fuzzey <martin.fuzzey@flowbird.group> wrote:

> In AP mode WPA2-PSK connections were not established.
> 
> The reason was that the AP was sending the first message
> of the 4 way handshake encrypted, even though no pairwise
> key had (correctly) yet been set.
> 
> Encryption was enabled if the "security_enable" driver flag
> was set and encryption was not explicitly disabled by
> IEEE80211_TX_INTFL_DONT_ENCRYPT.
> 
> However security_enable was set when *any* key, including
> the AP GTK key, had been set which was causing unwanted
> encryption even if no key was avaialble for the unicast
> packet to be sent.
> 
> Fix this by adding a check that we have a key and drop
> the old security_enable driver flag which is insufficient
> and redundant.
> 
> The Redpine downstream out of tree driver does it this way too.
> 
> Regarding the Fixes tag the actual code being modified was
> introduced earlier, with the original driver submission, in
> dad0d04fa7ba ("rsi: Add RS9113 wireless driver"), however
> at that time AP mode was not yet supported so there was
> no bug at that point.
> 
> So I have tagged the introduction of AP support instead
> which was part of the patch set "rsi: support for AP mode" [1]
> 
> It is not clear whether AP WPA has ever worked, I can see nothing
> on the kernel side that broke it afterwards yet the AP support
> patch series says "Tests are performed to confirm aggregation,
> connections in WEP and WPA/WPA2 security."
> 
> One possibility is that the initial tests were done with a modified
> userspace (hostapd).
> 
> [1] https://www.spinics.net/lists/linux-wireless/msg165302.html
> 
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> Fixes: 38ef62353acb ("rsi: security enhancements for AP mode")
> CC: stable@vger.kernel.org

Patch applied to wireless-drivers-next.git, thanks.

314538041b56 rsi: fix AP mode with WPA failure due to encrypted EAPOL
diff mbox series

Patch

diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
index ce98921..a51cb0a 100644
--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -203,7 +203,7 @@  int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
 		wh->frame_control |= cpu_to_le16(RSI_SET_PS_ENABLE);
 
 	if ((!(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)) &&
-	    (common->secinfo.security_enable)) {
+	    info->control.hw_key) {
 		if (rsi_is_cipher_wep(common))
 			ieee80211_size += 4;
 		else
diff --git a/drivers/net/wireless/rsi/rsi_91x_mac80211.c b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
index 1602530..57c9e35 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
@@ -1028,7 +1028,6 @@  static int rsi_mac80211_set_key(struct ieee80211_hw *hw,
 	mutex_lock(&common->mutex);
 	switch (cmd) {
 	case SET_KEY:
-		secinfo->security_enable = true;
 		status = rsi_hal_key_config(hw, vif, key, sta);
 		if (status) {
 			mutex_unlock(&common->mutex);
@@ -1047,8 +1046,6 @@  static int rsi_mac80211_set_key(struct ieee80211_hw *hw,
 		break;
 
 	case DISABLE_KEY:
-		if (vif->type == NL80211_IFTYPE_STATION)
-			secinfo->security_enable = false;
 		rsi_dbg(ERR_ZONE, "%s: RSI del key\n", __func__);
 		memset(key, 0, sizeof(struct ieee80211_key_conf));
 		status = rsi_hal_key_config(hw, vif, key, sta);
diff --git a/drivers/net/wireless/rsi/rsi_91x_mgmt.c b/drivers/net/wireless/rsi/rsi_91x_mgmt.c
index 33c76d3..b6d050a 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mgmt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mgmt.c
@@ -1803,8 +1803,7 @@  int rsi_send_wowlan_request(struct rsi_common *common, u16 flags,
 			RSI_WIFI_MGMT_Q);
 	cmd_frame->desc.desc_dword0.frame_type = WOWLAN_CONFIG_PARAMS;
 	cmd_frame->host_sleep_status = sleep_status;
-	if (common->secinfo.security_enable &&
-	    common->secinfo.gtk_cipher)
+	if (common->secinfo.gtk_cipher)
 		flags |= RSI_WOW_GTK_REKEY;
 	if (sleep_status)
 		cmd_frame->wow_flags = flags;
diff --git a/drivers/net/wireless/rsi/rsi_main.h b/drivers/net/wireless/rsi/rsi_main.h
index a1065e5..0f53585 100644
--- a/drivers/net/wireless/rsi/rsi_main.h
+++ b/drivers/net/wireless/rsi/rsi_main.h
@@ -151,7 +151,6 @@  enum edca_queue {
 };
 
 struct security_info {
-	bool security_enable;
 	u32 ptk_cipher;
 	u32 gtk_cipher;
 };