[PATCH RFC] d80211: Fix TKIP phase1 key mixing for hwcrypto

[PATCH RFC] d80211: Fix TKIP phase1 key mixing for hwcrypto

[Michael Buesch]

This patch is not runtime tested, as I did not implement tkip
support in bcm43xx, yet.

--

This fixes TKIP phase1 key mixing for hwcrypto on BigEndian
platforms.
Casting an u8 array to u16* is wrong and will only work
on le platforms.
Make it explicit and expect an u8* parameter for
ieee80211_tkip_gen_phase1key(). The function will take
care to return an u8 array, instead of an u16 array, as
that's what drivers assume.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

Index: bu3sch-wireless-dev/net/d80211/tkip.c
===================================================================
--- bu3sch-wireless-dev.orig/net/d80211/tkip.c	2007-01-11 19:09:43.000000000 +0100
+++ bu3sch-wireless-dev/net/d80211/tkip.c	2007-02-03 18:23:52.000000000 +0100
@@ -192,10 +192,15 @@ u8 * ieee80211_tkip_add_iv(u8 *pos, stru
 
 
 void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
-				  u16 *phase1key)
+				  u8 *phase1key)
 {
+	__le16 *k = (__le16 *)phase1key;
+	int i;
+
 	tkip_mixing_phase1(ta, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
-			   key->u.tkip.iv32, phase1key);
+			   key->u.tkip.iv32, (u16 *)k);
+	for (i = 0; i < 5; i++)
+		k[i] = cpu_to_le16(k[i]);
 }
 
 void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key, u8 *ta,
Index: bu3sch-wireless-dev/net/d80211/tkip.h
===================================================================
--- bu3sch-wireless-dev.orig/net/d80211/tkip.h	2007-01-11 19:09:43.000000000 +0100
+++ bu3sch-wireless-dev/net/d80211/tkip.h	2007-02-03 18:19:18.000000000 +0100
@@ -16,7 +16,7 @@
 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);
+				  u8 *phase1key);
 void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key, u8 *ta,
 			       u8 *rc4key);
 void ieee80211_tkip_encrypt_data(struct crypto_blkcipher *tfm,
Index: bu3sch-wireless-dev/net/d80211/wpa.c
===================================================================
--- bu3sch-wireless-dev.orig/net/d80211/wpa.c	2007-01-11 19:09:43.000000000 +0100
+++ bu3sch-wireless-dev/net/d80211/wpa.c	2007-02-03 18:18:50.000000000 +0100
@@ -349,7 +349,7 @@ skip_iv_inc:
 			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);
+					tx->u.tx.control->tkip_key);
 				key->u.tkip.tx_initialized = 1;
 				tx->u.tx.control->flags |=
 					    IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY;


-- 
Greetings Michael.

Re: [PATCH RFC] d80211: Fix TKIP phase1 key mixing for hwcrypto

[Jiri Benc]

On Sat, 3 Feb 2007 18:32:48 +0100, Michael Buesch wrote:
> @@ -192,10 +192,15 @@ u8 * ieee80211_tkip_add_iv(u8 *pos, stru
>  
>  
>  void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
> -				  u16 *phase1key)
> +				  u8 *phase1key)
>  {
> +	__le16 *k = (__le16 *)phase1key;
> +	int i;
> +
>  	tkip_mixing_phase1(ta, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
> -			   key->u.tkip.iv32, phase1key);
> +			   key->u.tkip.iv32, (u16 *)k);
> +	for (i = 0; i < 5; i++)
> +		k[i] = cpu_to_le16(k[i]);
>  }

Maybe a slightly better type checking but still looks ugly:

@@ -192,10 +192,16 @@ u8 * ieee80211_tkip_add_iv(u8 *pos, stru
 
 
 void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
-				  u16 *phase1key)
+				  u8 *phase1key)
 {
+	u16 *tmp_result = (u16 *)phase1key;
+	__le16 *k = (__le16 *)phase1key;
+	int i;
+
 	tkip_mixing_phase1(ta, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
-			   key->u.tkip.iv32, phase1key);
+			   key->u.tkip.iv32, tmp_result);
+	for (i = 0; i < 5; i++)
+		k[i] = cpu_to_le16(tmp_result[i]);
 }
 
 void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key, u8 *ta,


Moreover, I'm not sure if GCC is able to optimize out the for loop in
this case :-(

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH RFC] d80211: Fix TKIP phase1 key mixing for hwcrypto

[Michael Buesch]

