ath5k: Fix loop variable initializations

PATCH In review Bob Copeland 3 participants 6 messages May 12, 2008 – May 13, 2008 Networking drivers (wireless)

Share & tools

Bob Copeland

author

2008-05-12 15:04 UTC #0 lore raw

PATCH 1/1 ath5k: Fix loop variable initializations

From: Bob Copeland <[email protected]>
Date: Mon, 12 May 2008 09:25:33 -0400
Subject: [PATCH] ath5k: Fix loop variable initializations

In ath5k_tasklet_rx, both status structures 'rxs' and 'rs' are initialized at the top of the tasklet, but not within the loop. If the loop is executed multiple times in the tasklet, then the variables may see changes from previous packets.

For TKIP, this results in 'Invalid Michael MIC' errors if two packets are processed in the tasklet: rxs.flag gets set to RX_DECRYPTED by mac80211 when it decrypts the first encrypted packet. The subsequent packet will have RX_DECRYPTED set upon entry to mac80211, so mac80211 will not try to decrypt it.

We currently initialize all but two fields in the structures, so fix the other two.

Signed-off-by Bob Copeland <[email protected]>

---
 drivers/net/wireless/ath5k/base.c |    2 +-
 drivers/net/wireless/ath5k/hw.c   |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

drivers/net/wireless/ath5k/base.c +1 -1

index 4e5c8fc..fd8537e 100644
@@ -1888,7 +1888,7 @@ accept:
 		 * right now, so it's not too bad...
 		 */
 		rxs.mactime = ath5k_extend_tsf(sc->ah, rs.rs_tstamp);
-		rxs.flag |= RX_FLAG_TSFT;
+		rxs.flag = RX_FLAG_TSFT;
 
 		rxs.freq = sc->curchan->center_freq;
 		rxs.band = sc->curband->band;

drivers/net/wireless/ath5k/hw.c +2 -0

index 5fb1ae6..8137d82 100644
@@ -4119,6 +4119,7 @@ static int ath5k_hw_proc_5210_rx_status(struct ath5k_hw *ah,
 	rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1,
 		AR5K_5210_RX_DESC_STATUS1_RECEIVE_TIMESTAMP);
 	rs->rs_status = 0;
+	rs->rs_phyerr = 0;
 
 	/*
 	 * Key table status

@@ -4193,6 +4194,7 @@ static int ath5k_hw_proc_5212_rx_status(struct ath5k_hw *ah,
 	rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1,
 		AR5K_5212_RX_DESC_STATUS1_RECEIVE_TIMESTAMP);
 	rs->rs_status = 0;
+	rs->rs_phyerr = 0;
 
 	/*
 	 * Key table status

Signature · 4 lines

1.5.4.2.182.gb3092

-- 
Bob Copeland %% www.bobcopeland.com

Helge Deller

2008-05-12 20:40 UTC #1 lore raw

Thanks Bob!

This fixes http://bugzilla.kernel.org/show_bug.cgi?id=10605

Helge

Quoted 59 lines · Bob Copeland

From: Bob Copeland <[email protected]>
Date: Mon, 12 May 2008 09:25:33 -0400
Subject: [PATCH] ath5k: Fix loop variable initializations
In ath5k_tasklet_rx, both status structures 'rxs' and 'rs' are initialized at the top of the tasklet, but not within the loop. If the loop is executed multiple times in the tasklet, then the variables may see changes from previous packets.
For TKIP, this results in 'Invalid Michael MIC' errors if two packets are processed in the tasklet: rxs.flag gets set to RX_DECRYPTED by mac80211 when it decrypts the first encrypted packet. The subsequent packet will have RX_DECRYPTED set upon entry to mac80211, so mac80211 will not try to decrypt it.
We currently initialize all but two fields in the structures, so fix the other two.
Signed-off-by Bob Copeland <[email protected]>
---
 drivers/net/wireless/ath5k/base.c |    2 +-
 drivers/net/wireless/ath5k/hw.c   |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)
(quoted diff) +0 -0
b/drivers/net/wireless/ath5k/base.c index 4e5c8fc..fd8537e 100644
drivers/net/wireless/ath5k/base.c +1 -1
@@ -1888,7 +1888,7 @@ accept:
 * right now, so it's not too bad...
 */
 rxs.mactime = ath5k_extend_tsf(sc->ah, rs.rs_tstamp);
-             rxs.flag |= RX_FLAG_TSFT;
+             rxs.flag = RX_FLAG_TSFT;
 
 rxs.freq = sc->curchan->center_freq;
 rxs.band = sc->curband->band;
(quoted diff) +0 -0
b/drivers/net/wireless/ath5k/hw.c index 5fb1ae6..8137d82 100644
drivers/net/wireless/ath5k/hw.c +1 -0
@@ -4119,6 +4119,7 @@ static int ath5k_hw_proc_5210_rx_status(struct
 ath5k_hw *ah,
 rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1,
 AR5K_5210_RX_DESC_STATUS1_RECEIVE_TIMESTAMP);
 rs->rs_status = 0;
