[PATCH] mac80211 : fix a race with update_tkip_key

[PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

The mac80211 tkip code won't call update_tkip_key, if some rx packets
get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
first packet because the hardware key stuff is called asynchronously with
todo workqueue.

This patch workaround that by tracking if we send the key to hardware.

Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>

Index: linux-2.6/net/mac80211/tkip.c
===================================================================
--- linux-2.6.orig/net/mac80211/tkip.c	2009-06-30 20:27:02.000000000 +0000
+++ linux-2.6/net/mac80211/tkip.c	2009-08-21 22:02:34.000000000 +0000
@@ -100,7 +100,7 @@
 		p1k[3] += tkipS(p1k[2] ^ get_unaligned_le16(tk + 12 + j));
 		p1k[4] += tkipS(p1k[3] ^ get_unaligned_le16(tk + 0 + j)) + i;
 	}
-	ctx->initialized = 1;
+	ctx->initialized = TKIP_INITIALIZED_PHASE1;
 }

 static void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
@@ -183,7 +183,7 @@
 	/* Update the p1k only when the iv16 in the packet wraps around, this
 	 * might occur after the wrap around of iv16 in the key in case of
 	 * fragmented packets. */
-	if (iv16 == 0 || !ctx->initialized)
+	if (iv16 == 0 || ctx->initialized == TKIP_INITIALIZED_NONE)
 		tkip_mixing_phase1(tk, ctx, hdr->addr2, iv32);

 	if (type == IEEE80211_TKIP_P1_KEY) {
@@ -209,7 +209,7 @@
 	const u8 *tk = &key->conf.key[NL80211_TKIP_DATA_OFFSET_ENCR_KEY];

 	/* Calculate per-packet key */
-	if (ctx->iv16 == 0 || !ctx->initialized)
+	if (ctx->iv16 == 0 || ctx->initialized == TKIP_INITIALIZED_NONE)
 		tkip_mixing_phase1(tk, ctx, ta, ctx->iv32);

 	tkip_mixing_phase2(tk, ctx, ctx->iv16, rc4key);
@@ -259,7 +259,7 @@
 	if ((keyid >> 6) != key->conf.keyidx)
 		return TKIP_DECRYPT_INVALID_KEYIDX;

-	if (key->u.tkip.rx[queue].initialized &&
+	if (key->u.tkip.rx[queue].initialized != TKIP_INITIALIZED_NONE &&
 	    (iv32 < key->u.tkip.rx[queue].iv32 ||
 	     (iv32 == key->u.tkip.rx[queue].iv32 &&
 	      iv16 <= key->u.tkip.rx[queue].iv16))) {
@@ -275,11 +275,11 @@

 	if (only_iv) {
 		res = TKIP_DECRYPT_OK;
-		key->u.tkip.rx[queue].initialized = 1;
+		key->u.tkip.rx[queue].initialized = TKIP_INITIALIZED_UPDATE_KEY;
 		goto done;
 	}

-	if (!key->u.tkip.rx[queue].initialized ||
+	if (key->u.tkip.rx[queue].initialized == TKIP_INITIALIZED_NONE ||
 	    key->u.tkip.rx[queue].iv32 != iv32) {
 		/* IV16 wrapped around - perform TKIP phase 1 */
 		tkip_mixing_phase1(tk, &key->u.tkip.rx[queue], ta, iv32);
@@ -299,18 +299,20 @@
 			printk("\n");
 		}
 #endif
-		if (key->local->ops->update_tkip_key &&
-			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
-			u8 bcast[ETH_ALEN] =
-				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-			u8 *sta_addr = key->sta->sta.addr;
-
-			if (is_multicast_ether_addr(ra))
-				sta_addr = bcast;
-
-			drv_update_tkip_key(key->local, &key->conf, sta_addr,
-					    iv32, key->u.tkip.rx[queue].p1k);
-		}
+	}
+	if (key->local->ops->update_tkip_key &&
+	    key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
+	    key->u.tkip.rx[queue].initialized != TKIP_INITIALIZED_UPDATE_KEY) {
+		u8 bcast[ETH_ALEN] =
+			{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+		u8 *sta_addr = key->sta->sta.addr;
+
+		if (is_multicast_ether_addr(ra))
+			sta_addr = bcast;
+
+		drv_update_tkip_key(key->local, &key->conf, sta_addr,
+					iv32, key->u.tkip.rx[queue].p1k);
+		key->u.tkip.rx[queue].initialized = TKIP_INITIALIZED_UPDATE_KEY;
 	}

 	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);
Index: linux-2.6/net/mac80211/key.h
===================================================================
--- linux-2.6.orig/net/mac80211/key.h	2009-08-21 22:03:09.000000000 +0000
+++ linux-2.6/net/mac80211/key.h	2009-08-21 22:03:12.000000000 +0000
@@ -59,11 +59,17 @@
 	KEY_FLAG_TODO_DEFMGMTKEY	= BIT(6),
 };

+enum ieee80211_internal_tkip_initialized {
+	TKIP_INITIALIZED_NONE,
+	TKIP_INITIALIZED_PHASE1,
+	TKIP_INITIALIZED_UPDATE_KEY,
+};
+
 struct tkip_ctx {
 	u32 iv32;
 	u16 iv16;
 	u16 p1k[5];
-	int initialized;
+	enum ieee80211_internal_tkip_initialized initialized;
 };

 struct ieee80211_key {

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Sat, 2009-08-22 at 00:13 +0200, gregor kowski wrote:
> The mac80211 tkip code won't call update_tkip_key, if some rx packets

s/,// s/rx //

> get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on

s/get/are/

> first packet because the hardware key stuff is called asynchronously with
> todo workqueue.
> 
> This patch workaround that by tracking if we send the key to hardware.

s/send/sent/, s/hardware/the driver/


> +enum ieee80211_internal_tkip_initialized {
> +	TKIP_INITIALIZED_NONE,
> +	TKIP_INITIALIZED_PHASE1,
> +	TKIP_INITIALIZED_UPDATE_KEY,
> +};

Those constants and the enum itself really need better names. This way,
there's no way to understand what it means without reading all the code.

johannes

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

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

Hi,

sorry for the long delay.

On Sat, Aug 22, 2009 at 8:45 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sat, 2009-08-22 at 00:13 +0200, gregor kowski wrote:
>
>> +enum ieee80211_internal_tkip_initialized {
>> +     TKIP_INITIALIZED_NONE,
>> +     TKIP_INITIALIZED_PHASE1,
>> +     TKIP_INITIALIZED_UPDATE_KEY,
>> +};
>
> Those constants and the enum itself really need better names. This way,
> there's no way to understand what it means without reading all the code.
>
Could you suggest better name ?

ieee80211_internal_tkip_state and
TKIP_STATE_NOT_INIT
TKIP_STATE_PHASE1_DONE
TKIP_STATE_PHASE1_HW_UPLOADED
?

Thanks

Gregor

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Sat, 2009-11-07 at 19:10 +0100, gregor kowski wrote:

> >> +enum ieee80211_internal_tkip_initialized {
> >> +     TKIP_INITIALIZED_NONE,
> >> +     TKIP_INITIALIZED_PHASE1,
> >> +     TKIP_INITIALIZED_UPDATE_KEY,
> >> +};
> >
> > Those constants and the enum itself really need better names. This way,
> > there's no way to understand what it means without reading all the code.
> >
> Could you suggest better name ?
> 
> ieee80211_internal_tkip_state and
> TKIP_STATE_NOT_INIT
> TKIP_STATE_PHASE1_DONE
> TKIP_STATE_PHASE1_HW_UPLOADED

I, eh, ... have no idea :)

johannes

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

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

The mac80211 tkip code won't call update_tkip_key, if rx packets
are received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
first packet because the hardware key stuff is called asynchronously with
todo workqueue.

This patch workaround that by tracking if we sent the key to the driver.

Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>

Index: linux-2.6/net/mac80211/tkip.c
===================================================================
--- linux-2.6.orig/net/mac80211/tkip.c	2009-11-16 21:44:37.000000000 +0000
+++ linux-2.6/net/mac80211/tkip.c	2009-11-16 21:46:28.000000000 +0000
@@ -100,7 +100,7 @@
 		p1k[3] += tkipS(p1k[2] ^ get_unaligned_le16(tk + 12 + j));
 		p1k[4] += tkipS(p1k[3] ^ get_unaligned_le16(tk + 0 + j)) + i;
 	}
-	ctx->initialized = 1;
+	ctx->initialized = TKIP_STATE_PHASE1_DONE;
 }

 static void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
