[ath9k-devel] [PATCH] ath9k: stop on rates with idx -1 in ath9k rate control's .tx_status

[ath9k-devel] [PATCH] ath9k: stop on rates with idx -1 in ath9k rate control's .tx_status

[Pavel Roskin]

Rate control algorithms are supposed to stop processing when they
encounter a rate with the index -1.  Checking for rate->count not being
zero is not enough.

Allowing a rate with negative index leads to memory corruption in
ath_debug_stat_rc().

One consequence of the bug is discussed at
https://bugzilla.redhat.com/show_bug.cgi?id=768639

Signed-off-by: Pavel Roskin <proski@gnu.org>
Cc: stable at vger.kernel.org
---

 drivers/net/wireless/ath/ath9k/rc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 635b592..a427a16 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -1346,7 +1346,7 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
 	fc = hdr->frame_control;
 	for (i = 0; i < sc->hw->max_rates; i++) {
 		struct ieee80211_tx_rate *rate = &tx_info->status.rates[i];
-		if (!rate->count)
+		if (rate->idx < 0 || !rate->count)
 			break;
 
 		final_ts_idx = i;

[PATCH] ath9k: stop on rates with idx -1 in ath9k rate control's .tx_status

[Pavel Roskin]

Rate control algorithms are supposed to stop processing when they
encounter a rate with the index -1.  Checking for rate->count not being
zero is not enough.

Allowing a rate with negative index leads to memory corruption in
ath_debug_stat_rc().

One consequence of the bug is discussed at
https://bugzilla.redhat.com/show_bug.cgi?id=768639

Signed-off-by: Pavel Roskin <proski@gnu.org>
Cc: stable@vger.kernel.org
---

 drivers/net/wireless/ath/ath9k/rc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 635b592..a427a16 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -1346,7 +1346,7 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
 	fc = hdr->frame_control;
 	for (i = 0; i < sc->hw->max_rates; i++) {
 		struct ieee80211_tx_rate *rate = &tx_info->status.rates[i];
-		if (!rate->count)
+		if (rate->idx < 0 || !rate->count)
 			break;
 
 		final_ts_idx = i;

[ath9k-devel] [PATCH] ath9k: stop on rates with idx -1 in ath9k rate control's .tx_status

[Peter Stuge]

Pavel Roskin wrote:
> Rate control algorithms are supposed to stop processing when they
> encounter a rate with the index -1.  Checking for rate->count not being
> zero is not enough.

Maybe it's a good idea to use an unsigned variable and check for the
actual error condition when it can happen, instead of checking for an
"overflow" after the fact?


> +++ b/drivers/net/wireless/ath/ath9k/rc.c
> @@ -1346,7 +1346,7 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
>  	fc = hdr->frame_control;
>  	for (i = 0; i < sc->hw->max_rates; i++) {
>  		struct ieee80211_tx_rate *rate = &tx_info->status.rates[i];
> -		if (!rate->count)
> +		if (rate->idx < 0 || !rate->count)
>  			break;

At the very least this could test for <= 0.


//Peter

Re: [ath9k-devel] [PATCH] ath9k: stop on rates with idx -1 in ath9k rate control's .tx_status

[Peter Stuge]

Pavel Roskin wrote:
> Rate control algorithms are supposed to stop processing when they
> encounter a rate with the index -1.  Checking for rate->count not being
> zero is not enough.

Maybe it's a good idea to use an unsigned variable and check for the
actual error condition when it can happen, instead of checking for an
"overflow" after the fact?


> +++ b/drivers/net/wireless/ath/ath9k/rc.c
> @@ -1346,7 +1346,7 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
>  	fc = hdr->frame_control;
>  	for (i = 0; i < sc->hw->max_rates; i++) {
>  		struct ieee80211_tx_rate *rate = &tx_info->status.rates[i];
> -		if (!rate->count)
> +		if (rate->idx < 0 || !rate->count)
>  			break;

At the very least this could test for <= 0.


//Peter

[ath9k-devel] [PATCH] ath9k: stop on rates with idx -1 in ath9k rate control's .tx_status

[Pavel Roskin]

Quoting Peter Stuge <peter@stuge.se>:

> Pavel Roskin wrote:
>> Rate control algorithms are supposed to stop processing when they
>> encounter a rate with the index -1.  Checking for rate->count not being
>> zero is not enough.
>
> Maybe it's a good idea to use an unsigned variable and check for the
> actual error condition when it can happen, instead of checking for an
> "overflow" after the fact?

It's not my choice, it's written in include/net/mac80211.h:

  * A value of -1 for @idx indicates an invalid rate and, if used
  * in an array of retry rates, that no more rates should be tried.

I just want ath9k/rc to follow the documentation.

I believe the code in mac80211 uses both count=0 and idx=-1 to  
indicate the end on the rate list, but in some cases, only idx=-1 is  
used (my guess is that the list needs to be shortened).  In any case,  
mac80211 is must be correct because it expect rate control algorithms  
to do what's documented in the header.

>> -		if (!rate->count)
>> +		if (rate->idx < 0 || !rate->count)
>>  			break;
>
> At the very least this could test for <= 0.

Index 0 may be valid.  Other rate control algorithms only check for  
negative values.

-- 
Regards,
Pavel Roskin

Re: [ath9k-devel] [PATCH] ath9k: stop on rates with idx -1 in ath9k rate control's .tx_status

[Pavel Roskin]

Quoting Peter Stuge <peter@stuge.se>:

> Pavel Roskin wrote:
>> Rate control algorithms are supposed to stop processing when they
>> encounter a rate with the index -1.  Checking for rate->count not being
>> zero is not enough.
>
> Maybe it's a good idea to use an unsigned variable and check for the
> actual error condition when it can happen, instead of checking for an
> "overflow" after the fact?

It's not my choice, it's written in include/net/mac80211.h:

  * A value of -1 for @idx indicates an invalid rate and, if used
  * in an array of retry rates, that no more rates should be tried.

I just want ath9k/rc to follow the documentation.

I believe the code in mac80211 uses both count=0 and idx=-1 to  
indicate the end on the rate list, but in some cases, only idx=-1 is  
used (my guess is that the list needs to be shortened).  In any case,  
mac80211 is must be correct because it expect rate control algorithms  
to do what's documented in the header.

>> -		if (!rate->count)
>> +		if (rate->idx < 0 || !rate->count)
>>  			break;
>
> At the very least this could test for <= 0.

Index 0 may be valid.  Other rate control algorithms only check for  
negative values.

-- 
Regards,
Pavel Roskin

[ath9k-devel] [PATCH] ath9k: stop on rates with idx -1 in ath9k rate control's .tx_status

[Adrian Chadd]

Hi!

On 11 February 2012 07:01, Pavel Roskin <proski@gnu.org> wrote:
> Rate control algorithms are supposed to stop processing when they
> encounter a rate with the index -1. ?Checking for rate->count not being
> zero is not enough.
>
> Allowing a rate with negative index leads to memory corruption in
> ath_debug_stat_rc().
>
> One consequence of the bug is discussed at
> https://bugzilla.redhat.com/show_bug.cgi?id=768639
>
> Signed-off-by: Pavel Roskin <proski@gnu.org>
> Cc: stable at vger.kernel.org

Great catch!



Adrian

Re: [PATCH] ath9k: stop on rates with idx -1 in ath9k rate control's .tx_status

[Adrian Chadd]

Hi!

On 11 February 2012 07:01, Pavel Roskin <proski@gnu.org> wrote:
> Rate control algorithms are supposed to stop processing when they
> encounter a rate with the index -1.  Checking for rate->count not being
> zero is not enough.
>
> Allowing a rate with negative index leads to memory corruption in
> ath_debug_stat_rc().
>
> One consequence of the bug is discussed at
> https://bugzilla.redhat.com/show_bug.cgi?id=768639
>
> Signed-off-by: Pavel Roskin <proski@gnu.org>
> Cc: stable@vger.kernel.org

Great catch!



Adrian