On Monday 05 February 2007 22:19, Jiri Benc wrote:
> On Sat, 3 Feb 2007 18:32:48 +0100, Michael Buesch wrote:
> > @@ -192,10 +192,15 @@ u8 * ieee80211_tkip_add_iv(u8 *pos, stru
> >  
> >  
> >  void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
> > -				  u16 *phase1key)
> > +				  u8 *phase1key)
> >  {
> > +	__le16 *k = (__le16 *)phase1key;
> > +	int i;
> > +
> >  	tkip_mixing_phase1(ta, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
> > -			   key->u.tkip.iv32, phase1key);
> > +			   key->u.tkip.iv32, (u16 *)k);
> > +	for (i = 0; i < 5; i++)
> > +		k[i] = cpu_to_le16(k[i]);
> >  }
> 
> Maybe a slightly better type checking but still looks ugly:

Hm, well. I don't really see how typechecking is better in this case,
but if you like it more, I'm ok with it. ;)

> @@ -192,10 +192,16 @@ u8 * ieee80211_tkip_add_iv(u8 *pos, stru
>  
>  
>  void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
> -				  u16 *phase1key)
> +				  u8 *phase1key)
>  {
> +	u16 *tmp_result = (u16 *)phase1key;
> +	__le16 *k = (__le16 *)phase1key;
> +	int i;
> +
>  	tkip_mixing_phase1(ta, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
> -			   key->u.tkip.iv32, phase1key);
> +			   key->u.tkip.iv32, tmp_result);
> +	for (i = 0; i < 5; i++)
> +		k[i] = cpu_to_le16(tmp_result[i]);
>  }
>  
>  void ieee80211_tkip_gen_rc4key(struct ieee80211_key *key, u8 *ta,
> 
> 
> Moreover, I'm not sure if GCC is able to optimize out the for loop in
> this case :-(

Yeah, I was going to check this and was going to add #ifdefs if it doesn't.
But that was not my major concern at this point.
It was more that people agree to me that it _is_ broken on BE platforms.
(I cannot test it, yet, as tkip has other problems for bcm43xx).

-- 
Greetings Michael.

d80211: Fix TKIP key type

[Michael Wu]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

d80211: Fix TKIP key type

Avoid the messy typecasting and let drivers handle byteordering.

Signed-off-by: Michael Wu <flamingice@sourmilk.net>

diff --git a/include/net/d80211.h b/include/net/d80211.h
index 65a5d36..0bd6b15 100644
--- a/include/net/d80211.h
+++ b/include/net/d80211.h
@@ -202,7 +202,10 @@ 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 tkip_key[16];	/* generated phase2/phase1 key for hw TKIP */
+	union {
+		u16 phase1[5];
+		u8 phase2[16];
+	} tkip;			/* 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
diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
diff --git a/net/d80211/ieee80211_sta.c b/net/d80211/ieee80211_sta.c
diff --git a/net/d80211/wpa.c b/net/d80211/wpa.c
index 7b64abf..d5ef61a 100644
--- a/net/d80211/wpa.c
+++ b/net/d80211/wpa.c
@@ -344,12 +344,12 @@ skip_iv_inc:
 
 		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.phase2);
 		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);
+					    tx->u.tx.control->tkip.phase1);
 				key->u.tkip.tx_initialized = 1;
 				tx->u.tx.control->flags |=
 					    IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY;

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: d80211: Fix TKIP key type

[Michael Buesch]

On Tuesday 06 February 2007 16:33, Michael Wu wrote:
> d80211: Fix TKIP key type
> 
> Avoid the messy typecasting and let drivers handle byteordering.

Well, ok.

We need to remove all the tkip stuff from tx_control anyway,
as it's broken. It does not work to pass tkip keys along with
TX packets, as we also need the key for RX (on bcm43xx at least).
So I think we need a callback or something that sets the key in HW.
Maybe we can do that in the already existing set_key callback.
Dunno yet.

> Signed-off-by: Michael Wu <flamingice@sourmilk.net>
> 
> diff --git a/include/net/d80211.h b/include/net/d80211.h
> index 65a5d36..0bd6b15 100644
> --- a/include/net/d80211.h
> +++ b/include/net/d80211.h
> @@ -202,7 +202,10 @@ 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 tkip_key[16];	/* generated phase2/phase1 key for hw TKIP */
> +	union {
> +		u16 phase1[5];
> +		u8 phase2[16];
> +	} tkip;			/* 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
> diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
> diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
> diff --git a/net/d80211/ieee80211_sta.c b/net/d80211/ieee80211_sta.c
> diff --git a/net/d80211/wpa.c b/net/d80211/wpa.c
> index 7b64abf..d5ef61a 100644
> --- a/net/d80211/wpa.c
> +++ b/net/d80211/wpa.c
> @@ -344,12 +344,12 @@ skip_iv_inc:
>  
>  		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.phase2);
>  		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);
> +					    tx->u.tx.control->tkip.phase1);
>  				key->u.tkip.tx_initialized = 1;
>  				tx->u.tx.control->flags |=
>  					    IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY;
> 

-- 
Greetings Michael.

Re: d80211: Fix TKIP key type

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Tue, 2007-02-06 at 16:44 +0100, Michael Buesch wrote:

> So I think we need a callback or something that sets the key in HW.
> Maybe we can do that in the already existing set_key callback.

Yes, your original patch with the library functions should be fine, if
the driver needs it it can request it either in set_key or during tx (if
the TSC cycled or whatever)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: d80211: Fix TKIP key type

[Michael Buesch]

On Tuesday 06 February 2007 16:33, Michael Wu wrote:
> d80211: Fix TKIP key type
> 
> Avoid the messy typecasting and let drivers handle byteordering.
> 
> Signed-off-by: Michael Wu <flamingice@sourmilk.net>

Jiri, can you avoid pulling this patch, yet?
I am trying to get a better solution for the whole TKIP stuff
that actually works with bcm43xx. My patch will fix this issue then, too.

> diff --git a/include/net/d80211.h b/include/net/d80211.h
> index 65a5d36..0bd6b15 100644
> --- a/include/net/d80211.h
> +++ b/include/net/d80211.h
> @@ -202,7 +202,10 @@ 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 tkip_key[16];	/* generated phase2/phase1 key for hw TKIP */
> +	union {
> +		u16 phase1[5];
> +		u8 phase2[16];
> +	} tkip;			/* 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
> diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
> diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
> diff --git a/net/d80211/ieee80211_sta.c b/net/d80211/ieee80211_sta.c
> diff --git a/net/d80211/wpa.c b/net/d80211/wpa.c
> index 7b64abf..d5ef61a 100644
> --- a/net/d80211/wpa.c
> +++ b/net/d80211/wpa.c
> @@ -344,12 +344,12 @@ skip_iv_inc:
>  
>  		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.phase2);
>  		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);
> +					    tx->u.tx.control->tkip.phase1);
>  				key->u.tkip.tx_initialized = 1;
>  				tx->u.tx.control->flags |=
>  					    IEEE80211_TXCTL_TKIP_NEW_PHASE1_KEY;
> 

-- 
Greetings Michael.

Re: [PATCH RFC] d80211: Fix TKIP phase1 key mixing for hwcrypto

[Jiri Benc]

On Tue, 6 Feb 2007 16:02:45 +0100, Michael Buesch wrote:
> On Monday 05 February 2007 22:19, Jiri Benc wrote:
> > On Sat, 3 Feb 2007 18:32:48 +0100, Michael Buesch wrote:
> > > @@ -192,10 +192,15 @@ u8 * ieee80211_tkip_add_iv(u8 *pos, stru
> > >  
> > >  
> > >  void ieee80211_tkip_gen_phase1key(struct ieee80211_key *key, u8 *ta,
> > > -				  u16 *phase1key)
> > > +				  u8 *phase1key)
> > >  {
> > > +	__le16 *k = (__le16 *)phase1key;
> > > +	int i;
> > > +
> > >  	tkip_mixing_phase1(ta, &key->key[ALG_TKIP_TEMP_ENCR_KEY],
> > > -			   key->u.tkip.iv32, phase1key);
> > > +			   key->u.tkip.iv32, (u16 *)k);
> > > +	for (i = 0; i < 5; i++)
> > > +		k[i] = cpu_to_le16(k[i]);
> > >  }
> > 
> > Maybe a slightly better type checking but still looks ugly:
> 
> Hm, well. I don't really see how typechecking is better in this case,

cpu_to_le16 with a __le16 variable as a parameter always looks
suspicious. In the version I sent it is at least clear that it's
intended.

> but if you like it more, I'm ok with it. ;)

I dislike both of them but have no better idea.

> Yeah, I was going to check this and was going to add #ifdefs if it doesn't.
> But that was not my major concern at this point.
> It was more that people agree to me that it _is_ broken on BE platforms.

Looks like it is.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: d80211: Fix TKIP key type

[Jiri Benc]

On Thu, 8 Feb 2007 18:15:27 +0100, Michael Buesch wrote:
> On Tuesday 06 February 2007 16:33, Michael Wu wrote:
> > d80211: Fix TKIP key type
> > 
> > Avoid the messy typecasting and let drivers handle byteordering.
> > 
> > Signed-off-by: Michael Wu <flamingice@sourmilk.net>
> 
> Jiri, can you avoid pulling this patch, yet?
> I am trying to get a better solution for the whole TKIP stuff
> that actually works with bcm43xx. My patch will fix this issue then, too.

I wasn't going to apply it anyway as it doesn't seem to solve the
problem - it just shifts it to someone else.

 Jiri

-- 
Jiri Benc
SUSE Labs