@@ -183,7 +183,7 @@
 	/* Update the p1k only when the iv16 in the packet wraps around, this
 	 * might occur after the wrap around of iv16 in the key in case of
 	 * fragmented packets. */
-	if (iv16 == 0 || !ctx->initialized)
+	if (iv16 == 0 || ctx->initialized == TKIP_STATE_NOT_INIT)
 		tkip_mixing_phase1(tk, ctx, hdr->addr2, iv32);

 	if (type == IEEE80211_TKIP_P1_KEY) {
@@ -209,7 +209,7 @@
 	const u8 *tk = &key->conf.key[NL80211_TKIP_DATA_OFFSET_ENCR_KEY];

 	/* Calculate per-packet key */
-	if (ctx->iv16 == 0 || !ctx->initialized)
+	if (ctx->iv16 == 0 || ctx->initialized == TKIP_STATE_NOT_INIT)
 		tkip_mixing_phase1(tk, ctx, ta, ctx->iv32);

 	tkip_mixing_phase2(tk, ctx, ctx->iv16, rc4key);
@@ -259,7 +259,7 @@
 	if ((keyid >> 6) != key->conf.keyidx)
 		return TKIP_DECRYPT_INVALID_KEYIDX;

-	if (key->u.tkip.rx[queue].initialized &&
+	if (key->u.tkip.rx[queue].initialized != TKIP_STATE_NOT_INIT &&
 	    (iv32 < key->u.tkip.rx[queue].iv32 ||
 	     (iv32 == key->u.tkip.rx[queue].iv32 &&
 	      iv16 <= key->u.tkip.rx[queue].iv16))) {
@@ -275,11 +275,11 @@

 	if (only_iv) {
 		res = TKIP_DECRYPT_OK;
-		key->u.tkip.rx[queue].initialized = 1;
+		key->u.tkip.rx[queue].initialized = TKIP_STATE_PHASE1_HW_UPLOADED;
 		goto done;
 	}

-	if (!key->u.tkip.rx[queue].initialized ||
+	if (key->u.tkip.rx[queue].initialized == TKIP_STATE_NOT_INIT ||
 	    key->u.tkip.rx[queue].iv32 != iv32) {
 		/* IV16 wrapped around - perform TKIP phase 1 */
 		tkip_mixing_phase1(tk, &key->u.tkip.rx[queue], ta, iv32);
@@ -299,18 +299,20 @@
 			printk("\n");
 		}
 #endif
-		if (key->local->ops->update_tkip_key &&
-			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
-			u8 bcast[ETH_ALEN] =
-				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-			u8 *sta_addr = key->sta->sta.addr;
-
-			if (is_multicast_ether_addr(ra))
-				sta_addr = bcast;
-
-			drv_update_tkip_key(key->local, &key->conf, sta_addr,
-					    iv32, key->u.tkip.rx[queue].p1k);
-		}
+	}
+	if (key->local->ops->update_tkip_key &&
+	    key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
+	    key->u.tkip.rx[queue].initialized != TKIP_STATE_PHASE1_HW_UPLOADED) {
+		u8 bcast[ETH_ALEN] =
+			{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+		u8 *sta_addr = key->sta->sta.addr;
+
+		if (is_multicast_ether_addr(ra))
+			sta_addr = bcast;
+
+		drv_update_tkip_key(key->local, &key->conf, sta_addr,
+					iv32, key->u.tkip.rx[queue].p1k);
+		key->u.tkip.rx[queue].initialized = TKIP_STATE_PHASE1_HW_UPLOADED;
 	}

 	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);
Index: linux-2.6/net/mac80211/key.h
===================================================================
--- linux-2.6.orig/net/mac80211/key.h	2009-11-16 21:44:37.000000000 +0000
+++ linux-2.6/net/mac80211/key.h	2009-11-16 21:50:23.000000000 +0000
@@ -59,11 +59,17 @@
 	KEY_FLAG_TODO_DEFMGMTKEY	= BIT(6),
 };

+enum ieee80211_internal_tkip_state {
+	TKIP_STATE_NOT_INIT,
+	TKIP_STATE_PHASE1_DONE,
+	TKIP_STATE_PHASE1_HW_UPLOADED,
+};
+
 struct tkip_ctx {
 	u32 iv32;
 	u16 iv16;
 	u16 p1k[5];
-	int initialized;
+	enum ieee80211_internal_tkip_state initialized;
 };

 struct ieee80211_key {

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Mon, 2009-11-16 at 22:53 +0100, gregor kowski wrote:
> The mac80211 tkip code won't call update_tkip_key, if rx packets
> are received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
> first packet because the hardware key stuff is called asynchronously with
> todo workqueue.

> -	ctx->initialized = 1;
> +	ctx->initialized = TKIP_STATE_PHASE1_DONE;

Now you just need to rename the variable holding it to "state" too...

johannes

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

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

The mac80211 tkip code won't call update_tkip_key, if rx packets
are received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
first packet because the hardware key stuff is called asynchronously with
todo workqueue.

This patch workaround that by tracking if we sent the key to the driver.

Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>

Index: linux-2.6/net/mac80211/tkip.c
===================================================================
--- linux-2.6.orig/net/mac80211/tkip.c	2009-11-16 21:44:37.000000000 +0000
+++ linux-2.6/net/mac80211/tkip.c	2009-12-07 21:00:45.000000000 +0000
@@ -100,7 +100,7 @@
 		p1k[3] += tkipS(p1k[2] ^ get_unaligned_le16(tk + 12 + j));
 		p1k[4] += tkipS(p1k[3] ^ get_unaligned_le16(tk + 0 + j)) + i;
 	}
-	ctx->initialized = 1;
+	ctx->state = TKIP_STATE_PHASE1_DONE;
 }

 static void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
@@ -183,7 +183,7 @@
 	/* Update the p1k only when the iv16 in the packet wraps around, this
 	 * might occur after the wrap around of iv16 in the key in case of
 	 * fragmented packets. */
-	if (iv16 == 0 || !ctx->initialized)
+	if (iv16 == 0 || ctx->state == TKIP_STATE_NOT_INIT)
 		tkip_mixing_phase1(tk, ctx, hdr->addr2, iv32);

 	if (type == IEEE80211_TKIP_P1_KEY) {
@@ -209,7 +209,7 @@
 	const u8 *tk = &key->conf.key[NL80211_TKIP_DATA_OFFSET_ENCR_KEY];

 	/* Calculate per-packet key */
-	if (ctx->iv16 == 0 || !ctx->initialized)
+	if (ctx->iv16 == 0 || ctx->state == TKIP_STATE_NOT_INIT)
 		tkip_mixing_phase1(tk, ctx, ta, ctx->iv32);

 	tkip_mixing_phase2(tk, ctx, ctx->iv16, rc4key);
@@ -259,7 +259,7 @@
 	if ((keyid >> 6) != key->conf.keyidx)
 		return TKIP_DECRYPT_INVALID_KEYIDX;

-	if (key->u.tkip.rx[queue].initialized &&
+	if (key->u.tkip.rx[queue].state != TKIP_STATE_NOT_INIT &&
 	    (iv32 < key->u.tkip.rx[queue].iv32 ||
 	     (iv32 == key->u.tkip.rx[queue].iv32 &&
 	      iv16 <= key->u.tkip.rx[queue].iv16))) {
@@ -275,11 +275,11 @@

 	if (only_iv) {
 		res = TKIP_DECRYPT_OK;
-		key->u.tkip.rx[queue].initialized = 1;
+		key->u.tkip.rx[queue].state = TKIP_STATE_PHASE1_HW_UPLOADED;
 		goto done;
 	}

-	if (!key->u.tkip.rx[queue].initialized ||
+	if (key->u.tkip.rx[queue].state == TKIP_STATE_NOT_INIT ||
 	    key->u.tkip.rx[queue].iv32 != iv32) {
 		/* IV16 wrapped around - perform TKIP phase 1 */
 		tkip_mixing_phase1(tk, &key->u.tkip.rx[queue], ta, iv32);
@@ -299,18 +299,20 @@
 			printk("\n");
 		}
 #endif
-		if (key->local->ops->update_tkip_key &&
-			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
-			u8 bcast[ETH_ALEN] =
-				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-			u8 *sta_addr = key->sta->sta.addr;
-
-			if (is_multicast_ether_addr(ra))
-				sta_addr = bcast;
-
-			drv_update_tkip_key(key->local, &key->conf, sta_addr,
-					    iv32, key->u.tkip.rx[queue].p1k);
-		}
+	}
+	if (key->local->ops->update_tkip_key &&
+	    key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
+	    key->u.tkip.rx[queue].state != TKIP_STATE_PHASE1_HW_UPLOADED) {
+		u8 bcast[ETH_ALEN] =
+			{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+		u8 *sta_addr = key->sta->sta.addr;
+
+		if (is_multicast_ether_addr(ra))
+			sta_addr = bcast;
+
+		drv_update_tkip_key(key->local, &key->conf, sta_addr,
+					iv32, key->u.tkip.rx[queue].p1k);
+		key->u.tkip.rx[queue].state = TKIP_STATE_PHASE1_HW_UPLOADED;
 	}

 	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);
Index: linux-2.6/net/mac80211/key.h
===================================================================
--- linux-2.6.orig/net/mac80211/key.h	2009-11-16 21:44:37.000000000 +0000
+++ linux-2.6/net/mac80211/key.h	2009-12-07 20:58:47.000000000 +0000
@@ -59,11 +59,17 @@
 	KEY_FLAG_TODO_DEFMGMTKEY	= BIT(6),
 };

+enum ieee80211_internal_tkip_state {
+	TKIP_STATE_NOT_INIT,
+	TKIP_STATE_PHASE1_DONE,
+	TKIP_STATE_PHASE1_HW_UPLOADED,
+};
+
 struct tkip_ctx {
 	u32 iv32;
 	u16 iv16;
 	u16 p1k[5];
-	int initialized;
+	enum ieee80211_internal_tkip_state state;
 };

 struct ieee80211_key {

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

On 11/16/09, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2009-11-16 at 22:53 +0100, gregor kowski wrote:
>> The mac80211 tkip code won't call update_tkip_key, if rx packets
>> are received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
>> first packet because the hardware key stuff is called asynchronously with
>> todo workqueue.
>
>> -	ctx->initialized = 1;
>> +	ctx->initialized = TKIP_STATE_PHASE1_DONE;
>
> Now you just need to rename the variable holding it to "state" too...
Done

Sorry for the delay

Gregor

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

The patch doesn't seem to apply to new version of kernel.
I am submitting a new one.

On 12/7/09, gregor kowski <gregor.kowski@gmail.com> wrote:
> On 11/16/09, Johannes Berg <johannes@sipsolutions.net> wrote:
>> On Mon, 2009-11-16 at 22:53 +0100, gregor kowski wrote:
>>> The mac80211 tkip code won't call update_tkip_key, if rx packets
>>> are received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
>>> first packet because the hardware key stuff is called asynchronously
>>> with
>>> todo workqueue.
>>
>>> -	ctx->initialized = 1;
>>> +	ctx->initialized = TKIP_STATE_PHASE1_DONE;
>>
>> Now you just need to rename the variable holding it to "state" too...
> Done
>
> Sorry for the delay
>
> Gregor
>

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

The mac80211 tkip code won't call update_tkip_key, if rx packets
are received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
first packet because the hardware key stuff is called asynchronously with
todo workqueue.

This patch workaround that by tracking if we sent the key to the driver.

Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>

Index: linux-2.6/net/mac80211/tkip.c
===================================================================
--- linux-2.6.orig/net/mac80211/tkip.c	2009-12-08 20:37:41.000000000 +0000
+++ linux-2.6/net/mac80211/tkip.c	2009-12-08 20:42:23.000000000 +0000
@@ -100,7 +100,7 @@
 		p1k[3] += tkipS(p1k[2] ^ get_unaligned_le16(tk + 12 + j));
 		p1k[4] += tkipS(p1k[3] ^ get_unaligned_le16(tk + 0 + j)) + i;
 	}
-	ctx->initialized = 1;
+	ctx->state = TKIP_STATE_PHASE1_DONE;
 }

 static void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
@@ -183,7 +183,7 @@
 	/* Update the p1k only when the iv16 in the packet wraps around, this
 	 * might occur after the wrap around of iv16 in the key in case of
 	 * fragmented packets. */
-	if (iv16 == 0 || !ctx->initialized)
+	if (iv16 == 0 || ctx->state == TKIP_STATE_NOT_INIT)
 		tkip_mixing_phase1(tk, ctx, hdr->addr2, iv32);

 	if (type == IEEE80211_TKIP_P1_KEY) {
@@ -209,7 +209,7 @@
 	const u8 *tk = &key->conf.key[NL80211_TKIP_DATA_OFFSET_ENCR_KEY];

 	/* Calculate per-packet key */
-	if (ctx->iv16 == 0 || !ctx->initialized)
+	if (ctx->iv16 == 0 || ctx->state == TKIP_STATE_NOT_INIT)
 		tkip_mixing_phase1(tk, ctx, ta, ctx->iv32);

 	tkip_mixing_phase2(tk, ctx, ctx->iv16, rc4key);
@@ -259,7 +259,7 @@
 	if ((keyid >> 6) != key->conf.keyidx)
 		return TKIP_DECRYPT_INVALID_KEYIDX;

-	if (key->u.tkip.rx[queue].initialized &&
+	if (key->u.tkip.rx[queue].state != TKIP_STATE_NOT_INIT &&
 	    (iv32 < key->u.tkip.rx[queue].iv32 ||
 	     (iv32 == key->u.tkip.rx[queue].iv32 &&
 	      iv16 <= key->u.tkip.rx[queue].iv16))) {
@@ -275,11 +275,11 @@

 	if (only_iv) {
 		res = TKIP_DECRYPT_OK;
-		key->u.tkip.rx[queue].initialized = 1;
+		key->u.tkip.rx[queue].state = TKIP_STATE_PHASE1_HW_UPLOADED;
 		goto done;
 	}

-	if (!key->u.tkip.rx[queue].initialized ||
+	if (key->u.tkip.rx[queue].state == TKIP_STATE_NOT_INIT ||
 	    key->u.tkip.rx[queue].iv32 != iv32) {
 		/* IV16 wrapped around - perform TKIP phase 1 */
 		tkip_mixing_phase1(tk, &key->u.tkip.rx[queue], ta, iv32);
@@ -299,18 +299,20 @@
 			printk("\n");
 		}
 #endif
-		if (key->local->ops->update_tkip_key &&
-			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
-			static const u8 bcast[ETH_ALEN] =
-				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-			const u8 *sta_addr = key->sta->sta.addr;
-
-			if (is_multicast_ether_addr(ra))
-				sta_addr = bcast;
-
-			drv_update_tkip_key(key->local, &key->conf, sta_addr,
-					    iv32, key->u.tkip.rx[queue].p1k);
-		}
+	}
+	if (key->local->ops->update_tkip_key &&
+	    key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
+	    key->u.tkip.rx[queue].state != TKIP_STATE_PHASE1_HW_UPLOADED) {
+		static const u8 bcast[ETH_ALEN] =
+		{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+		const u8 *sta_addr = key->sta->sta.addr;
+
+		if (is_multicast_ether_addr(ra))
+			sta_addr = bcast;
+
+		drv_update_tkip_key(key->local, &key->conf, sta_addr,
+				iv32, key->u.tkip.rx[queue].p1k);
+		key->u.tkip.rx[queue].state = TKIP_STATE_PHASE1_HW_UPLOADED;
 	}

 	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);
Index: linux-2.6/net/mac80211/key.h
===================================================================
--- linux-2.6.orig/net/mac80211/key.h	2009-12-08 20:37:41.000000000 +0000
+++ linux-2.6/net/mac80211/key.h	2009-12-08 20:39:23.000000000 +0000
@@ -59,11 +59,17 @@
 	KEY_FLAG_TODO_DEFMGMTKEY	= BIT(6),
 };

+enum ieee80211_internal_tkip_state {
+	TKIP_STATE_NOT_INIT,
+	TKIP_STATE_PHASE1_DONE,
+	TKIP_STATE_PHASE1_HW_UPLOADED,
+};
+
 struct tkip_ctx {
 	u32 iv32;
 	u16 iv16;
 	u16 p1k[5];
-	int initialized;
+	enum ieee80211_internal_tkip_state state;
 };

 struct ieee80211_key {

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

Hi,

On 12/9/09, gregor kowski <gregor.kowski@gmail.com> wrote:
> The mac80211 tkip code won't call update_tkip_key, if rx packets
> are received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
> first packet because the hardware key stuff is called asynchronously with
> todo workqueue.
>
> This patch workaround that by tracking if we sent the key to the driver.
>
What's the status of this patch ?

Regards,

Gregor

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[John W. Linville]

On Mon, Dec 28, 2009 at 05:46:46PM +0100, gregor kowski wrote:
> Hi,
> 
> On 12/9/09, gregor kowski <gregor.kowski@gmail.com> wrote:
> > The mac80211 tkip code won't call update_tkip_key, if rx packets
> > are received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
> > first packet because the hardware key stuff is called asynchronously with
> > todo workqueue.
> >
> > This patch workaround that by tracking if we sent the key to the driver.
> >
> What's the status of this patch ?

commit ca99861d5421c91f5a8fd3a77acb4b7be14f119d
Author: gregor kowski <gregor.kowski@gmail.com>
Date:   Wed Dec 9 23:25:05 2009 +0100

    mac80211 : fix a race with update_tkip_key
    
    The mac80211 tkip code won't call update_tkip_key, if rx packets
    are received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
    first packet because the hardware key stuff is called asynchronously with
    todo workqueue.
    
    This patch workaround that by tracking if we sent the key to the driver.
    
    Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

Race could happen with QOS :
let's consider queue 0 low prio and queue 1 high prio

- queue 1 send packet lot's of packet (iv32 = 0, iv32=1, iv32=2)
- queue 0 send a packet with iv32=1 and iv16=0xffff (or near wrap)

If queue 0 packet with iv32=1 is received after queue 1 with iv32=2, we will
- send tkip key for iv32=2 to hardware
- send tkip key for iv32=1 to hardware
and then we should wait wrap to iv32=3 to have a chance to send
correct key to hardware.


With this patch, we only send tkip key to hardware on the first wrap of a queue.
With the previous example, we will only send  tkip key for iv32=2 to hardware.

Gregor

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Fri, 2009-06-19 at 21:37 +0200, gregor kowski wrote:
> Update : I changed the logic. There is a single flag per key, tracking
> if we send the key instead of a flag per rx queue.
> 
> The mac80211 tkip code won't call update_tkip_key, if some rx packets
> get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
> first packet because the hardware key stuff is called asynchronously with
> todo workqueue.
> 
> This patch workaround that by tracking if we send the key to hardware.

What was wrong with using an enum, in the existing variable?

johannes

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

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

Update : I changed the logic. There is a single flag per key, tracking
if we send the key instead of a flag per rx queue.

The mac80211 tkip code won't call update_tkip_key, if some rx packets
get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
first packet because the hardware key stuff is called asynchronously with
todo workqueue.

This patch workaround that by tracking if we send the key to hardware.


Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>
Index: linux-2.6/net/mac80211/tkip.c
===================================================================
--- linux-2.6.orig/net/mac80211/tkip.c	2009-06-19 19:13:47.000000000 +0000
+++ linux-2.6/net/mac80211/tkip.c	2009-06-19 19:21:50.000000000 +0000
@@ -282,6 +282,7 @@
 	    key->u.tkip.rx[queue].iv32 != iv32) {
 		/* IV16 wrapped around - perform TKIP phase 1 */
 		tkip_mixing_phase1(tk, &key->u.tkip.rx[queue], ta, iv32);
+		key->u.tkip.rx_tkip_key_sent = 0;
 #ifdef CONFIG_MAC80211_TKIP_DEBUG
 		{
 			int i;
@@ -298,19 +299,21 @@
 			printk("\n");
 		}
 #endif
-		if (key->local->ops->update_tkip_key &&
-			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
-			u8 bcast[ETH_ALEN] =
-				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-			u8 *sta_addr = key->sta->sta.addr;
+	}
+	if (key->local->ops->update_tkip_key &&
+	    key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
+	    !key->u.tkip.rx_tkip_key_sent) {
+		u8 bcast[ETH_ALEN] =
+			{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+		u8 *sta_addr = key->sta->sta.addr;

-			if (is_multicast_ether_addr(ra))
-				sta_addr = bcast;
+		if (is_multicast_ether_addr(ra))
+			sta_addr = bcast;

-			key->local->ops->update_tkip_key(
-				local_to_hw(key->local), &key->conf,
-				sta_addr, iv32, key->u.tkip.rx[queue].p1k);
-		}
+		key->local->ops->update_tkip_key(
+			local_to_hw(key->local), &key->conf,
+			sta_addr, iv32, key->u.tkip.rx[queue].p1k);
+		key->u.tkip.rx_tkip_key_sent = 1;
 	}

 	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);
Index: linux-2.6/net/mac80211/key.h
===================================================================
--- linux-2.6.orig/net/mac80211/key.h	2009-06-19 19:22:20.000000000 +0000
+++ linux-2.6/net/mac80211/key.h	2009-06-19 19:22:31.000000000 +0000
@@ -86,6 +86,7 @@

 			/* last received RSC */
 			struct tkip_ctx rx[NUM_RX_DATA_QUEUES];
+			int rx_tkip_key_sent;
 		} tkip;
 		struct {
 			u8 tx_pn[6];

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

Update : I changed the logic. There is a single flag per key, tracking
if we send the key instead of a flag per rx queue.

The mac80211 tkip code won't call update_tkip_key, if some rx packets
get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
first packet because the hardware key stuff is called asynchronously with
todo workqueue.

This patch workaround that by tracking if we send the key to hardware.


Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>
Index: linux-2.6/net/mac80211/tkip.c
===================================================================
--- linux-2.6.orig/net/mac80211/tkip.c	2009-06-19 19:13:47.000000000 +0000
+++ linux-2.6/net/mac80211/tkip.c	2009-06-19 19:21:50.000000000 +0000
@@ -282,6 +282,7 @@
 	    key->u.tkip.rx[queue].iv32 != iv32) {
 		/* IV16 wrapped around - perform TKIP phase 1 */
 		tkip_mixing_phase1(tk, &key->u.tkip.rx[queue], ta, iv32);
+		key->u.tkip.rx_tkip_key_sent = 0;
 #ifdef CONFIG_MAC80211_TKIP_DEBUG
 		{
 			int i;
@@ -298,19 +299,21 @@
 			printk("\n");
 		}
 #endif
-		if (key->local->ops->update_tkip_key &&
-			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
-			u8 bcast[ETH_ALEN] =
-				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-			u8 *sta_addr = key->sta->sta.addr;
+	}
+	if (key->local->ops->update_tkip_key &&
+	    key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
+	    !key->u.tkip.rx_tkip_key_sent) {
+		u8 bcast[ETH_ALEN] =
+			{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+		u8 *sta_addr = key->sta->sta.addr;

-			if (is_multicast_ether_addr(ra))
-				sta_addr = bcast;
+		if (is_multicast_ether_addr(ra))
+			sta_addr = bcast;

-			key->local->ops->update_tkip_key(
-				local_to_hw(key->local), &key->conf,
-				sta_addr, iv32, key->u.tkip.rx[queue].p1k);
-		}
+		key->local->ops->update_tkip_key(
+			local_to_hw(key->local), &key->conf,
+			sta_addr, iv32, key->u.tkip.rx[queue].p1k);
+		key->u.tkip.rx_tkip_key_sent = 1;
 	}

 	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);
Index: linux-2.6/net/mac80211/key.h
===================================================================
--- linux-2.6.orig/net/mac80211/key.h	2009-06-19 19:22:20.000000000 +0000
+++ linux-2.6/net/mac80211/key.h	2009-06-19 19:22:31.000000000 +0000
@@ -86,6 +86,7 @@

 			/* last received RSC */
 			struct tkip_ctx rx[NUM_RX_DATA_QUEUES];
+			int rx_tkip_key_sent;
 		} tkip;
 		struct {
 			u8 tx_pn[6];

On 6/12/09, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2009-06-12 at 22:41 +0200, gregor kowski wrote:
>
>
> Please try using a proper email client that can inline patches, as
> outlined in
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/email-clients.txt;hb=HEAD
>
> Due to you not doing that, I now have to copy/paste from your attachment
> rather than being able to simply reply.
>
>> Here is a patch that should fix all issue :
>
> Remove that sentence?
>
>> The mac80211 tkip code won't call update_tkip_key, if some rx packets
>> get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
>> first packet because the hardware key stuff is called asynchronously with
>> todo workqueue.
>
> That seems fine.
>
>> This patch workaround that by always calling once update_tkip_key if
>> the packet wasn't decrypted by the hardware.
>
> But I don't think this is actually true?
>
>> +enum {
>> +       INITIALIZED_NONE,
>> +       INITIALIZED_PHASE1,
>> +       INITIALIZED_UPDATE_KEY,
>> +};
>
> Please declare this in a header file and use the proper enum type for
> the variable as well so the compiler knows what you're doing. You should
> also prefix it then.
>
> johannes
>

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Fri, 2009-06-12 at 22:41 +0200, gregor kowski wrote:


Please try using a proper email client that can inline patches, as
outlined in
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/email-clients.txt;hb=HEAD

Due to you not doing that, I now have to copy/paste from your attachment
rather than being able to simply reply.

> Here is a patch that should fix all issue :

Remove that sentence?

> The mac80211 tkip code won't call update_tkip_key, if some rx packets
> get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
> first packet because the hardware key stuff is called asynchronously with
> todo workqueue.

That seems fine.

> This patch workaround that by always calling once update_tkip_key if
> the packet wasn't decrypted by the hardware.

But I don't think this is actually true?

> +enum {
> +       INITIALIZED_NONE,
> +       INITIALIZED_PHASE1,
> +       INITIALIZED_UPDATE_KEY,
> +};

Please declare this in a header file and use the proper enum type for
the variable as well so the compiler knows what you're doing. You should
also prefix it then.

johannes

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

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

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

Here is a patch that should fix all issue :
The mac80211 tkip code won't call update_tkip_key, if some rx packets
get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
first packet because the hardware key stuff is called asynchronously with
todo workqueue.

This patch workaround that by always calling once update_tkip_key if
the packet wasn't decrypted by the hardware.

[-- Attachment #2: tkip_corev2.diff --]
[-- Type: text/x-diff, Size: 3756 bytes --]

The mac80211 tkip code won't call update_tkip_key, if some rx packets
get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
first packet because the hardware key stuff is called asynchronously with 
todo workqueue.

This patch workaround that by always calling once update_tkip_key if
the packet wasn't decrypted by the hardware.

Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>
Index: linux-2.6/net/mac80211/tkip.c
===================================================================
--- linux-2.6.orig/net/mac80211/tkip.c	2009-06-10 20:12:32.000000000 +0000
+++ linux-2.6/net/mac80211/tkip.c	2009-06-12 20:31:35.000000000 +0000
@@ -19,6 +19,12 @@
 
 #define PHASE1_LOOP_COUNT 8
 
+enum {
+	INITIALIZED_NONE,
+	INITIALIZED_PHASE1,
+	INITIALIZED_UPDATE_KEY,
+};
+
 /*
  * 2-byte by 2-byte subset of the full AES S-box table; second part of this
  * table is identical to first part but byte-swapped
@@ -99,7 +105,7 @@
 		p1k[3] += tkipS(p1k[2] ^ get_unaligned_le16(tk + 12 + j));
 		p1k[4] += tkipS(p1k[3] ^ get_unaligned_le16(tk + 0 + j)) + i;
 	}
-	ctx->initialized = 1;
+	ctx->initialized = INITIALIZED_PHASE1;
 }
 
 static void tkip_mixing_phase2(const u8 *tk, struct tkip_ctx *ctx,
@@ -182,7 +188,7 @@
 	/* Update the p1k only when the iv16 in the packet wraps around, this
 	 * might occur after the wrap around of iv16 in the key in case of
 	 * fragmented packets. */
-	if (iv16 == 0 || !ctx->initialized)
+	if (iv16 == 0 || ctx->initialized == INITIALIZED_NONE)
 		tkip_mixing_phase1(tk, ctx, hdr->addr2, iv32);
 
 	if (type == IEEE80211_TKIP_P1_KEY) {
@@ -208,7 +214,7 @@
 	const u8 *tk = &key->conf.key[NL80211_TKIP_DATA_OFFSET_ENCR_KEY];
 
 	/* Calculate per-packet key */
-	if (ctx->iv16 == 0 || !ctx->initialized)
+	if (ctx->iv16 == 0 || ctx->initialized == INITIALIZED_NONE)
 		tkip_mixing_phase1(tk, ctx, ta, ctx->iv32);
 
 	tkip_mixing_phase2(tk, ctx, ctx->iv16, rc4key);
@@ -258,7 +264,7 @@
 	if ((keyid >> 6) != key->conf.keyidx)
 		return TKIP_DECRYPT_INVALID_KEYIDX;
 
-	if (key->u.tkip.rx[queue].initialized &&
+	if (key->u.tkip.rx[queue].initialized != INITIALIZED_NONE &&
 	    (iv32 < key->u.tkip.rx[queue].iv32 ||
 	     (iv32 == key->u.tkip.rx[queue].iv32 &&
 	      iv16 <= key->u.tkip.rx[queue].iv16))) {
@@ -274,11 +280,11 @@
 
 	if (only_iv) {
 		res = TKIP_DECRYPT_OK;
-		key->u.tkip.rx[queue].initialized = 1;
+		key->u.tkip.rx[queue].initialized = INITIALIZED_UPDATE_KEY;
 		goto done;
 	}
 
-	if (!key->u.tkip.rx[queue].initialized ||
+	if (key->u.tkip.rx[queue].initialized == INITIALIZED_NONE ||
 	    key->u.tkip.rx[queue].iv32 != iv32) {
 		/* IV16 wrapped around - perform TKIP phase 1 */
 		tkip_mixing_phase1(tk, &key->u.tkip.rx[queue], ta, iv32);
@@ -298,19 +304,21 @@
 			printk("\n");
 		}
 #endif
-		if (key->local->ops->update_tkip_key &&
-			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
-			u8 bcast[ETH_ALEN] =
-				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-			u8 *sta_addr = key->sta->sta.addr;
-
-			if (is_multicast_ether_addr(ra))
-				sta_addr = bcast;
-
-			key->local->ops->update_tkip_key(
-				local_to_hw(key->local), &key->conf,
-				sta_addr, iv32, key->u.tkip.rx[queue].p1k);
-		}
+	}
+	if (key->local->ops->update_tkip_key &&
+	    key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
+	    key->u.tkip.rx[queue].initialized != INITIALIZED_UPDATE_KEY) {
+		u8 bcast[ETH_ALEN] =
+			{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+		u8 *sta_addr = key->sta->sta.addr;
+
+		if (is_multicast_ether_addr(ra))
+			sta_addr = bcast;
+
+		key->local->ops->update_tkip_key(
+			local_to_hw(key->local), &key->conf,
+			sta_addr, iv32, key->u.tkip.rx[queue].p1k);
+		key->u.tkip.rx[queue].initialized = INITIALIZED_UPDATE_KEY;
 	}
 
 	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Thu, 2009-06-11 at 00:17 +0200, gregor kowski wrote:

> >> Might depend on something else? Anyhow I don't see the point of
> >> continuing to call the callback. Maybe it should just be part of the key
> >> todo instead when the key is initially uploaded to the hw.
> >
> > Or what do you think about this. This will call the callback only once per wrap.

> But this won't work if you want to support QOS.
> So how update_tkip_key is supposed to work with QOS (ie different
> rx->queue value) and multiple tkip IV ?
> 
> BTW did you know why there only one tkip IV on tx and not one per qos queue ?

Umm, have you read the standard? This is correct behaviour and works as
specified with QoS.

johannes

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

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Wed, 2009-06-10 at 21:42 +0200, gregor kowski wrote:

>  	if (only_iv) {
>  		res = TKIP_DECRYPT_OK;
> -		key->u.tkip.rx[queue].initialized = 1;
> +		key->u.tkip.rx[queue].initialized = 2;
>  		goto done;

Please change this variable to an enum with proper states.

> -		if (key->local->ops->update_tkip_key &&
> -			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> -			u8 bcast[ETH_ALEN] =
> -				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> -			u8 *sta_addr = key->sta->sta.addr;
> -
> -			if (is_multicast_ether_addr(ra))
> -				sta_addr = bcast;
> -
> -			key->local->ops->update_tkip_key(
> -				local_to_hw(key->local), &key->conf,
> -				sta_addr, iv32, key->u.tkip.rx[queue].p1k);
> -		}
> +	}
> +	/* initialized == 2 means we already call update_tkip_key */
> +	if (key->local->ops->update_tkip_key &&
> +		key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
> +		key->u.tkip.rx[queue].initialized != 2) {

and please indent only to after if ( here, like this:

	if (key->... &&
	    key->flags & ...
	    key->...) {
		u8 bcast[...]...

johannes

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

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

On Wed, Jun 10, 2009 at 9:42 PM, gregor kowski<gregor.kowski@gmail.com> wrote:
> On Tue, Jun 9, 2009 at 7:52 PM, Johannes Berg<johannes@sipsolutions.net> wrote:
>> On Tue, 2009-06-09 at 19:48 +0200, gregor kowski wrote:
>>
>>> > Right. But drivers are free to even only _encrypt_ tkip frames and never
>>> > _decrypt_ them after having accepted a hardware key, iow that is
>>> > perfectly valid behaviour and I don't think we should keep uploading the
>>> > key to the driver. Worst case is that the proper upload fails and we
>>> > decrypt all frames in software until the next rollover.
>>> >
>>> What's the point of setting the tkip callback if we aren't interested
>>> in decrypting data by hardware ?
>>
>> Might depend on something else? Anyhow I don't see the point of
>> continuing to call the callback. Maybe it should just be part of the key
>> todo instead when the key is initially uploaded to the hw.
>
> Or what do you think about this. This will call the callback only once per wrap.
But this won't work if you want to support QOS.
So how update_tkip_key is supposed to work with QOS (ie different
rx->queue value) and multiple tkip IV ?

BTW did you know why there only one tkip IV on tx and not one per qos queue ?

Gregor

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

On Tue, Jun 9, 2009 at 7:52 PM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Tue, 2009-06-09 at 19:48 +0200, gregor kowski wrote:
>
>> > Right. But drivers are free to even only _encrypt_ tkip frames and never
>> > _decrypt_ them after having accepted a hardware key, iow that is
>> > perfectly valid behaviour and I don't think we should keep uploading the
>> > key to the driver. Worst case is that the proper upload fails and we
>> > decrypt all frames in software until the next rollover.
>> >
>> What's the point of setting the tkip callback if we aren't interested
>> in decrypting data by hardware ?
>
> Might depend on something else? Anyhow I don't see the point of
> continuing to call the callback. Maybe it should just be part of the key
> todo instead when the key is initially uploaded to the hw.

Or what do you think about this. This will call the callback only once per wrap.

Gregor.


Index: linux-2.6/net/mac80211/tkip.c
===================================================================
--- linux-2.6.orig/net/mac80211/tkip.c	2009-06-08 19:37:19.000000000 +0000
+++ linux-2.6/net/mac80211/tkip.c	2009-06-10 19:28:20.000000000 +0000
@@ -274,7 +274,7 @@

 	if (only_iv) {
 		res = TKIP_DECRYPT_OK;
-		key->u.tkip.rx[queue].initialized = 1;
+		key->u.tkip.rx[queue].initialized = 2;
 		goto done;
 	}

@@ -298,19 +298,22 @@
 			printk("\n");
 		}
 #endif
-		if (key->local->ops->update_tkip_key &&
-			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
-			u8 bcast[ETH_ALEN] =
-				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-			u8 *sta_addr = key->sta->sta.addr;
-
-			if (is_multicast_ether_addr(ra))
-				sta_addr = bcast;
-
-			key->local->ops->update_tkip_key(
-				local_to_hw(key->local), &key->conf,
-				sta_addr, iv32, key->u.tkip.rx[queue].p1k);
-		}
+	}
+	/* initialized == 2 means we already call update_tkip_key */
+	if (key->local->ops->update_tkip_key &&
+		key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
+		key->u.tkip.rx[queue].initialized != 2) {
+		u8 bcast[ETH_ALEN] =
+			{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+		u8 *sta_addr = key->sta->sta.addr;
+
+		if (is_multicast_ether_addr(ra))
+			sta_addr = bcast;
+
+		key->local->ops->update_tkip_key(
+			local_to_hw(key->local), &key->conf,
+			sta_addr, iv32, key->u.tkip.rx[queue].p1k);
+		key->u.tkip.rx[queue].initialized = 2;
 	}

 	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Tue, 2009-06-09 at 19:48 +0200, gregor kowski wrote:

> > Right. But drivers are free to even only _encrypt_ tkip frames and never
> > _decrypt_ them after having accepted a hardware key, iow that is
> > perfectly valid behaviour and I don't think we should keep uploading the
> > key to the driver. Worst case is that the proper upload fails and we
> > decrypt all frames in software until the next rollover.
> >
> What's the point of setting the tkip callback if we aren't interested
> in decrypting data by hardware ?

Might depend on something else? Anyhow I don't see the point of
continuing to call the callback. Maybe it should just be part of the key
todo instead when the key is initially uploaded to the hw.

johannes

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

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

On Tue, Jun 9, 2009 at 4:02 PM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Mon, 2009-06-08 at 19:51 +0200, gregor kowski wrote:
>
>> > There's a quite obvious disconnect between what your patch does and what
>> > your description says, please fix one of them. As it is, the patch only
>> > skips the IV rollover which is *completely* wrong because it will call
>> > the function for *every* packet.
>
>> I don't understand what you mean : the callback will be called for
>> every packet the hardware doesn't decrypted. If the hardware decrypt
>> the packet, only_iv is set and we don't go here.
>
> Right. But drivers are free to even only _encrypt_ tkip frames and never
> _decrypt_ them after having accepted a hardware key, iow that is
> perfectly valid behaviour and I don't think we should keep uploading the
> key to the driver. Worst case is that the proper upload fails and we
> decrypt all frames in software until the next rollover.
>
What's the point of setting the tkip callback if we aren't interested
in decrypting data by hardware ?

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Mon, 2009-06-08 at 19:51 +0200, gregor kowski wrote:

> > There's a quite obvious disconnect between what your patch does and what
> > your description says, please fix one of them. As it is, the patch only
> > skips the IV rollover which is *completely* wrong because it will call
> > the function for *every* packet.

> I don't understand what you mean : the callback will be called for
> every packet the hardware doesn't decrypted. If the hardware decrypt
> the packet, only_iv is set and we don't go here.

Right. But drivers are free to even only _encrypt_ tkip frames and never
_decrypt_ them after having accepted a hardware key, iow that is
perfectly valid behaviour and I don't think we should keep uploading the
key to the driver. Worst case is that the proper upload fails and we
decrypt all frames in software until the next rollover.

johannes

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

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

On Mon, Jun 8, 2009 at 8:29 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Sun, 2009-06-07 at 21:49 +0000, gregor kowski wrote:
>> The mac80211 tkip code won't call update_tkip_key, if some rx packets
>> get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
>> first packet because the hardware key stuff is called asynchronously
>> with
>> todo workqueue.
>>
>> This patch workaround that by always calling update_tkip_key if
>> the packet wasn't decrypted by the hardware.
>>
>> Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>
>> Index: linux-2.6/net/mac80211/tkip.c
>> ===================================================================
>> --- linux-2.6.orig/net/mac80211/tkip.c  2009-06-07 19:32:26.000000000
>> +0000
>> +++ linux-2.6/net/mac80211/tkip.c       2009-06-07 21:31:31.000000000
>> +0000
>> @@ -298,19 +298,19 @@
>>                         printk("\n");
>>                 }
>>  #endif
>> -               if (key->local->ops->update_tkip_key &&
>> -                       key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
>> -                       u8 bcast[ETH_ALEN] =
>> -                               {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
>> -                       u8 *sta_addr = key->sta->sta.addr;
>> +       }
>> +       if (key->local->ops->update_tkip_key &&
>> +               key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
>> +               u8 bcast[ETH_ALEN] =
>> +                       {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
>> +               u8 *sta_addr = key->sta->sta.addr;
>
> There's a quite obvious disconnect between what your patch does and what
> your description says, please fix one of them. As it is, the patch only
> skips the IV rollover which is *completely* wrong because it will call
> the function for *every* packet.
I don't understand what you mean : the callback will be called for
every packet the hardware doesn't decrypted. If the hardware decrypt
the packet, only_iv is set and we don't go here.


Gregor

Re: [PATCH] mac80211 : fix a race with update_tkip_key

[Johannes Berg]

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

On Sun, 2009-06-07 at 21:49 +0000, gregor kowski wrote:
> The mac80211 tkip code won't call update_tkip_key, if some rx packets
> get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
> first packet because the hardware key stuff is called asynchronously
> with 
> todo workqueue.
> 
> This patch workaround that by always calling update_tkip_key if
> the packet wasn't decrypted by the hardware.
> 
> Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>
> Index: linux-2.6/net/mac80211/tkip.c
> ===================================================================
> --- linux-2.6.orig/net/mac80211/tkip.c  2009-06-07 19:32:26.000000000
> +0000
> +++ linux-2.6/net/mac80211/tkip.c       2009-06-07 21:31:31.000000000
> +0000
> @@ -298,19 +298,19 @@
>                         printk("\n");
>                 }
>  #endif
> -               if (key->local->ops->update_tkip_key &&
> -                       key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> -                       u8 bcast[ETH_ALEN] =
> -                               {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> -                       u8 *sta_addr = key->sta->sta.addr;
> +       }
> +       if (key->local->ops->update_tkip_key &&
> +               key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> +               u8 bcast[ETH_ALEN] =
> +                       {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> +               u8 *sta_addr = key->sta->sta.addr;

There's a quite obvious disconnect between what your patch does and what
your description says, please fix one of them. As it is, the patch only
skips the IV rollover which is *completely* wrong because it will call
the function for *every* packet.

johannes

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

[PATCH] mac80211 : fix a race with update_tkip_key

[gregor kowski]

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



[-- Attachment #2: tkip_core.diff --]
[-- Type: text/x-diff, Size: 1552 bytes --]

The mac80211 tkip code won't call update_tkip_key, if some rx packets
get received without KEY_FLAG_UPLOADED_TO_HARDWARE. This can happen on
first packet because the hardware key stuff is called asynchronously with 
todo workqueue.

This patch workaround that by always calling update_tkip_key if
the packet wasn't decrypted by the hardware.

Signed-off-by: Gregor Kowski <gregor.kowski@gmail.com>
Index: linux-2.6/net/mac80211/tkip.c
===================================================================
--- linux-2.6.orig/net/mac80211/tkip.c	2009-06-07 19:32:26.000000000 +0000
+++ linux-2.6/net/mac80211/tkip.c	2009-06-07 21:31:31.000000000 +0000
@@ -298,19 +298,19 @@
 			printk("\n");
 		}
 #endif
-		if (key->local->ops->update_tkip_key &&
-			key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
-			u8 bcast[ETH_ALEN] =
-				{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-			u8 *sta_addr = key->sta->sta.addr;
+	}
+	if (key->local->ops->update_tkip_key &&
+		key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
+		u8 bcast[ETH_ALEN] =
+			{0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+		u8 *sta_addr = key->sta->sta.addr;
 
-			if (is_multicast_ether_addr(ra))
-				sta_addr = bcast;
+		if (is_multicast_ether_addr(ra))
+			sta_addr = bcast;
 
-			key->local->ops->update_tkip_key(
-				local_to_hw(key->local), &key->conf,
-				sta_addr, iv32, key->u.tkip.rx[queue].p1k);
-		}
+		key->local->ops->update_tkip_key(
+			local_to_hw(key->local), &key->conf,
+			sta_addr, iv32, key->u.tkip.rx[queue].p1k);
 	}
 
 	tkip_mixing_phase2(tk, &key->u.tkip.rx[queue], iv16, rc4key);