2009-08-11 18:36:37

by Roel Kluin

Subject: [PATCH] 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?

diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index ba06e78..a67b7f6 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -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);

2009-08-12 12:45:05

by Vasanthakumar Thiagarajan

Subject: Re: [PATCH] ath9k: Prevent read buffer overflow

On Wed, Aug 12, 2009 at 06:05:55PM +0530, roel kluin wrote:
> On Wed, Aug 12, 2009 at 1:58 PM, Vasanthakumar
> Thiagarajan<[email protected]> wrote:
> > On Wed, Aug 12, 2009 at 12:10:30AM +0530, Roel Kluin wrote:
> >> Prevent a read from valid_rate_index[] with a negative index
> >>
> >> Signed-off-by: Roel Kluin <[email protected]>
> >> ---
> >> Maybe we should add this?
> >>
> >> diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
> >> index ba06e78..a67b7f6 100644
> >> --- a/drivers/net/wireless/ath/ath9k/rc.c
> >> +++ b/drivers/net/wireless/ath/ath9k/rc.c
> >> @@ -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
>
> 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

2009-08-12 12:35:55

by Roel Kluin

Subject: Re: [PATCH] ath9k: Prevent read buffer overflow

On Wed, Aug 12, 2009 at 1:58 PM, Vasanthakumar
Thiagarajan<[email protected]> wrote:
> On Wed, Aug 12, 2009 at 12:10:30AM +0530, Roel Kluin wrote:
>> Prevent a read from valid_rate_index[] with a negative index
>>
>> Signed-off-by: Roel Kluin <[email protected]>
>> ---
>> Maybe we should add this?
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
>> index ba06e78..a67b7f6 100644
>> --- a/drivers/net/wireless/ath/ath9k/rc.c
>> +++ b/drivers/net/wireless/ath/ath9k/rc.c
>> @@ -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

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@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
> rate_table->rate_cnt))
drivers/net/wireless/ath/ath9k/rc.h: int rate_cnt;

Roel

2009-08-12 12:00:18

by Vasanthakumar Thiagarajan

Subject: Re: [PATCH] ath9k: Prevent read buffer overflow

On Wed, Aug 12, 2009 at 12:10:30AM +0530, Roel Kluin wrote:
> Prevent a read from valid_rate_index[] with a negative index
>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> Maybe we should add this?
>
> diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
> index ba06e78..a67b7f6 100644
> --- a/drivers/net/wireless/ath/ath9k/rc.c
> +++ b/drivers/net/wireless/ath/ath9k/rc.c
> @@ -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