+     rs->rs_phyerr = 0;
 
 /*
* Key table status
drivers/net/wireless/ath5k/hw.c +1 -0
@@ -4193,6 +4194,7 @@ static int ath5k_hw_proc_5212_rx_status(struct
 ath5k_hw *ah,
 rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1,
 AR5K_5212_RX_DESC_STATUS1_RECEIVE_TIMESTAMP);
 rs->rs_status = 0;
+     rs->rs_phyerr = 0;
 
 /*
* Key table status

Nick Kossifidis

2008-05-12 23:18 UTC #2 lore raw

In reply to Bob Copeland

Have you checked this one...

1838 if (rs.rs_status & AR5K_RXERR_MIC) {
1839 rxs.flag |= RX_FLAG_MMIC_ERROR;
1840 goto accept;

are you sure that doesn't happen ?

Also why initialize rxs.flag here?

Quoted 1 line

rxs.flag = RX_FLAG_TSFT;

Why not doing a simple
rxs.flag = 0 ;
or a check of flags set by mac80211 at the start of the loop ?

This way we'll also have a clean rxs.flag when doing rxs.flag |= RX_FLAG_MMIC_ERROR
and we won't clear RX_FLAG_MMIC_ERROR when doing rxs.flag = RX_FLAG_TSFT;

Signature · 3 lines

GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Bob Copeland

author

2008-05-13 00:17 UTC #3 lore raw

Quoted 3 lines · Nick Kossifidis

Why not doing a simple
rxs.flag = 0 ;
or a check of flags set by mac80211 at the start of the loop ?

Yeah that's a better idea. I'll respin the patch with this and the other suggestion.

Signature · 1 line

Bob Copeland %% www.bobcopeland.com

Bob Copeland

author

2008-05-13 01:16 UTC #4 lore raw

Quoted 4 lines · Bob Copeland

Quoted 3 lines · Nick Kossifidis
Why not doing a simple
rxs.flag = 0 ;
or a check of flags set by mac80211 at the start of the loop ?

Here's an updated version:

In ath5k_tasklet_rx, both status structures 'rxs' and 'rs' are initialized at the top of the tasklet, but not within the loop. If the loop is executed multiple times in the tasklet then the variables may see changes from previous packets.

We currently initialize all but two fields in the structures, so fix the other two.

Signed-off-by Bob Copeland <[email protected]>

---
 drivers/net/wireless/ath5k/base.c |    2 ++
 drivers/net/wireless/ath5k/hw.c   |    6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

drivers/net/wireless/ath5k/base.c +2 -0

index 4e5c8fc..635b9ac 100644
@@ -1787,6 +1787,8 @@ ath5k_tasklet_rx(unsigned long data)
 
 	spin_lock(&sc->rxbuflock);
 	do {
+		rxs.flag = 0;
+
 		if (unlikely(list_empty(&sc->rxbuf))) {
 			ATH5K_WARN(sc, "empty rx buf pool\n");
 			break;

drivers/net/wireless/ath5k/hw.c +4 -2

index 5fb1ae6..77990b5 100644
@@ -4119,6 +4119,7 @@ static int ath5k_hw_proc_5210_rx_status(struct ath5k_hw *ah,
 	rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1,
 		AR5K_5210_RX_DESC_STATUS1_RECEIVE_TIMESTAMP);
 	rs->rs_status = 0;
+	rs->rs_phyerr = 0;
 
 	/*
 	 * Key table status

@@ -4145,7 +4146,7 @@ static int ath5k_hw_proc_5210_rx_status(struct ath5k_hw *ah,
 		if (rx_status->rx_status_1 &
 				AR5K_5210_RX_DESC_STATUS1_PHY_ERROR) {
 			rs->rs_status |= AR5K_RXERR_PHY;
-			rs->rs_phyerr = AR5K_REG_MS(rx_status->rx_status_1,
+			rs->rs_phyerr |= AR5K_REG_MS(rx_status->rx_status_1,
 					   AR5K_5210_RX_DESC_STATUS1_PHY_ERROR);
 		}

@@ -4193,6 +4194,7 @@ static int ath5k_hw_proc_5212_rx_status(struct ath5k_hw *ah,
 	rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1,
 		AR5K_5212_RX_DESC_STATUS1_RECEIVE_TIMESTAMP);
 	rs->rs_status = 0;
+	rs->rs_phyerr = 0;
 
 	/*
 	 * Key table status

@@ -4215,7 +4217,7 @@ static int ath5k_hw_proc_5212_rx_status(struct ath5k_hw *ah,
 		if (rx_status->rx_status_1 &
 				AR5K_5212_RX_DESC_STATUS1_PHY_ERROR) {
 			rs->rs_status |= AR5K_RXERR_PHY;
-			rs->rs_phyerr = AR5K_REG_MS(rx_err->rx_error_1,
+			rs->rs_phyerr |= AR5K_REG_MS(rx_err->rx_error_1,
 					   AR5K_RX_DESC_ERROR1_PHY_ERROR_CODE);
 		}

Signature · 5 lines

1.5.4.2.182.gb3092


-- 
Bob Copeland %% www.bobcopeland.com

Nick Kossifidis

2008-05-12 23:25 UTC #5 lore raw

In reply to Bob Copeland

Quoted 2 lines

+ rs->rs_phyerr = 0;

Do you mind also changing these to |= instead of = (just for code clarity) ?

rs->rs_phyerr = AR5K_REG_MS(rx_status->rx_status_1, AR5K_5210_RX_DESC_STATUS1_PHY_ERROR);

rs->rs_phyerr = AR5K_REG_MS(rx_err->rx_error_1,
AR5K_RX_DESC_ERROR1_PHY_ERROR_CODE);

Signature · 3 lines

GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick