diff mbox series

[v9,08/16] drivers: Migrate drivers from set_tx to key_flag API

Message ID 20200104221015.90469-9-alexander@wetzel-home.de
State Changes Requested
Headers show
Series Seamless PTK rekeys | expand

Commit Message

Alexander Wetzel Jan. 4, 2020, 10:10 p.m. UTC 
Migrate all remaining drivers from set_tx to the key_flag API.

This patch assumes that the drivers are only using set_tx to handle
default non-pairwise keys as it was intended and don't need it to be set
for pairwise and/or group keys which are RX only.

Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---
 src/drivers/driver_atheros.c |  6 +++---
 src/drivers/driver_bsd.c     |  8 ++++----
 src/drivers/driver_hostap.c  |  3 ++-
 src/drivers/driver_ndis.c    | 12 ++++++++----
 src/drivers/driver_privsep.c |  6 +++---
 src/drivers/driver_wext.c    | 10 ++++++----
 6 files changed, 26 insertions(+), 19 deletions(-)

Comments

Jouni Malinen Jan. 6, 2020, 9:22 a.m. UTC | #1 
On Sat, Jan 04, 2020 at 11:10:07PM +0100, Alexander Wetzel wrote:
> Migrate all remaining drivers from set_tx to the key_flag API.
> 
> This patch assumes that the drivers are only using set_tx to handle
> default non-pairwise keys as it was intended and don't need it to be set
> for pairwise and/or group keys which are RX only.

Maybe this and removal of set_tx from the set_key() params struct should
be postponed until that assumption can actually be shown to be correct?
It does not look like either of these are really needed to be able to
address the real functional change here with nl80211 and I'm not exactly
keen on change driver_*.c behavior without anyone having tested the
changes. driver_wext.c may actually be covered in hwsim testing, but the
other ones would need manual testing if code review cannot clearly show
that there is no change in actually driver operations. Each driver_*.c
can be handled separately after having been tested or reviewed.
Alexander Wetzel Jan. 6, 2020, 1:56 p.m. UTC | #2 
Am 06.01.20 um 10:22 schrieb Jouni Malinen:
> On Sat, Jan 04, 2020 at 11:10:07PM +0100, Alexander Wetzel wrote:
>> Migrate all remaining drivers from set_tx to the key_flag API.
>>
>> This patch assumes that the drivers are only using set_tx to handle
>> default non-pairwise keys as it was intended and don't need it to be set
>> for pairwise and/or group keys which are RX only.
> 
> Maybe this and removal of set_tx from the set_key() params struct should
> be postponed until that assumption can actually be shown to be correct?
> It does not look like either of these are really needed to be able to
> address the real functional change here with nl80211 and I'm not exactly
> keen on change driver_*.c behavior without anyone having tested the
> changes. driver_wext.c may actually be covered in hwsim testing, but the
> other ones would need manual testing if code review cannot clearly show
> that there is no change in actually driver operations. Each driver_*.c
> can be handled separately after having been tested or reviewed.
> 

Perfectly fine. Some of the older patch series had the set_tx removal at 
the very end of the series. I'll migrate the API to the parameter struct 
with set_tx and drop this patch from the next series.

Alexander
Jouni Malinen Jan. 8, 2020, 8:18 p.m. UTC | #3 
On Mon, Jan 06, 2020 at 02:56:07PM +0100, Alexander Wetzel wrote:
> Perfectly fine. Some of the older patch series had the set_tx removal at the
> very end of the series. I'll migrate the API to the parameter struct with
> set_tx and drop this patch from the next series.

Please note that I need to add another set_key() parameter for a
different pending patch and instead of doing that with a new function
argument, I'll do it with this struct conversion, so I'll have this part
ready without additional patches needed.
diff mbox series

Patch

diff --git a/src/drivers/driver_atheros.c b/src/drivers/driver_atheros.c
index 4d9e9011c..52e0e1278 100644
--- a/src/drivers/driver_atheros.c
+++ b/src/drivers/driver_atheros.c
@@ -561,7 +561,7 @@  atheros_set_key(const char *ifname, void *priv, enum wpa_alg alg,
 	if (addr == NULL || is_broadcast_ether_addr(addr)) {
 		os_memset(wk.ik_macaddr, 0xff, IEEE80211_ADDR_LEN);
 		wk.ik_keyix = key_idx;
-		if (set_tx)
+		if (key_flag & KEY_FLAG_DEFAULT)
 			wk.ik_flags |= IEEE80211_KEY_DEFAULT;
 	} else {
 		os_memcpy(wk.ik_macaddr, addr, IEEE80211_ADDR_LEN);
@@ -573,9 +573,9 @@  atheros_set_key(const char *ifname, void *priv, enum wpa_alg alg,
 	ret = set80211priv(drv, IEEE80211_IOCTL_SETKEY, &wk, sizeof(wk));
 	if (ret < 0) {
 		wpa_printf(MSG_DEBUG, "%s: Failed to set key (addr %s"
-			   " key_idx %d alg %d key_len %lu set_tx %d)",
+			   " key_idx %d alg %d key_len %lu key_flag %d)",
 			   __func__, ether_sprintf(wk.ik_macaddr), key_idx,
-			   alg, (unsigned long) key_len, set_tx);
+			   alg, (unsigned long) key_len, key_flag);
 	}
 
 	return ret;
diff --git a/src/drivers/driver_bsd.c b/src/drivers/driver_bsd.c
index 5ee6eb730..ec7d2629e 100644
--- a/src/drivers/driver_bsd.c
+++ b/src/drivers/driver_bsd.c
@@ -341,9 +341,9 @@  bsd_set_key(const char *ifname, void *priv, enum wpa_alg alg,
 	struct bsd_driver_data *drv = priv;
 #endif /* IEEE80211_KEY_NOREPLAY */
 
-	wpa_printf(MSG_DEBUG, "%s: alg=%d addr=%p key_idx=%d set_tx=%d "
+	wpa_printf(MSG_DEBUG, "%s: alg=%d addr=%p key_idx=%d key_flag=%d "
 		   "seq_len=%zu key_len=%zu", __func__, alg, addr, key_idx,
-		   set_tx, seq_len, key_len);
+		   key_flag, seq_len, key_len);
 
 	if (alg == WPA_ALG_NONE) {
 #ifndef HOSTAPD
@@ -371,7 +371,7 @@  bsd_set_key(const char *ifname, void *priv, enum wpa_alg alg,
 	}
 
 	wk.ik_flags = IEEE80211_KEY_RECV;
-	if (set_tx)
+	if (key_flag & KEY_FLAG_DEFAULT)
 		wk.ik_flags |= IEEE80211_KEY_XMIT;
 
 	if (addr == NULL) {
@@ -392,7 +392,7 @@  bsd_set_key(const char *ifname, void *priv, enum wpa_alg alg,
 				key_idx;
 		}
 	}
-	if (wk.ik_keyix != IEEE80211_KEYIX_NONE && set_tx)
+	if (wk.ik_keyix != IEEE80211_KEYIX_NONE && key_flag & KEY_FLAG_DEFAULT)
 		wk.ik_flags |= IEEE80211_KEY_DEFAULT;
 #ifndef HOSTAPD
 #ifdef IEEE80211_KEY_NOREPLAY
diff --git a/src/drivers/driver_hostap.c b/src/drivers/driver_hostap.c
index 818188ada..bb7284d9e 100644
--- a/src/drivers/driver_hostap.c
+++ b/src/drivers/driver_hostap.c
@@ -441,7 +441,8 @@  static int wpa_driver_hostap_set_key(const char *ifname, void *priv,
 		os_free(buf);
 		return -1;
 	}
-	param->u.crypt.flags = set_tx ? HOSTAP_CRYPT_FLAG_SET_TX_KEY : 0;
+	param->u.crypt.flags = key_flag & KEY_FLAG_DEFAULT ?
+			       HOSTAP_CRYPT_FLAG_SET_TX_KEY : 0;
 	param->u.crypt.idx = key_idx;
 	param->u.crypt.key_len = key_len;
 	memcpy((u8 *) (param + 1), key, key_len);
diff --git a/src/drivers/driver_ndis.c b/src/drivers/driver_ndis.c
index 3f542e8ee..4acfb30f3 100644
--- a/src/drivers/driver_ndis.c
+++ b/src/drivers/driver_ndis.c
@@ -945,7 +945,7 @@  static int wpa_driver_ndis_add_wep(struct wpa_driver_ndis_data *drv,
 		return -1;
 	wep->Length = len;
 	wep->KeyIndex = key_idx;
-	if (set_tx)
+	if (key_flag & KEY_FLAG_DEFAULT)
 		wep->KeyIndex |= 1 << 31;
 #if 0 /* Setting bit30 does not seem to work with some NDIS drivers */
 	if (pairwise)
@@ -1006,7 +1006,7 @@  static int wpa_driver_ndis_set_key(const char *ifname, void *priv,
 
 	nkey->Length = len;
 	nkey->KeyIndex = key_idx;
-	if (set_tx)
+	if (key_flag & KEY_FLAG_DEFAULT)
 		nkey->KeyIndex |= 1 << 31;
 	if (pairwise)
 		nkey->KeyIndex |= 1 << 30;
@@ -1077,7 +1077,10 @@  wpa_driver_ndis_associate(void *priv,
 						bcast, i,
 						i == params->wep_tx_keyidx,
 						NULL, 0, params->wep_key[i],
-						params->wep_key_len[i], 0);
+						params->wep_key_len[i],
+						i == params->wep_tx_keyidx ?
+						     KEY_FLAG_GROUP_RX_TX_DEFAULT :
+						     KEY_FLAG_GROUP_RX_TX);
 		}
 	}
 
@@ -1114,7 +1117,8 @@  wpa_driver_ndis_associate(void *priv,
 			wpa_driver_ndis_set_key(drv->ifname, drv, WPA_ALG_WEP,
 						bcast, 0, 1,
 						NULL, 0, dummy_key,
-						sizeof(dummy_key), 0);
+						sizeof(dummy_key),
+						KEY_FLAG_GROUP_RX_TX_DEFAULT);
 		}
 #endif /* CONFIG_WPS */
 	} else {
diff --git a/src/drivers/driver_privsep.c b/src/drivers/driver_privsep.c
index f8840f848..568de3baf 100644
--- a/src/drivers/driver_privsep.c
+++ b/src/drivers/driver_privsep.c
@@ -215,8 +215,8 @@  static int wpa_driver_privsep_set_key(const char *ifname, void *priv,
 	struct wpa_driver_privsep_data *drv = priv;
 	struct privsep_cmd_set_key cmd;
 
-	wpa_printf(MSG_DEBUG, "%s: priv=%p alg=%d key_idx=%d set_tx=%d",
-		   __func__, priv, alg, key_idx, set_tx);
+	wpa_printf(MSG_DEBUG, "%s: priv=%p alg=%d key_idx=%d key_flag=%d",
+		   __func__, priv, alg, key_idx, key_flag);
 
 	os_memset(&cmd, 0, sizeof(cmd));
 	cmd.alg = alg;
@@ -225,7 +225,7 @@  static int wpa_driver_privsep_set_key(const char *ifname, void *priv,
 	else
 		os_memset(cmd.addr, 0xff, ETH_ALEN);
 	cmd.key_idx = key_idx;
-	cmd.set_tx = set_tx;
+	cmd.set_tx = key_flag & KEY_FLAG_DEFAULT;
 	if (seq && seq_len > 0 && seq_len < sizeof(cmd.seq)) {
 		os_memcpy(cmd.seq, seq, seq_len);
 		cmd.seq_len = seq_len;
diff --git a/src/drivers/driver_wext.c b/src/drivers/driver_wext.c
index 368443f57..ade8eed4e 100644
--- a/src/drivers/driver_wext.c
+++ b/src/drivers/driver_wext.c
@@ -1740,7 +1740,7 @@  static int wpa_driver_wext_set_key_ext(void *priv, enum wpa_alg alg,
 
 	if (addr == NULL || is_broadcast_ether_addr(addr))
 		ext->ext_flags |= IW_ENCODE_EXT_GROUP_KEY;
-	if (set_tx)
+	if (key_flag & KEY_FLAG_DEFAULT)
 		ext->ext_flags |= IW_ENCODE_EXT_SET_TX_KEY;
 
 	ext->addr.sa_family = ARPHRD_ETHER;
@@ -1822,6 +1822,8 @@  static int wpa_driver_wext_set_key_ext(void *priv, enum wpa_alg alg,
  *	8-byte Rx Mic Key
  * @key_len: Length of the key buffer in octets (WEP: 5 or 13,
  *	TKIP: 32, CCMP: 16)
+ * @key_flag: Additional key information. Only KEY_FLAG_DEFAULT is supported
+ *	to mark group/broadcast default Tx keys.
  * Returns: 0 on success, -1 on failure
  *
  * This function uses SIOCSIWENCODEEXT by default, but tries to use
@@ -1837,9 +1839,9 @@  int wpa_driver_wext_set_key(const char *ifname, void *priv, enum wpa_alg alg,
 	struct iwreq iwr;
 	int ret = 0;
 
-	wpa_printf(MSG_DEBUG, "%s: alg=%d key_idx=%d set_tx=%d seq_len=%lu "
+	wpa_printf(MSG_DEBUG, "%s: alg=%d key_idx=%d key_flag=%d seq_len=%lu "
 		   "key_len=%lu",
-		   __FUNCTION__, alg, key_idx, set_tx,
+		   __FUNCTION__, alg, key_idx, key_flag,
 		   (unsigned long) seq_len, (unsigned long) key_len);
 
 	ret = wpa_driver_wext_set_key_ext(drv, alg, addr, key_idx, set_tx,
@@ -1873,7 +1875,7 @@  int wpa_driver_wext_set_key(const char *ifname, void *priv, enum wpa_alg alg,
 		ret = -1;
 	}
 
-	if (set_tx && alg != WPA_ALG_NONE) {
+	if ((key_flag & KEY_FLAG_DEFAULT) && alg != WPA_ALG_NONE) {
 		os_memset(&iwr, 0, sizeof(iwr));
 		os_strlcpy(iwr.ifr_name, drv->ifname, IFNAMSIZ);
 		iwr.u.encoding.flags = key_idx + 1;