d80211: hardware TKIP support for ipw3945

PATCH In review Hong Liu 7 participants 22 messages Oct 20, 2006 – Nov 16, 2006

Subsystems (3)

Networking Network drivers Networking drivers (wireless)

lore

Share & tools

lore

Hong Liu

author

2006-10-20 09:26 UTC #0 lore raw

PATCH 1/2 d80211: hardware TKIP support for ipw3945

ipw3945 TKIP hwcrypto only support RC4 encryption,
so the stack needs to pre compute the michael MIC and the RC4key for it.

Signed-off-by Hong Liu <[email protected]>

---

 include/net/d80211.h |    7 +++++++
 net/d80211/tkip.c    |   25 +++++++++++++++----------
 net/d80211/tkip.h    |    2 ++
 net/d80211/wpa.c     |   10 +++++++++-
 4 files changed, 33 insertions(+), 11 deletions(-)

a759f6251a0a5c7a8529c2ebd579acd6f0183cb4

include/net/d80211.h +7 -0

index a80f48b..48e1e01 100644
@@ -176,6 +176,7 @@ struct ieee80211_tx_control {
 			*/
 	int icv_len:8; /* Length of the ICV/MIC field in octets */
 	int iv_len:8; /* Length of the IV field in octets */
+	u8 rc4key[16]; /* generated RC4 key for hw TKIP */
 	unsigned int queue:4; /* hardware queue to use for this frame;
 		      * 0 = highest, hw->queues-1 = lowest */
 	unsigned int sw_retry_attempt:4; /* no. of times hw has tried to

@@ -476,6 +477,12 @@ struct ieee80211_hw {
 	/* Force software encryption for TKIP packets if WMM is enabled. */
 	unsigned int no_tkip_wmm_hwaccel:1;
 
+	/* Do TKIP key mixing in stack, send the generated RC4 key with
+	 * with each TX frame */
+	unsigned int tkip_include_rc4key:1;
+	/* calculate michael MIC in stack */
+	unsigned int tkip_include_mmic:1;
+
 	/* set if the payload needs to be padded at even boundaries after the
 	 * header */
 	unsigned int extra_hdr_room:1;

net/d80211/tkip.c +15 ~~-10~~

index 7e3665a..c031814 100644
@@ -190,17 +190,9 @@ u8 * ieee80211_tkip_add_iv(u8 *pos, stru
 	return pos;
 }
 
-
-/* Encrypt packet payload with TKIP using @key. @pos is a pointer to the
- * beginning of the buffer containing payload. This payload must include
- * headroom of eight octets for IV and Ext. IV and taildroom of four octets
- * for ICV. @payload_len is the length of payload (_not_ including extra
- * headroom and tailroom). @ta is the transmitter addresses. */
-void ieee80211_tkip_encrypt_data(struct crypto_tfm *tfm, struct ieee80211_key *key,
-				 u8 *pos, size_t payload_len, u8 *ta)
+void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key,
+			       u8 *rc4key, u8* ta)
 {
-	u8 rc4key[16];
-
 	/* Calculate per-packet key */
 	if (key->u.tkip.iv16 == 0 || !key->u.tkip.tx_initialized) {
 		/* IV16 wrapped around - perform TKIP phase 1 */

@@ -212,6 +204,19 @@ void ieee80211_tkip_encrypt_data(struct 
 	tkip_mixing_phase2(key->u.tkip.p1k, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
 			   key->u.tkip.iv16, rc4key);
 
+}
+
+/* Encrypt packet payload with TKIP using @key. @pos is a pointer to the
+ * beginning of the buffer containing payload. This payload must include
+ * headroom of eight octets for IV and Ext. IV and taildroom of four octets
+ * for ICV. @payload_len is the length of payload (_not_ including extra
+ * headroom and tailroom). @ta is the transmitter addresses. */
+void ieee80211_tkip_encrypt_data(struct crypto_tfm *tfm, struct ieee80211_key *key,
+				 u8 *pos, size_t payload_len, u8 *ta)
+{
+	u8 rc4key[16];
+
+	ieee80211_tkip_gen_rc4key(key, rc4key, ta);
 	pos = ieee80211_tkip_add_iv(pos, key, rc4key[0], rc4key[1], rc4key[2]);
 	ieee80211_wep_encrypt_data(tfm, rc4key, 16, pos, payload_len);
 }

net/d80211/tkip.h +2 -0

index e36b85c..407556b 100644
@@ -15,6 +15,8 @@ #include "ieee80211_key.h"
 
 u8 * ieee80211_tkip_add_iv(u8 *pos, struct ieee80211_key *key,
 			   u8 iv0, u8 iv1, u8 iv2);
+void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key,
+			       u8 *rc4key, u8* ta);
 void ieee80211_tkip_encrypt_data(struct crypto_tfm *tfm, struct ieee80211_key *key,
 				 u8 *pos, size_t payload_len, u8 *ta);
 enum {

net/d80211/wpa.c +9 -1

index 31abf9b..5e62464 100644
@@ -104,7 +104,8 @@ #ifdef CONFIG_HOSTAPD_WPA_TESTING
 #endif /* CONFIG_HOSTAPD_WPA_TESTING */
 
 	if (!tx->key->force_sw_encrypt && !tx->local->conf.sw_decrypt &&
-	    !tx->fragmented && !wpa_test) {
+	    !tx->fragmented && !tx->local->hw->tkip_include_mmic &&
+	    !wpa_test) {
 		/* hwaccel - with no need for preallocated room for Michael MIC
 		 */
 		return TXRX_CONTINUE;

@@ -336,6 +337,13 @@ #endif /* CONFIG_HOSTAPD_WPA_TESTING */
 					    0x7f),
 				      (u8) key->u.tkip.iv16);
 
+		if (tx->local->hw->tkip_include_rc4key) {
+			hdr = (struct ieee80211_hdr *)skb->data;
+			ieee80211_tkip_gen_rc4key(key,
+						  tx->u.tx.control->rc4key,
+						  hdr->addr2);
+		}
+
 		tx->u.tx.control->key_idx = tx->key->hw_key_idx;
 		return 0;
 	}

Signature · 1 line

1.3.3

Matthieu CASTET

2006-10-21 21:10 UTC #1 lore raw

Quoted 3 lines · Hong Liu

ipw3945 TKIP hwcrypto only support RC4 encryption,
so the stack needs to pre compute the michael MIC and the RC4key for it.

I suppose the hw doesn't do any hardware mic for the RX path. It believe it should be also added.

Jiri Benc

2006-10-23 12:40 UTC #2 lore raw

In reply to Hong Liu

