Skip to contentSkip to conversation

ath9k: Prevent read buffer overflow

PATCH New Roel Kluin 2 participants 4 messages Aug 11, 2009 – Aug 12, 2009 Qualcomm Atheros ATH9K wireless driver

lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 ath9k: Prevent read buffer overflow

Prevent a read from valid_rate_index[] with a negative index

Signed-off-by Roel Kluin <[email protected]>

---
Maybe we should add this?

drivers/net/wireless/ath/ath9k/rc.c +1 -1
index ba06e78..a67b7f6 100644
@@ -1458,7 +1458,7 @@ static void ath_rc_init(struct ath_softc *sc,
 		ath_rc_priv->rate_max_phy = ath_rc_priv->valid_phy_rateidx[i][j-1];
 	}
 	ASSERT(ath_rc_priv->rate_table_size <= RATE_TABLE_SIZE);
-	ASSERT(k <= RATE_TABLE_SIZE);
+	ASSERT(k <= RATE_TABLE_SIZE && k >= 4);
 
 	ath_rc_priv->max_valid_rate = k;
 	ath_rc_sort_validrates(rate_table, ath_rc_priv);
NAK
#1 lore raw

PATCH 1/1 ath9k: Prevent read buffer overflow

Quoted 11 lines · Roel Kluin

Prevent a read from valid_rate_index[] with a negative index

Signed-off-by Roel Kluin <[email protected]>

---
Maybe we should add this?

drivers/net/wireless/ath/ath9k/rc.c +0 -0
index ba06e78..a67b7f6 100644

drivers/net/wireless/ath/ath9k/rc.c static void ath_rc_init(struct ath_softc *sc, · line 1461

Quoted 5 lines
drivers/net/wireless/ath/ath9k/rc.c +1 -1
@@ -1458,7 +1458,7 @@ static void ath_rc_init(struct ath_softc *sc,
                ath_rc_priv->rate_max_phy = ath_rc_priv->valid_phy_rateidx[i][j-1];
        }
        ASSERT(ath_rc_priv->rate_table_size <= RATE_TABLE_SIZE);
-       ASSERT(k <= RATE_TABLE_SIZE);
+       ASSERT(k <= RATE_TABLE_SIZE && k >= 4);

NACK, k is initialized to 0 in the for loop few lines above this ASSERT.

Vasanth

author
#2 lore raw

PATCH 1/1 ath9k: Prevent read buffer overflow

Quoted 22 lines · Vasanthakumar Thiagarajan
Quoted 11 lines · Roel Kluin

Prevent a read from valid_rate_index[] with a negative index

Signed-off-by Roel Kluin <[email protected]>

---
Maybe we should add this?

drivers/net/wireless/ath/ath9k/rc.c +0 -0
index ba06e78..a67b7f6 100644

drivers/net/wireless/ath/ath9k/rc.c static void ath_rc_init(struct ath_softc *sc, · line 1461

Quoted 5 lines
drivers/net/wireless/ath/ath9k/rc.c +1 -1
@@ -1458,7 +1458,7 @@ static void ath_rc_init(struct ath_softc *sc,
 ? ? ? ? ? ? ? ? ath_rc_priv->rate_max_phy = ath_rc_priv->valid_phy_rateidx[i][j-1];
 ? ? ? ? }
 ? ? ? ? ASSERT(ath_rc_priv->rate_table_size <= RATE_TABLE_SIZE);
- ? ? ? ASSERT(k <= RATE_TABLE_SIZE);
+ ? ? ? ASSERT(k <= RATE_TABLE_SIZE && k >= 4);

NACK, k is initialized to 0 in the for loop few lines above this ASSERT.

Quoted 1 line

Vasanth

You could be right, but please explain, I don't understand:

k can only increment if ath_rc_priv->valid_phy_ratecnt[i] != 0 for i = 0 to WLAN_RC_PHY_MAX,

A few lines above these `ath_rc_priv->valid_phy_ratecnt[]' are initialized to 0.

Say there was no working rate, and we call ath_rc_init_validrates(),

then in ath_rc_init_validrates() ath_rc_priv->valid_phy_ratecnt[] can be initialized in this loop:

for (i = 0; i < rate_table->rate_cnt; i++) {
    ...
}

but where is this rate_cnt initialized?

[roel at zoinx linux-git]$ git grep rate_cnt drivers/net/wireless/ath/ath9k/debug.c: max = 80 + sc->cur_rate_table->rate_cnt * 64; drivers/net/wireless/ath/ath9k/debug.c: for (i = 0; i < sc->cur_rate_table->rate_cnt; i++) { drivers/net/wireless/ath/ath9k/main.c: if (rate_table->rate_cnt > ATH_RATE_MAX)
drivers/net/wireless/ath/ath9k/main.c: maxrates = rate_table->rate_cnt;
drivers/net/wireless/ath/ath9k/rc.c: for (i = 0; i <
rate_table->rate_cnt; i++) {
drivers/net/wireless/ath/ath9k/rc.c: for (j = 0; j <
rate_table->rate_cnt; j++) {
drivers/net/wireless/ath/ath9k/rc.c: for (j = 0; j <
rate_table->rate_cnt; j++) {
drivers/net/wireless/ath/ath9k/rc.c: if ((tx_rate < 0) || (tx_rate

Quoted 1 line

rate_table->rate_cnt))

drivers/net/wireless/ath/ath9k/rc.h: int rate_cnt;

Roel

#3 lore raw

PATCH 1/1 ath9k: Prevent read buffer overflow

Quoted 24 lines · roel kluin
Quoted 22 lines · Vasanthakumar Thiagarajan
Quoted 11 lines · Roel Kluin

Prevent a read from valid_rate_index[] with a negative index

Signed-off-by Roel Kluin <[email protected]>

---
Maybe we should add this?

drivers/net/wireless/ath/ath9k/rc.c +0 -0
index ba06e78..a67b7f6 100644

drivers/net/wireless/ath/ath9k/rc.c static void ath_rc_init(struct ath_softc *sc, · line 1461

Quoted 5 lines
drivers/net/wireless/ath/ath9k/rc.c +1 -1
@@ -1458,7 +1458,7 @@ static void ath_rc_init(struct ath_softc *sc,
                ath_rc_priv->rate_max_phy = ath_rc_priv->valid_phy_rateidx[i][j-1];
        }
        ASSERT(ath_rc_priv->rate_table_size <= RATE_TABLE_SIZE);
-       ASSERT(k <= RATE_TABLE_SIZE);
+       ASSERT(k <= RATE_TABLE_SIZE && k >= 4);

NACK, k is initialized to 0 in the for loop few lines above this ASSERT.

Quoted 3 lines
Quoted 1 line

Vasanth

but where is this rate_cnt initialized?

from the static rate table for the respective mode. You can find these tables in the begining of rc.c.

Vasanth