include/net/d80211.h struct ieee80211_tx_control { · line 179

Quoted 7 lines · Hong Liu

include/net/d80211.h +1 -0

@@ -176,6 +176,7 @@ struct ieee80211_tx_control {
 			*/
 	int icv_len:8; /* Length of the ICV/MIC field in octets */
 	int iv_len:8; /* Length of the IV field in octets */
+	u8 rc4key[16]; /* generated RC4 key for hw TKIP */

I don't like extending ieee80211_tx_control by 16 more bytes. The driver is required to store a copy of each ieee80211_tx_control (because it's copied to ieee80211_tx_status). I don't have a better idea, though. Anybody?

include/net/d80211.h struct ieee80211_hw { · line 484

Quoted 9 lines

include/net/d80211.h +5 -0

@@ -476,6 +477,12 @@ struct ieee80211_hw {
 	/* Force software encryption for TKIP packets if WMM is enabled. */
 	unsigned int no_tkip_wmm_hwaccel:1;
 
+	/* Do TKIP key mixing in stack, send the generated RC4 key with
+	 * with each TX frame */
+	unsigned int tkip_include_rc4key:1;
+	/* calculate michael MIC in stack */
+	unsigned int tkip_include_mmic:1;

Please write more descriptive comments (e.g. there should be stated that tkip_include_mmic is relevant only when using hw crypto).

Also, it would help if you don't use bitfileds and rebase your patch on top of http://www.spinics.net/lists/netdev/msg17316.html and
http://www.spinics.net/lists/netdev/msg17314.html; but I can do it for you when applying the patch.

include/net/d80211.h struct ieee80211_hw { · line 486

Quoted 2 lines

include/net/d80211.h +2 -0

+void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key,
+			       u8 *rc4key, u8* ta)

Put the destination buffer (rc4key) as the last parameter.

Thanks,

Jiri

Signature · 2 lines

Jiri Benc
SUSE Labs

Johannes Berg

maintainer

2006-10-23 12:47 UTC #3 lore raw

Quoted 4 lines · Jiri Benc

I don't like extending ieee80211_tx_control by 16 more bytes. The driver is required to store a copy of each ieee80211_tx_control (because it's copied to ieee80211_tx_status). I don't have a better idea, though. Anybody?

A pointer that goes somewhere else? I suppose it could even be in the skb's cb field.

Quoted 2 lines

Please write more descriptive comments (e.g. there should be stated that tkip_include_mmic is relevant only when using hw crypto).

Also, something I just came to think of, bcm43xx does phase2 mixing in hw and requires phase1 in software. Do we handle that with or without this patch? If not, it'd be nice to fix it up together.

johannes

Jiri Benc

2006-10-23 12:56 UTC #4 lore raw

Quoted 8 lines · Johannes Berg

Quoted 4 lines · Jiri Benc
I don't like extending ieee80211_tx_control by 16 more bytes. The driver is required to store a copy of each ieee80211_tx_control (because it's copied to ieee80211_tx_status). I don't have a better idea, though. Anybody?
A pointer that goes somewhere else? I suppose it could even be in the skb's cb field.

Yes, that could work. Thanks!

Quoted 3 lines

Also, something I just came to think of, bcm43xx does phase2 mixing in hw and requires phase1 in software. Do we handle that with or without this patch? If not, it'd be nice to fix it up together.

It's not supported now. And it's really a good idea to extend the patch to support the bcm43xx case as well.

Jiri

Signature · 2 lines

Jiri Benc
SUSE Labs

Stuffed Crust

2006-10-23 13:01 UTC #19 lore raw

In reply to Jiri Benc

Quoted 8 lines · Jiri Benc

include/net/d80211.h struct ieee80211_tx_control { · line 179
Quoted 3 lines
include/net/d80211.h +1 -0
 	int icv_len:8; /* Length of the ICV/MIC field in octets */
 	int iv_len:8; /* Length of the IV field in octets */
+	u8 rc4key[16]; /* generated RC4 key for hw TKIP */
I don't like extending ieee80211_tx_control by 16 more bytes. The driver is required to store a copy of each ieee80211_tx_control (because it's copied to ieee80211_tx_status). I don't have a better idea, though. Anybody?

If the key isn't passed directly in via the tx_control structure, there needs to be a way for the driver to be able to get the key for that packet. Many chipsets won't care, but many expect the encryption key to be passed in with each packet. (For WEP, TKIP, or AES-CCMP)

Instead of expanding tx_control, adding an API call might suffice. with TKIP all you need to do the RC4 mixing is already in the packet (macaddr, EIV (keynum+PN), and payload. The stack just needs to be able to look up the PTK given the macaddr+keyidx, and the chipset driver can then derive whatever key it needs and stuff the hardware with it.

Crypto is the main way where wireless chipsets make life difficult for a common stack..

- Solomon

Signature · 3 lines

Solomon Peachy        		       pizza at shaftnet dot org	 
Melbourne, FL                          ^^ (mail/jabber/gtalk) ^^
Quidquid latine dictum sit, altum viditur.          ICQ: 1318344

David Kimdon

2006-10-23 15:29 UTC #20 lore raw

In reply to Jiri Benc

Quoted 13 lines · Jiri Benc

include/net/d80211.h struct ieee80211_tx_control { · line 179
Quoted 7 lines · Hong Liu
include/net/d80211.h +1 -0
@@ -176,6 +176,7 @@ struct ieee80211_tx_control {
 			*/
 	int icv_len:8; /* Length of the ICV/MIC field in octets */
 	int iv_len:8; /* Length of the IV field in octets */
+	u8 rc4key[16]; /* generated RC4 key for hw TKIP */
I don't like extending ieee80211_tx_control by 16 more bytes. The driver is required to store a copy of each ieee80211_tx_control (because it's copied to ieee80211_tx_status). I don't have a better idea, though. Anybody?

We could be more selective about what the driver is required to return in ieee80211_tx_status, the rc4key isn't particularily interesting to ieee80211_tx_status(). I expect there are other uninteresting fields (tx_rate, rts_cts_rate, come to mind, for example). We could put the fields we are interested in directly in ieee80211_tx_status, or have a new structure rather than re-using ieee80211_tx_control inside ieee80211_tx_status.

-David

Jiri Benc

2006-10-23 16:31 UTC #21 lore raw

Quoted 7 lines · David Kimdon

We could be more selective about what the driver is required to return in ieee80211_tx_status, the rc4key isn't particularily interesting to ieee80211_tx_status(). I expect there are other uninteresting fields (tx_rate, rts_cts_rate, come to mind, for example). We could put the fields we are interested in directly in ieee80211_tx_status, or have a new structure rather than re-using ieee80211_tx_control inside ieee80211_tx_status.

Something like this?

Jiri

---
 drivers/net/wireless/d80211/adm8211/adm8211.c |    7 +
 include/net/d80211.h                          |   28 ++++---
 net/d80211/ieee80211.c                        |   98 +++++++++++++-------------
 net/d80211/ieee80211_scan.c                   |    4 -
 net/d80211/ieee80211_sta.c                    |    4 -
 5 files changed, 73 insertions(+), 68 deletions(-)

drivers/net/wireless/d80211/adm8211/adm8211.c +4 -3

@@ -439,7 +439,7 @@ static void adm8211_interrupt_tci(struct
 		pci_unmap_single(priv->pdev, priv->tx_buffers[entry].mapping,
 				 priv->tx_buffers[entry].skb->len, PCI_DMA_TODEVICE);
 
-		if ((priv->tx_buffers[entry].tx_status.control.flags &
+		if ((priv->tx_buffers[entry].tx_status.common.flags &
 		     IEEE80211_TXCTL_REQ_TX_STATUS) ||
 		    !is_multicast_ether_addr(ieee80211_get_DA(&priv->tx_buffers[entry].hdr))) {
 			struct ieee80211_hdr *hdr;

@@ -1780,7 +1780,8 @@ static void adm8211_tx_raw(struct net_de
 
 	priv->tx_buffers[entry].skb = skb;
 	priv->tx_buffers[entry].mapping = mapping;
-	memcpy(&priv->tx_buffers[entry].tx_status.control, control, sizeof(*control));
+	memcpy(&priv->tx_buffers[entry].tx_status.common, &control->common,
+	       sizeof(control->common));
 	memcpy(&priv->tx_buffers[entry].hdr, hdr, sizeof(*hdr));
 	priv->tx_ring[entry].buffer1 = cpu_to_le32(mapping);

@@ -1862,7 +1863,7 @@ static int adm8211_tx(struct net_device 
 	if (short_preamble)
 		txhdr->header_control |= cpu_to_le16(ADM8211_TXHDRCTL_SHORT_PREAMBLE);
 
-	if (control->flags & IEEE80211_TXCTL_USE_RTS_CTS)
+	if (control->common.flags & IEEE80211_TXCTL_USE_RTS_CTS)
 		txhdr->header_control |= cpu_to_le16(ADM8211_TXHDRCTL_ENABLE_RTS);
 
 	if (fc & IEEE80211_FCTL_PROTECTED)

include/net/d80211.h +16 ~~-12~~

@@ -134,13 +134,7 @@ struct ieee80211_low_level_stats {
  * the hardware to use given values (depending on what is supported). */
 #define HW_KEY_IDX_INVALID -1
 
-struct ieee80211_tx_control {
-	enum { PKT_NORMAL = 0, PKT_PROBE_RESP } pkt_type;
-	int tx_rate; /* Transmit rate, given as the hw specific value for the
-		      * rate (from struct ieee80211_rate) */
-	int rts_cts_rate; /* Transmit rate for RTS/CTS frame, given as the hw
-			   * specific value for the rate (from
-			   * struct ieee80211_rate) */
+struct ieee80211_tx_ctrl_common {
 
 #define IEEE80211_TXCTL_REQ_TX_STATUS	(1<<0)/* request TX status callback for
 						* this frame */

@@ -161,6 +155,20 @@ struct ieee80211_tx_control {
 						* the frame */
 	u16 flags;			       /* tx control flags defined
 						* above */
+	enum { PKT_NORMAL = 0, PKT_PROBE_RESP } pkt_type;
+	u8 queue;		/* hardware queue to use for this frame;
+				 * 0 = highest, hw->queues-1 = lowest */
+	int type;	/* internal */
+	int ifindex;	/* internal */
+};
+
+struct ieee80211_tx_control {
+	struct ieee80211_tx_ctrl_common common;
+	int tx_rate; /* Transmit rate, given as the hw specific value for the
+		      * rate (from struct ieee80211_rate) */
+	int rts_cts_rate; /* Transmit rate for RTS/CTS frame, given as the hw
+			   * specific value for the rate (from
+			   * struct ieee80211_rate) */
 	u16 rts_cts_duration;	/* duration field for RTS/CTS frame */
 	u8 retry_limit;		/* 1 = only first attempt, 2 = one retry, .. */
 	u8 power_level;		/* per-packet transmit power level, in dBm */

@@ -169,8 +177,6 @@ struct ieee80211_tx_control {
 				 * hw->set_key() */
 	u8 icv_len;		/* length of the ICV/MIC field in octets */
 	u8 iv_len;		/* length of the IV field in octets */
-	u8 queue;		/* hardware queue to use for this frame;
-				 * 0 = highest, hw->queues-1 = lowest */
 	u8 sw_retry_attempt;	/* number of times hw has tried to
 				 * transmit frame (not incl. hw retries) */

@@ -180,8 +186,6 @@ struct ieee80211_tx_control {
 			     * struct ieee80211_rate). To be used to limit
 			     * packet dropping when probing higher rates, if hw
 			     * supports multiple retry rates. -1 = not used */
-	int type;	/* internal */
-	int ifindex;	/* internal */
 };
 
 #define RX_FLAG_MMIC_ERROR       0x1

@@ -207,7 +211,7 @@ struct ieee80211_rx_status {
  * frame. */
 struct ieee80211_tx_status {
 	/* copied ieee80211_tx_control structure */
-	struct ieee80211_tx_control control;
+	struct ieee80211_tx_ctrl_common common;
 
 #define IEEE80211_TX_STATUS_TX_FILTERED	(1<<0)
 #define IEEE80211_TX_STATUS_ACK		(1<<1) /* whether the TX frame was ACKed */

net/d80211/ieee80211.c +49 ~~-49~~

262 lines not on this page · raw message

net/d80211/ieee80211_scan.c +2 -2

@@ -329,12 +329,12 @@ void ieee80211_init_scan(struct net_devi
 
 	memset(&local->scan.tx_control, 0, sizeof(local->scan.tx_control));
 	local->scan.tx_control.key_idx = HW_KEY_IDX_INVALID;
-	local->scan.tx_control.flags |= IEEE80211_TXCTL_DO_NOT_ENCRYPT;
+	local->scan.tx_control.common.flags |= IEEE80211_TXCTL_DO_NOT_ENCRYPT;
 	memset(&extra, 0, sizeof(extra));
 	extra.endidx = local->num_curr_rates;
 	local->scan.tx_control.tx_rate =
 		rate_control_get_rate(local, dev, local->scan.skb, &extra)->val;
-	local->scan.tx_control.flags |= IEEE80211_TXCTL_NO_ACK;
+	local->scan.tx_control.common.flags |= IEEE80211_TXCTL_NO_ACK;
 }

net/d80211/ieee80211_sta.c +2 -2

@@ -2050,7 +2050,7 @@ static int ieee80211_sta_join_ibss(struc
 		}
 
 		memset(&control, 0, sizeof(control));
-		control.pkt_type = PKT_PROBE_RESP;
+		control.common.pkt_type = PKT_PROBE_RESP;
 		memset(&extra, 0, sizeof(extra));
 		extra.endidx = local->num_curr_rates;
 		rate = rate_control_get_rate(local, dev, skb, &extra);

@@ -2064,7 +2064,7 @@ static int ieee80211_sta_join_ibss(struc
 			rate->val2 : rate->val;
 		control.antenna_sel = local->conf.antenna_sel;
 		control.power_level = local->conf.power_level;
-		control.flags |= IEEE80211_TXCTL_NO_ACK;
+		control.common.flags |= IEEE80211_TXCTL_NO_ACK;
 		control.retry_limit = 1;
 		control.rts_cts_duration = 0;

Signature · 2 lines

Jiri Benc
SUSE Labs

Hong Liu

author

2006-10-24 08:27 UTC #5 lore raw

In reply to Jiri Benc

Quoted 12 lines · Jiri Benc

Quoted 8 lines · Johannes Berg
Quoted 4 lines · Jiri Benc
I don't like extending ieee80211_tx_control by 16 more bytes. The driver is required to store a copy of each ieee80211_tx_control (because it's copied to ieee80211_tx_status). I don't have a better idea, though. Anybody?
A pointer that goes somewhere else? I suppose it could even be in the skb's cb field.
Yes, that could work. Thanks!

Quoted 8 lines

Quoted 3 lines
Also, something I just came to think of, bcm43xx does phase2 mixing in hw and requires phase1 in software. Do we handle that with or without this patch? If not, it'd be nice to fix it up together.
It's not supported now. And it's really a good idea to extend the patch to support the bcm43xx case as well.
Jiri

It's really strange bcm43xx only does phase2 mixing in hw. With all the code that does phase2 mixing, doing phase1 mixing is very cheap.

With Jiri's ieee80211_tx_ctrl_common patch, I think we can put the rc4key or phase1 key in ieee80211_tx_control with each TX packet.

Thanks,
Hong

Johannes Berg

maintainer

2006-10-24 08:34 UTC #6 lore raw

Quoted 2 lines · Hong Liu

It's really strange bcm43xx only does phase2 mixing in hw. With all the code that does phase2 mixing, doing phase1 mixing is very cheap.

Yeah, well, they have the tkip sbox in hw.

Quoted 2 lines

With Jiri's ieee80211_tx_ctrl_common patch, I think we can put the rc4key or phase1 key in ieee80211_tx_control with each TX packet.

No, the phase1 key is not dependent on the packet so it should not be there. It should be set via the regular key setting mechanism, instead of the TKIP key the driver would be given the phase1 key if required.

johannes

Hong Liu

author

2006-10-24 08:45 UTC #7 lore raw

Quoted 7 lines · Johannes Berg

Quoted 2 lines · Hong Liu
It's really strange bcm43xx only does phase2 mixing in hw. With all the code that does phase2 mixing, doing phase1 mixing is very cheap.
Yeah, well, they have the tkip sbox in hw.

Quoted 8 lines

Quoted 2 lines
With Jiri's ieee80211_tx_ctrl_common patch, I think we can put the rc4key or phase1 key in ieee80211_tx_control with each TX packet.
No, the phase1 key is not dependent on the packet so it should not be there. It should be set via the regular key setting mechanism, instead of the TKIP key the driver would be given the phase1 key if required.
johannes

The first time when we set the TKIP key, we can set the phase1 key if the driver requires.
The problem is when IV16 wraps, who will generate the new phase1 key? If the stack need to do this, then we will need to call set_key in the packet TX path which I think may not be appropriate.

Thanks,
Hong

Johannes Berg

maintainer

2006-10-24 09:09 UTC #8 lore raw

Quoted 2 lines · Hong Liu

The first time when we set the TKIP key, we can set the phase1 key if the driver requires.

Right.

Quoted 1 line

The problem is when IV16 wraps, who will generate the new phase1 key?

Ok, I was confused for a moment, but yes, when the 16-bit part of the IV wraps then the upper part changes and hence you need to regenerate the phase1 key.

Quoted 2 lines

If the stack need to do this, then we will need to call set_key in the packet TX path which I think may not be appropriate.

Well, I checked again and found that bcm34xx requires the IV (48) to be maintained by software, iow with each packet you tell the hardware what the IV16 should be. Hence you know when it has wrapped and thus (before submitting the DMA for the packet) you can regenerate the phase1 key in software and upload it to the hardware key memory.

This is getting quite complicated. How about having the stack maintain the IV *only*, and adding helper functions for
* generating the phase 1 key
* generating the phase 2 key based on the phase 1 key
that the drivers can use as required?

ipw3945 would call both key generation functions as required, and bcm43xx would just call the phase 1 function when a key is uploaded and when the iv16 wraps, not using the phase 2 function at all.

johannes

Johannes Berg

maintainer

2006-10-24 09:11 UTC #9 lore raw

Quoted 2 lines · Johannes Berg

How about having the stack maintain
the IV *only*,

The idea here is that even if not required by hw that does both, it's cheap to maintain this counter. Though for all I care the driver could do it too.

johannes

Hong Liu

author

2006-10-25 08:35 UTC #10 lore raw

In reply to Johannes Berg

Quoted 28 lines · Johannes Berg

Quoted 2 lines · Hong Liu
The first time when we set the TKIP key, we can set the phase1 key if the driver requires.
Right.
Quoted 1 line
The problem is when IV16 wraps, who will generate the new phase1 key?
Ok, I was confused for a moment, but yes, when the 16-bit part of the IV wraps then the upper part changes and hence you need to regenerate the phase1 key.
Quoted 2 lines
If the stack need to do this, then we will need to call set_key in the packet TX path which I think may not be appropriate.
Well, I checked again and found that bcm34xx requires the IV (48) to be maintained by software, iow with each packet you tell the hardware what the IV16 should be. Hence you know when it has wrapped and thus (before submitting the DMA for the packet) you can regenerate the phase1 key in software and upload it to the hardware key memory.
This is getting quite complicated. How about having the stack maintain the IV *only*, and adding helper functions for
* generating the phase 1 key
* generating the phase 2 key based on the phase 1 key
that the drivers can use as required?

Quoted 7 lines

ipw3945 would call both key generation functions as required, and bcm43xx would just call the phase 1 function when a key is uploaded and when the iv16 wraps, not using the phase 2 function at all.
johannes

I'd prefer to let the stack tell the driver when there is new phase1 key generated.
Rebase the patch on David Kimdon's bitfield removing patches.

Signed-off-by Hong Liu <[email protected]>

include/net/d80211.h +12 -0

index 812f2d1..2570072 100644
@@ -159,6 +159,7 @@ #define IEEE80211_TXCTL_CLEAR_DST_MASK	(
 #define IEEE80211_TXCTL_REQUEUE		(1<<7)
 #define IEEE80211_TXCTL_FIRST_FRAGMENT	(1<<8) /* this is a first fragment of
 						* the frame */
+#define IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY (1<<9)
 	u16 flags;			       /* tx control flags defined
 						* above */
 	u16 rts_cts_duration;	/* duration field for RTS/CTS frame */

@@ -169,6 +170,8 @@ #define IEEE80211_TXCTL_FIRST_FRAGMENT	(
 				 * hw->set_key() */
 	u8 icv_len;		/* length of the ICV/MIC field in octets */
 	u8 iv_len;		/* length of the IV field in octets */
+	u8 tkip_keylen;
+	u8 tkip_key[16];/* generated RC4/phase1 key for hw TKIP */
 	u8 queue;		/* hardware queue to use for this frame;
 				 * 0 = highest, hw->queues-1 = lowest */
 	u8 sw_retry_attempt;	/* number of times hw has tried to

@@ -487,6 +490,15 @@ #define IEEE80211_HW_MONITOR_DURING_OPER
 	 * i.e. more than one skb per frame */
 #define IEEE80211_HW_FRAGLIST (1<<11)
 
+	/* calculate Michael MIC for an MSDU when doing hwcrypto */
+#define IEEE80211_HW_TKIP_INCLUDE_MMIC (1<<12)
+	/* Do TKIP phase1 key mixing in stack to support cards only do
+	 * phase2 key mixing when doing hwcrypto */
+#define IEEE80211_HW_TKIP_REQ_PHASE1_KEY (1<<13)
+	/* Do TKIP phase1 and phase2 key mixing in stack and send the generated
+	 * per-packet RC4 key with each TX frame when doing hwcrypto */
+#define IEEE80211_HW_TKIP_REQ_PHASE2_KEY (1<<14)
+
 	u32 flags;			/* hardware flags defined above */
 
         /* This is the time in us to change channels

net/d80211/tkip.c +20 -9

index 7e3665a..fd02449 100644
@@ -190,17 +190,16 @@ u8 * ieee80211_tkip_add_iv(u8 *pos, stru
 	return pos;
 }
 
-
-/* Encrypt packet payload with TKIP using @key. @pos is a pointer to the
- * beginning of the buffer containing payload. This payload must include
- * headroom of eight octets for IV and Ext. IV and taildroom of four octets
- * for ICV. @payload_len is the length of payload (_not_ including extra
- * headroom and tailroom). @ta is the transmitter addresses. */
-void ieee80211_tkip_encrypt_data(struct crypto_tfm *tfm, struct ieee80211_key *key,
-				 u8 *pos, size_t payload_len, u8 *ta)
+void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
+				  u16 *phase1key)
 {
-	u8 rc4key[16];
+	tkip_mixing_phase1(ta, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
+			   key->u.tkip.iv32, phase1key);
+}
 
+void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key, u8 *ta,
+			       u8 *rc4key)
+{
 	/* Calculate per-packet key */
 	if (key->u.tkip.iv16 == 0 || !key->u.tkip.tx_initialized) {
 		/* IV16 wrapped around - perform TKIP phase 1 */

@@ -211,7 +210,19 @@ void ieee80211_tkip_encrypt_data(struct 
 
 	tkip_mixing_phase2(key->u.tkip.p1k, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
 			   key->u.tkip.iv16, rc4key);
+}
+
+/* Encrypt packet payload with TKIP using @key. @pos is a pointer to the
+ * beginning of the buffer containing payload. This payload must include
+ * headroom of eight octets for IV and Ext. IV and taildroom of four octets
+ * for ICV. @payload_len is the length of payload (_not_ including extra
+ * headroom and tailroom). @ta is the transmitter addresses. */
+void ieee80211_tkip_encrypt_data(struct crypto_tfm *tfm, struct ieee80211_key *key,
+				 u8 *pos, size_t payload_len, u8 *ta)
+{
+	u8 rc4key[16];
 
+	ieee80211_tkip_gen_rc4key(key, ta, rc4key);
 	pos = ieee80211_tkip_add_iv(pos, key, rc4key[0], rc4key[1], rc4key[2]);
 	ieee80211_wep_encrypt_data(tfm, rc4key, 16, pos, payload_len);
 }

net/d80211/tkip.h +4 -0

index e36b85c..9b22717 100644
@@ -15,6 +15,10 @@ #include "ieee80211_key.h"
 
 u8 * ieee80211_tkip_add_iv(u8 *pos, struct ieee80211_key *key,
 			   u8 iv0, u8 iv1, u8 iv2);
+void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
+				  u16 *phase1key);
+void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key, u8 *ta,
+			       u8 *rc4key);
 void ieee80211_tkip_encrypt_data(struct crypto_tfm *tfm, struct ieee80211_key *key,
 				 u8 *pos, size_t payload_len, u8 *ta);
 enum {

net/d80211/wpa.c +24 -2

index e6ea53e..bb325bf 100644
@@ -105,7 +105,9 @@ #endif /* CONFIG_HOSTAPD_WPA_TESTING */
 
 	if (!tx->key->force_sw_encrypt &&
 	    !(tx->local->conf.flags & IEEE80211_CONF_SW_DECRYPT) &&
-	    !tx->fragmented && !wpa_test) {
+	    !tx->fragmented &&
+	    !(tx->local->hw->flags & IEEE80211_HW_TKIP_INCLUDE_MMIC) &&
+	    !wpa_test) {
 		/* hwaccel - with no need for preallocated room for Michael MIC
 		 */
 		return TXRX_CONTINUE;

@@ -332,14 +334,34 @@ #ifdef CONFIG_HOSTAPD_WPA_TESTING
 	    && !tx->wpa_test
 #endif /* CONFIG_HOSTAPD_WPA_TESTING */
 		) {
-		/* hwaccel - with preallocated room for IV */
+		u32 flags = tx->local->hw->flags;
+		hdr = (struct ieee80211_hdr *)skb->data;
 
+		/* hwaccel - with preallocated room for IV */
 		ieee80211_tkip_add_iv(pos, key,
 				      (u8) (key->u.tkip.iv16 >> 8),
 				      (u8) (((key->u.tkip.iv16 >> 8) | 0x20) &
 					    0x7f),
 				      (u8) key->u.tkip.iv16);
 
+		if (flags & IEEE80211_HW_TKIP_REQ_PHASE1_KEY) {
+			if (key->u.tkip.iv16 == 0 ||
+			    !key->u.tkip.tx_initialized) {
+				ieee80211_tkip_gen_phase1key(key, hdr->addr2,
+					    (u16 *)tx->u.tx.control->tkip_key);
+				key->u.tkip.tx_initialized = 1;
+				tx->u.tx.control->flags |=
+					    IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY;
+				tx->u.tx.control->tkip_keylen = 10;
+			} else
+				tx->u.tx.control->flags &=
+					    ~IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY;
+		} else if (flags & IEEE80211_HW_TKIP_REQ_PHASE2_KEY) {
+			ieee80211_tkip_gen_rc4key(key, hdr->addr2,
+						  tx->u.tx.control->tkip_key);
+			tx->u.tx.control->tkip_keylen = 16;
+		}
+
 		tx->u.tx.control->key_idx = tx->key->hw_key_idx;
 		return 0;
 	}

Johannes Berg

maintainer

2006-10-25 08:49 UTC #11 lore raw

Quoted 2 lines · Hong Liu

I'd prefer to let the stack tell the driver when there is new phase1 key generated.

Fine too, I guess.

include/net/d80211.h #define IEEE80211_TXCTL_FIRST_FRAGMENT ( · line 173

Quoted 1 line

include/net/d80211.h +1 -0
+	u8 tkip_keylen;

What do you need that for? The driver should know whether it requested a phase 1 or phase 2 key.

include/net/d80211.h #define IEEE80211_TXCTL_FIRST_FRAGMENT ( · line 174

Quoted 1 line

include/net/d80211.h +1 -0

+	u8 tkip_key[16];/* generated RC4/phase1 key for hw TKIP */

Do we really have to stick this into this structure? But I'll let Jiri figure out how to remove the structure bloat :)

include/net/d80211.h #define IEEE80211_HW_MONITOR_DURING_OPER · line 500

Quoted 8 lines

include/net/d80211.h +8 -0

+	/* calculate Michael MIC for an MSDU when doing hwcrypto */
+#define IEEE80211_HW_TKIP_INCLUDE_MMIC (1<<12)
+	/* Do TKIP phase1 key mixing in stack to support cards only do
+	 * phase2 key mixing when doing hwcrypto */
+#define IEEE80211_HW_TKIP_REQ_PHASE1_KEY (1<<13)
+	/* Do TKIP phase1 and phase2 key mixing in stack and send the generated
+	 * per-packet RC4 key with each TX frame when doing hwcrypto */
+#define IEEE80211_HW_TKIP_REQ_PHASE2_KEY (1<<14)

Maybe a comment indicating that you must not set both of these flags would be good. Or (see below)

Should there be some flag indicating if the hw/firmware checked the MIC on reception? The current code has bad assumptions there:

(from the pre-flags version)

/* Some devices handle Michael MIC internally and do not include MIC in
* the received packets passed up. device_strips_mic must be set
* for such devices. The 'encryption' frame control bit is expected to
* be still set in the IEEE 802.11 header with this option unlike with
* the device_hides_wep configuration option.
*/
unsigned int device_strips_mic:1;

What if the devices leaves the MIC there but indicates if it was checked?

Quoted 1 line

net/d80211/wpa.c +1 -0

+		if (flags & IEEE80211_HW_TKIP_REQ_PHASE1_KEY) {

...

net/d80211/wpa.c #ifdef CONFIG_HOSTAPD_WPA_TESTING · line 359

Quoted 1 line

net/d80211/wpa.c +1 -0

+		} else if (flags & IEEE80211_HW_TKIP_REQ_PHASE2_KEY) {

...

if you change the order of these tests then setting both flags will be fine.

johannes

Hong Liu

author

2006-11-14 02:31 UTC #12 lore raw

Resend the patch according to Johannes's comments. Still put he tkip_key in tx_control structure.

Signed-off-by Hong Liu <[email protected]>

include/net/d80211.h +11 -0

index 812f2d1..cf87adc 100644
@@ -159,6 +159,7 @@ #define IEEE80211_TXCTL_CLEAR_DST_MASK	(
 #define IEEE80211_TXCTL_REQUEUE		(1<<7)
 #define IEEE80211_TXCTL_FIRST_FRAGMENT	(1<<8) /* this is a first fragment of
 						* the frame */
+#define IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY (1<<9)
 	u32 flags;			       /* tx control flags defined
 						* above */
 	u16 rts_cts_duration;	/* duration field for RTS/CTS frame */

@@ -169,6 +170,7 @@ #define IEEE80211_TXCTL_FIRST_FRAGMENT	(
 				 * hw->set_key() */
 	u8 icv_len;		/* length of the ICV/MIC field in octets */
 	u8 iv_len;		/* length of the IV field in octets */
+	u8 tkip_key[16];	/* generated phase2/phase1 key for hw TKIP */
 	u8 queue;		/* hardware queue to use for this frame;
 				 * 0 = highest, hw->queues-1 = lowest */
 	u8 sw_retry_attempt;	/* number of times hw has tried to

@@ -487,6 +489,15 @@ #define IEEE80211_HW_MONITOR_DURING_OPER
 	 * i.e. more than one skb per frame */
 #define IEEE80211_HW_FRAGLIST (1<<11)
 
+	/* calculate Michael MIC for an MSDU when doing hwcrypto */
+#define IEEE80211_HW_TKIP_INCLUDE_MMIC (1<<12)
+	/* Do TKIP phase1 key mixing in stack to support cards only do
+	 * phase2 key mixing when doing hwcrypto */
+#define IEEE80211_HW_TKIP_REQ_PHASE1_KEY (1<<13)
+	/* Do TKIP phase1 and phase2 key mixing in stack and send the generated
+	 * per-packet RC4 key with each TX frame when doing hwcrypto */
+#define IEEE80211_HW_TKIP_REQ_PHASE2_KEY (1<<14)
+
 	u32 flags;			/* hardware flags defined above */
 
         /* This is the time in us to change channels

net/d80211/tkip.c +20 -9

index 7e3665a..fd02449 100644
@@ -190,17 +190,16 @@ u8 * ieee80211_tkip_add_iv(u8 *pos, stru
 	return pos;
 }
 
-
-/* Encrypt packet payload with TKIP using @key. @pos is a pointer to the
- * beginning of the buffer containing payload. This payload must include
- * headroom of eight octets for IV and Ext. IV and taildroom of four octets
- * for ICV. @payload_len is the length of payload (_not_ including extra
- * headroom and tailroom). @ta is the transmitter addresses. */
-void ieee80211_tkip_encrypt_data(struct crypto_tfm *tfm, struct ieee80211_key *key,
-				 u8 *pos, size_t payload_len, u8 *ta)
+void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
+				  u16 *phase1key)
 {
-	u8 rc4key[16];
+	tkip_mixing_phase1(ta, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
+			   key->u.tkip.iv32, phase1key);
+}
 
+void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key, u8 *ta,
+			       u8 *rc4key)
+{
 	/* Calculate per-packet key */
 	if (key->u.tkip.iv16 == 0 || !key->u.tkip.tx_initialized) {
 		/* IV16 wrapped around - perform TKIP phase 1 */

@@ -211,7 +210,19 @@ void ieee80211_tkip_encrypt_data(struct 
 
 	tkip_mixing_phase2(key->u.tkip.p1k, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
 			   key->u.tkip.iv16, rc4key);
+}
+
+/* Encrypt packet payload with TKIP using @key. @pos is a pointer to the
+ * beginning of the buffer containing payload. This payload must include
+ * headroom of eight octets for IV and Ext. IV and taildroom of four octets
+ * for ICV. @payload_len is the length of payload (_not_ including extra
+ * headroom and tailroom). @ta is the transmitter addresses. */
+void ieee80211_tkip_encrypt_data(struct crypto_tfm *tfm, struct ieee80211_key *key,
+				 u8 *pos, size_t payload_len, u8 *ta)
+{
+	u8 rc4key[16];
 
+	ieee80211_tkip_gen_rc4key(key, ta, rc4key);
 	pos = ieee80211_tkip_add_iv(pos, key, rc4key[0], rc4key[1], rc4key[2]);
 	ieee80211_wep_encrypt_data(tfm, rc4key, 16, pos, payload_len);
 }

net/d80211/tkip.h +4 -0

index e36b85c..9b22717 100644
@@ -15,6 +15,10 @@ #include "ieee80211_key.h"
 
 u8 * ieee80211_tkip_add_iv(u8 *pos, struct ieee80211_key *key,
 			   u8 iv0, u8 iv1, u8 iv2);
+void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
+				  u16 *phase1key);
+void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key, u8 *ta,
+			       u8 *rc4key);
 void ieee80211_tkip_encrypt_data(struct crypto_tfm *tfm, struct ieee80211_key *key,
 				 u8 *pos, size_t payload_len, u8 *ta);
 enum {

net/d80211/wpa.c +22 -2

index e6ea53e..7484575 100644
@@ -105,7 +105,9 @@ #endif /* CONFIG_HOSTAPD_WPA_TESTING */
 
 	if (!tx->key->force_sw_encrypt &&
 	    !(tx->local->conf.flags & IEEE80211_CONF_SW_DECRYPT) &&
-	    !tx->fragmented && !wpa_test) {
+	    !tx->fragmented &&
+	    !(tx->local->hw->flags & IEEE80211_HW_TKIP_INCLUDE_MMIC) &&
+	    !wpa_test) {
 		/* hwaccel - with no need for preallocated room for Michael MIC
 		 */
 		return TXRX_CONTINUE;

@@ -332,14 +334,32 @@ #ifdef CONFIG_HOSTAPD_WPA_TESTING
 	    && !tx->wpa_test
 #endif /* CONFIG_HOSTAPD_WPA_TESTING */
 		) {
-		/* hwaccel - with preallocated room for IV */
+		u32 flags = tx->local->hw->flags;
+		hdr = (struct ieee80211_hdr *)skb->data;
 
+		/* hwaccel - with preallocated room for IV */
 		ieee80211_tkip_add_iv(pos, key,
 				      (u8) (key->u.tkip.iv16 >> 8),
 				      (u8) (((key->u.tkip.iv16 >> 8) | 0x20) &
 					    0x7f),
 				      (u8) key->u.tkip.iv16);
 
+		if (flags & IEEE80211_HW_TKIP_REQ_PHASE2_KEY)
+			ieee80211_tkip_gen_rc4key(key, hdr->addr2,
+						  tx->u.tx.control->tkip_key);
+		else if (flags & IEEE80211_HW_TKIP_REQ_PHASE1_KEY) {
+			if (key->u.tkip.iv16 == 0 ||
+			    !key->u.tkip.tx_initialized) {
+				ieee80211_tkip_gen_phase1key(key, hdr->addr2,
+					    (u16 *)tx->u.tx.control->tkip_key);
+				key->u.tkip.tx_initialized = 1;
+				tx->u.tx.control->flags |=
+					    IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY;
+			} else
+				tx->u.tx.control->flags &=
+					    ~IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY;
+		}
+
 		tx->u.tx.control->key_idx = tx->key->hw_key_idx;
 		return 0;
 	}

Quoted 55 lines · Johannes Berg

Quoted 2 lines · Hong Liu
I'd prefer to let the stack tell the driver when there is new phase1 key generated.
Fine too, I guess.
Quoted 1 line
+ u8 tkip_keylen;
What do you need that for? The driver should know whether it requested a phase 1 or phase 2 key.
Quoted 1 line
+ u8 tkip_key[16];/* generated RC4/phase1 key for hw TKIP */
Do we really have to stick this into this structure? But I'll let Jiri figure out how to remove the structure bloat :)
Review
Quoted 8 lines
(quoted diff) +8 -0
+     /* calculate Michael MIC for an MSDU when doing hwcrypto */
+#define IEEE80211_HW_TKIP_INCLUDE_MMIC (1<<12)
+     /* Do TKIP phase1 key mixing in stack to support cards only do
+      * phase2 key mixing when doing hwcrypto */
+#define IEEE80211_HW_TKIP_REQ_PHASE1_KEY (1<<13)
+     /* Do TKIP phase1 and phase2 key mixing in stack and send the generated
+      * per-packet RC4 key with each TX frame when doing hwcrypto */
+#define IEEE80211_HW_TKIP_REQ_PHASE2_KEY (1<<14)
Maybe a comment indicating that you must not set both of these flags would be good. Or (see below)
Should there be some flag indicating if the hw/firmware checked the MIC on reception? The current code has bad assumptions there:
(from the pre-flags version)
/* Some devices handle Michael MIC internally and do not include MIC in
* the received packets passed up. device_strips_mic must be set
* for such devices. The 'encryption' frame control bit is expected to
* be still set in the IEEE 802.11 header with this option unlike with
* the device_hides_wep configuration option.
*/
unsigned int device_strips_mic:1;
What if the devices leaves the MIC there but indicates if it was checked?
Quoted 1 line
+ if (flags & IEEE80211_HW_TKIP_REQ_PHASE1_KEY) {
...
Quoted 1 line
+ } else if (flags & IEEE80211_HW_TKIP_REQ_PHASE2_KEY) {
...
if you change the order of these tests then setting both flags will be fine.
johannes

Johannes Berg

maintainer

2006-11-15 16:24 UTC #13 lore raw

Quoted 1 line · Hong Liu

Resend the patch according to Johannes's comments.

Thanks :)

Quoted 1 line

Still put he tkip_key in tx_control structure.

Having the tkip_key in there is actually pretty bad for when/if we want to push the tx_control into skb->cb (possibly combined with some header or whatever). I don't think that has been decided on yet, will be a large change anyway and this helps a lot so :)

Instead of putting all this into the stack, however, I think we could make those drivers that require it do the bookkeeping and export ieee80211_tkip_gen_phase1key and ieee80211_tkip_gen_rc4key for their use. Maybe that's too much against Jiri's doctrine of "be easy on drivers" though :)

This might actually also go some way towards unification with ipw2x00's ieee80211 because those drivers require such functions as well as far as I know. Haven't looked too deeply though.

johannes

Johannes Berg

maintainer

2006-11-16 09:51 UTC #14 lore raw

Quoted 5 lines · Johannes Berg

Instead of putting all this into the stack, however, I think we could make those drivers that require it do the bookkeeping and export ieee80211_tkip_gen_phase1key and ieee80211_tkip_gen_rc4key for their use. Maybe that's too much against Jiri's doctrine of "be easy on drivers" though :)

Thought about this a bit more.

I think we should simplify the stack code by making it have *only* the distinction between between hardware and software crypto (on a per-key basis).

This would entail getting rid of the flags

IEEE80211_HW_TKIP_INCLUDE_MMIC
IEEE80211_HW_TKIP_REQ_PHASE1_KEY
IEEE80211_HW_TKIP_REQ_PHASE2_KEY

and possibly

IEEE80211_HW_NO_TKIP_WMM_HWACCEL

Instead, I would like to provide "library" functions to do these tasks and let drivers use them when they are necessary.

This should simplify and streamline code in the stack by getting rid of all the tests for these flags and associated code.

My reasoning for this is that
1) it doesn't break Jiri's doctrine of making things easy for drivers because, well, simple drivers just use sw crypto
2) since drivers will always hard-code these flags it actually makes the code more efficient -- no testing of flags required, just function calls we'd be making elsewhere after testing flags
3) instead of drivers hard-coding the flag setting they hardcode (yes, in a few more lines) the crypto they need

I think putting the code into the driver vs. the stack is a trade-off we should make because it makes things more efficient.

Could you live with a patch to do this and adjust ipw3945 accordingly?

For ipw3945 it would be just an addition of a call to ieee80211_tkip_gen_rc4key() as it lives in the stack after this patch, and for bcm43xx it would be similar to the code you added within "else if (flags & IEEE80211_HW_TKIP_REQ_PHASE1_KEY) {"

As for IEEE80211_HW_NO_TKIP_WMM_HWACCEL, we would have to tell the hw->set_key routine if WME is enabled for the STA associated with the key. I'm not yet sure how to do this best, plus the flag isn't actually checked in the TX path but only the key setting path so...

Basically, I think we have too much knowledge of hardware quirks in the stack. I'll be submitting a patch for this item later today.

johannes

Jouni Malinen

2006-11-16 17:27 UTC #15 lore raw

Quoted 3 lines · Johannes Berg

I think we should simplify the stack code by making it have *only* the distinction between between hardware and software crypto (on a per-key basis).

Hmm.. I think I need to see the patch for doing this before being able to decide whether I'd like this or not.. Michael MIC is a bit unfortunate design since it happens at MSDU, not MPDU level. Because of this, most hardware designs have to do something special when fragmentation is used. If the fragmentation is done in the stack, it may be difficult to move the Michael MIC operations into the driver since they need to happen before fragmentation (which would be done in the stack in this case)..

Other than this special case, it sounds reasonable to keep most of the small hardware design changes hidden in the driver code as long as the functions that are exported from the stack are the same ones that the stack would be using anyway (i.e., not having to maintain two versions of the same code).

Signature · 1 line

Jouni Malinen                                            PGP id EFC895FA

Johannes Berg

maintainer

2006-11-16 17:40 UTC #16 lore raw

Quoted 8 lines · Jouni Malinen

Hmm.. I think I need to see the patch for doing this before being able to decide whether I'd like this or not.. Michael MIC is a bit unfortunate design since it happens at MSDU, not MPDU level. Because of this, most hardware designs have to do something special when fragmentation is used. If the fragmentation is done in the stack, it may be difficult to move the Michael MIC operations into the driver since they need to happen before fragmentation (which would be done in the stack in this case)..

Oh, good point. Will need to think about this more. But right now, if that MMIC flag is set (i.e. hw does MMIC), we still do MMIC for fragmented frames. And there's actually a bug here I think: if hw does MMIC, how can the driver tell not to do it on those frames it's getting as fragments?

Quoted 5 lines

Other than this special case, it sounds reasonable to keep most of the small hardware design changes hidden in the driver code as long as the functions that are exported from the stack are the same ones that the stack would be using anyway (i.e., not having to maintain two versions of the same code).

Of course, for this TKIP stuff I'd only export those functions the drivers will need and remove the flags, for software crypto (in stack) they'd still be used as-is.

johannes

Jouni Malinen

2006-11-16 17:46 UTC #17 lore raw

Quoted 5 lines · Johannes Berg

Oh, good point. Will need to think about this more. But right now, if that MMIC flag is set (i.e. hw does MMIC), we still do MMIC for fragmented frames. And there's actually a bug here I think: if hw does MMIC, how can the driver tell not to do it on those frames it's getting as fragments?

It's been awhile since I working on this part, but if I remember correctly, this can be done easily by the driver behaving different based on the frame being fragmented or not (it is fragmented if MoreFrag flag is set or seq# != 0). That's the way it was done with hardware designs that allow full TKIP hwaccel for MSDUs that are not fragmented, but require software implementation of Michael MIC if MSDU is fragmented.

Signature · 1 line

Jouni Malinen                                            PGP id EFC895FA

Johannes Berg

maintainer

2006-11-16 17:51 UTC #18 lore raw

Quoted 7 lines · Jouni Malinen

It's been awhile since I working on this part, but if I remember correctly, this can be done easily by the driver behaving different based on the frame being fragmented or not (it is fragmented if MoreFrag flag is set or seq# != 0). That's the way it was done with hardware designs that allow full TKIP hwaccel for MSDUs that are not fragmented, but require software implementation of Michael MIC if MSDU is fragmented.

Oh right, I forgot that the driver can just look into the 802.11 header, was checking tx_control only :)

johannes