[PATCH 1/1] ath5k: fix hw rate index condition

[PATCH 1/1] ath5k: fix hw rate index condition

[Jiri Slaby]

Make sure we print out a warning when the index is out of bounds,
i.e. even on hw_rix == AR5K_MAX_RATES.

Also change to WARN and print text with the reported hw_rix.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Nick Kossifidis <mickflemm@gmail.com>
Cc: Luis R. Rodriguez <lrodriguez@atheros.com>
Cc: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath5k/base.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index c06d1cc..08d691d 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1100,7 +1100,8 @@ ath5k_mode_setup(struct ath5k_softc *sc)
 static inline int
 ath5k_hw_to_driver_rix(struct ath5k_softc *sc, int hw_rix)
 {
-	WARN_ON(hw_rix < 0 || hw_rix > AR5K_MAX_RATES);
+	WARN(hw_rix < 0 || hw_rix >= AR5K_MAX_RATES,
+			"hw_rix out of bounds: %x\n", hw_rix);
 	return sc->rate_idx[sc->curband->band][hw_rix];
 }
 
-- 
1.6.1.3

Re: [PATCH 1/1] ath5k: fix hw rate index condition

[Bob Copeland]

On Thu, 26 Feb 2009 23:44:31 +0100, Jiri Slaby wrote
> Make sure we print out a warning when the index is out of bounds,
> i.e. even on hw_rix == AR5K_MAX_RATES.
> 
> Also change to WARN and print text with the reported hw_rix.

ACK.

Speaking of, I think there's another potential oob array access at:

if (rxs.rate_idx >= 0 && rs.rs_rate == 
    sc->curband->bitrates[rxs.rate_idx].hw_value_short)
        rxs.flag |= RX_FLAG_SHORTPRE;

because sc->rate_idx is u8 instead of s8.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 1/1] ath5k: fix hw rate index condition

[Jiri Slaby]

On 27.2.2009 00:15, Bob Copeland wrote:
> Speaking of, I think there's another potential oob array access at:
>
> if (rxs.rate_idx>= 0&&  rs.rs_rate ==
>      sc->curband->bitrates[rxs.rate_idx].hw_value_short)
>          rxs.flag |= RX_FLAG_SHORTPRE;
>
> because sc->rate_idx is u8 instead of s8.

strcmp("sc->rate_idx", "rxs.rate_idx") != 0 :)

Or did I miss something?

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Bob Copeland]

On Thu, Feb 26, 2009 at 6:19 PM, Jiri Slaby <jirislaby@gmail.com> wrote:
> On 27.2.2009 00:15, Bob Copeland wrote:
>> Speaking of, I think there's another potential oob array access at:
>>
>> if (rxs.rate_idx>= 0&&  rs.rs_rate ==
>>      sc->curband->bitrates[rxs.rate_idx].hw_value_short)
>>          rxs.flag |= RX_FLAG_SHORTPRE;
>>
>> because sc->rate_idx is u8 instead of s8.
>
> strcmp("sc->rate_idx", "rxs.rate_idx") != 0 :)
>
> Or did I miss something?

:)  Sorry, I should've been clearer.

hw_to_driver_rix() returns sc->rate_idx[x][y] as an int, and that
array is initialized to (u8)-1 for invalid rates.  So, it can
return 255 if the hardware rate index (y) is bad, then the check
"rxs.rate_idx >= 0" would always be true, right?  If it's not a
real bug yet, it likely will be one day :)

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Jiri Slaby]

On 27.2.2009 00:28, Bob Copeland wrote:
> hw_to_driver_rix() returns sc->rate_idx[x][y] as an int, and that
> array is initialized to (u8)-1 for invalid rates.  So, it can
> return 255 if the hardware rate index (y) is bad, then the check
> "rxs.rate_idx>= 0" would always be true, right?  If it's not a
> real bug yet, it likely will be one day :)

Ah, yes, it really is a bug(tm), care to post a fix?

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Bob Copeland]

On Fri, Feb 27, 2009 at 12:32:55AM +0100, Jiri Slaby wrote:
> On 27.2.2009 00:28, Bob Copeland wrote:
>> hw_to_driver_rix() returns sc->rate_idx[x][y] as an int, and that
>> array is initialized to (u8)-1 for invalid rates.  So, it can
>> return 255 if the hardware rate index (y) is bad, then the check
>> "rxs.rate_idx>= 0" would always be true, right?  If it's not a
>> real bug yet, it likely will be one day :)
>
> Ah, yes, it really is a bug(tm), care to post a fix?

Actually, I remembered in the dark recesses of my moldering brain 
that someone had a lost patch for this a while ago, so I searched
the archives.  Pavel, ok to add your s-o-b?

From: Pavel Roskin <proski@gnu.org>
Subject: [PATCH] ath5k: use signed elements for rate index table

A lookup table is used to convert from hardware rate indexes back
to driver-based rate indexes.  For unknown hardware rates, we
initialize these values to -1, but since the array elements are of
type u8, they will be in the range 0-255.  This can cause array
overruns because subsequent sanity checks only check for negative
values.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath5k/base.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath5k/base.h b/drivers/net/wireless/ath5k/base.h
index 20e0d14..8229561 100644
--- a/drivers/net/wireless/ath5k/base.h
+++ b/drivers/net/wireless/ath5k/base.h
@@ -112,7 +112,7 @@ struct ath5k_softc {
 	struct ieee80211_supported_band sbands[IEEE80211_NUM_BANDS];
 	struct ieee80211_channel channels[ATH_CHAN_MAX];
 	struct ieee80211_rate	rates[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
-	u8			rate_idx[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
+	s8			rate_idx[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
 	enum nl80211_iftype	opmode;
 	struct ath5k_hw		*ah;		/* Atheros HW */
 
-- 
1.5.4.1


-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Luis R. Rodriguez]

On Thu, Feb 26, 2009 at 06:27:04PM -0800, Bob Copeland wrote:
> On Fri, Feb 27, 2009 at 12:32:55AM +0100, Jiri Slaby wrote:
> > On 27.2.2009 00:28, Bob Copeland wrote:
> >> hw_to_driver_rix() returns sc->rate_idx[x][y] as an int, and that
> >> array is initialized to (u8)-1 for invalid rates.  So, it can
> >> return 255 if the hardware rate index (y) is bad, then the check
> >> "rxs.rate_idx>= 0" would always be true, right?  If it's not a
> >> real bug yet, it likely will be one day :)
> >
> > Ah, yes, it really is a bug(tm), care to post a fix?
> 
> Actually, I remembered in the dark recesses of my moldering brain
> that someone had a lost patch for this a while ago, so I searched
> the archives.  Pavel, ok to add your s-o-b?
> 
> From: Pavel Roskin <proski@gnu.org>
> Subject: [PATCH] ath5k: use signed elements for rate index table
> 
> A lookup table is used to convert from hardware rate indexes back
> to driver-based rate indexes.  For unknown hardware rates, we
> initialize these values to -1, but since the array elements are of
> type u8, they will be in the range 0-255.  This can cause array
> overruns because subsequent sanity checks only check for negative
> values.
> 
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath5k/base.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath5k/base.h b/drivers/net/wireless/ath5k/base.h
> index 20e0d14..8229561 100644
> --- a/drivers/net/wireless/ath5k/base.h
> +++ b/drivers/net/wireless/ath5k/base.h
> @@ -112,7 +112,7 @@ struct ath5k_softc {
>         struct ieee80211_supported_band sbands[IEEE80211_NUM_BANDS];
>         struct ieee80211_channel channels[ATH_CHAN_MAX];
>         struct ieee80211_rate   rates[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
> -       u8                      rate_idx[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
> +       s8                      rate_idx[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];

Might be worth adding a note why this is the case. Can't we simply avoid
this by checking earlier for the error or simply assigning it an actual
default _good_ hw rate value?

  Luis

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Bob Copeland]

On Thu, Feb 26, 2009 at 06:39:12PM -0800, Luis R. Rodriguez wrote:
> Might be worth adding a note why this is the case. Can't we simply avoid
> this by checking earlier for the error or simply assigning it an actual
> default _good_ hw rate value?

I guess an alternative is to initialize to 0, that would count any rx
packets whose hw rate we don't know about as the base rate, so it would
probably bias the RC to 1mb, but this is already one of those 'should 
never happen' cases.  Also I can't forsee having a rate index > 127 so
changing the sign is pretty low risk.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Luis R. Rodriguez]

On Thu, Feb 26, 2009 at 07:06:08PM -0800, Bob Copeland wrote:
> On Thu, Feb 26, 2009 at 06:39:12PM -0800, Luis R. Rodriguez wrote:
> > Might be worth adding a note why this is the case. Can't we simply avoid
> > this by checking earlier for the error or simply assigning it an actual
> > default _good_ hw rate value?
> 
> I guess an alternative is to initialize to 0, that would count any rx
> packets whose hw rate we don't know about as the base rate, so it would
> probably bias the RC to 1mb, but this is already one of those 'should
> never happen' cases.

Understood.

> Also I can't forsee having a rate index > 127 so
> changing the sign is pretty low risk.

Sure, it just seems a bit strange to see a signed rate index,
that's all. And if its to deal with an error I think it may
be nicer to actually use a rate that works and then warn
rather than warn and not use a valid rate at all.

Mind you I haven't checked this code in while.

  Luis

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Pavel Roskin]

On Thu, 2009-02-26 at 21:27 -0500, Bob Copeland wrote:

> Actually, I remembered in the dark recesses of my moldering brain 
> that someone had a lost patch for this a while ago, so I searched
> the archives.  Pavel, ok to add your s-o-b?

Since my patch was dropped and the new patch was implemented without my
participation, it makes no sense to put my s-o-b on the code I didn't
write (even though I wrote something similar before).

-- 
Regards,
Pavel Roskin

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Pavel Roskin]

On Thu, 2009-02-26 at 22:06 -0500, Bob Copeland wrote:
> On Thu, Feb 26, 2009 at 06:39:12PM -0800, Luis R. Rodriguez wrote:
> > Might be worth adding a note why this is the case. Can't we simply avoid
> > this by checking earlier for the error or simply assigning it an actual
> > default _good_ hw rate value?
> 
> I guess an alternative is to initialize to 0, that would count any rx
> packets whose hw rate we don't know about as the base rate, so it would
> probably bias the RC to 1mb, but this is already one of those 'should 
> never happen' cases.

I would prefer that we don't hide problems.

If we don't know why we cannot get a valid rate, we should use WARN_ON
and find out why and when it happens.  I'm fine with using a bogus rate
with WARN_ON.

If we decide that we indeed cannot find the actual rate, then WARN_ON
should be removed and the bogus rate replaced with an "unknown rate",
that is, a special value that is never translated to a valid rate and
never given to any rate control algorithm.  Using a bogus rate without a
warning is wrong in my opinion.

It should be possible to represent "unknown rate" as such.  That applies
to all drivers.  I remember that b43 also failed to report the rate in
some cases (for the first received packet or something like that).

-- 
Regards,
Pavel Roskin

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Bob Copeland]

On Sun, Mar 1, 2009 at 12:07 AM, Pavel Roskin <proski@gnu.org> wrote:
> On Thu, 2009-02-26 at 21:27 -0500, Bob Copeland wrote:
>
>> Actually, I remembered in the dark recesses of my moldering brain
>> that someone had a lost patch for this a while ago, so I searched
>> the archives.  Pavel, ok to add your s-o-b?
>
> Since my patch was dropped and the new patch was implemented without my
> participation, it makes no sense to put my s-o-b on the code I didn't
> write (even though I wrote something similar before).

Ok, I just wanted to be sure to maintain proper credit, the "From" should
suffice.  I did rewrite the patch but it actually had an identical diff.
FWIW, the thread didn't give a clue why it didn't make it upstream, just
missed I guess (http://marc.info/?l=linux-wireless&m=122480002519627&w=2,
ultimately that problem was fixed by correctly setting the rs_more flag).

Anyway, the patch, while IMO correct, will still result in mac80211
warning in ieee80211_rx with -1 just as 255 will; it just fixes the
subsequent out of bound read.  If we want to tell mac80211 a real rate,
I think we should change it to s8 then hw_to_driver_rix should do
something like:

idx = array[x][y];
if (WARN_ON(idx < 0))
   idx = 0;

return idx;

Then we get the warning in the driver and we also return a real rate up
the stack.  I'll prep a patch for this unless there are any objections.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Bob Copeland]

On Sun, Mar 01, 2009 at 12:21:52AM -0500, Pavel Roskin wrote:
> I would prefer that we don't hide problems.
> 
> If we don't know why we cannot get a valid rate, we should use WARN_ON
> and find out why and when it happens.  I'm fine with using a bogus rate
> with WARN_ON.

So here is at least stage one of this, not yet the global "unknown rate"
infrastructure, but hopefully it will allow us to track down the issue.

It makes hw_to_driver_rix a little uglier, but oh well.  Thoughts?

From: Bob Copeland <me@bobcopeland.com>
Date: Mon, 2 Mar 2009 21:55:18 -0500
Subject: [PATCH] ath5k: warn and correct rate for unknown hw rate indexes

ath5k sets up a mapping table from the hardware rate index to
the rate index used by mac80211; however, we have seen some
received frames with incorrect rate indexes.  Such frames
normally get dropped with a warning in __ieee80211_rx(), but the 
warning doesn't include enough context to track down the error.

This patch adds a warning to hw_to_driver_rix for any lookups
that result in a rate index of -1, then returns a valid rate so
the frame can be processed.

This also includes the bug fix suggested by Pavel Roskin, in which
the mapping table is made signed, so rates initialized to -1 stay
that way.

Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/net/wireless/ath5k/base.c |   15 ++++++++++++---
 drivers/net/wireless/ath5k/base.h |    2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index f7c424d..8d4b11c 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1100,9 +1100,18 @@ ath5k_mode_setup(struct ath5k_softc *sc)
 static inline int
 ath5k_hw_to_driver_rix(struct ath5k_softc *sc, int hw_rix)
 {
-	WARN(hw_rix < 0 || hw_rix >= AR5K_MAX_RATES,
-			"hw_rix out of bounds: %x\n", hw_rix);
-	return sc->rate_idx[sc->curband->band][hw_rix];
+	int rix;
+
+	/* return base rate on errors */
+	if (WARN(hw_rix < 0 || hw_rix >= AR5K_MAX_RATES,
+			"hw_rix out of bounds: %x\n", hw_rix))
+		return 0;
+
+	rix = sc->rate_idx[sc->curband->band][hw_rix];
+	if (WARN(rix < 0, "invalid hw_rix: %x\n", hw_rix))
+		rix = 0;
+
+	return rix;
 }
 
 /***************\
diff --git a/drivers/net/wireless/ath5k/base.h b/drivers/net/wireless/ath5k/base.h
index 20e0d14..8229561 100644
--- a/drivers/net/wireless/ath5k/base.h
+++ b/drivers/net/wireless/ath5k/base.h
@@ -112,7 +112,7 @@ struct ath5k_softc {
 	struct ieee80211_supported_band sbands[IEEE80211_NUM_BANDS];
 	struct ieee80211_channel channels[ATH_CHAN_MAX];
 	struct ieee80211_rate	rates[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
-	u8			rate_idx[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
+	s8			rate_idx[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
 	enum nl80211_iftype	opmode;
 	struct ath5k_hw		*ah;		/* Atheros HW */
 
-- 
1.6.0.6



-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Nick Kossifidis]

2009/3/3 Bob Copeland <me@bobcopeland.com>:
> On Sun, Mar 01, 2009 at 12:21:52AM -0500, Pavel Roskin wrote:
>> I would prefer that we don't hide problems.
>>
>> If we don't know why we cannot get a valid rate, we should use WARN_ON
>> and find out why and when it happens.  I'm fine with using a bogus rate
>> with WARN_ON.
>
> So here is at least stage one of this, not yet the global "unknown rate"
> infrastructure, but hopefully it will allow us to track down the issue.
>
> It makes hw_to_driver_rix a little uglier, but oh well.  Thoughts?
>
> From: Bob Copeland <me@bobcopeland.com>
> Date: Mon, 2 Mar 2009 21:55:18 -0500
> Subject: [PATCH] ath5k: warn and correct rate for unknown hw rate indexes
>
> ath5k sets up a mapping table from the hardware rate index to
> the rate index used by mac80211; however, we have seen some
> received frames with incorrect rate indexes.  Such frames
> normally get dropped with a warning in __ieee80211_rx(), but the
> warning doesn't include enough context to track down the error.
>
> This patch adds a warning to hw_to_driver_rix for any lookups
> that result in a rate index of -1, then returns a valid rate so
> the frame can be processed.
>
> This also includes the bug fix suggested by Pavel Roskin, in which
> the mapping table is made signed, so rates initialized to -1 stay
> that way.
>
> Signed-off-by: Bob Copeland <me@bobcopeland.com>
> ---
>  drivers/net/wireless/ath5k/base.c |   15 ++++++++++++---
>  drivers/net/wireless/ath5k/base.h |    2 +-
>  2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index f7c424d..8d4b11c 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
> @@ -1100,9 +1100,18 @@ ath5k_mode_setup(struct ath5k_softc *sc)
>  static inline int
>  ath5k_hw_to_driver_rix(struct ath5k_softc *sc, int hw_rix)
>  {
> -       WARN(hw_rix < 0 || hw_rix >= AR5K_MAX_RATES,
> -                       "hw_rix out of bounds: %x\n", hw_rix);
> -       return sc->rate_idx[sc->curband->band][hw_rix];
> +       int rix;
> +
> +       /* return base rate on errors */
> +       if (WARN(hw_rix < 0 || hw_rix >= AR5K_MAX_RATES,
> +                       "hw_rix out of bounds: %x\n", hw_rix))
> +               return 0;
> +
> +       rix = sc->rate_idx[sc->curband->band][hw_rix];
> +       if (WARN(rix < 0, "invalid hw_rix: %x\n", hw_rix))
> +               rix = 0;
> +
> +       return rix;
>  }
>
>  /***************\
> diff --git a/drivers/net/wireless/ath5k/base.h b/drivers/net/wireless/ath5k/base.h
> index 20e0d14..8229561 100644
> --- a/drivers/net/wireless/ath5k/base.h
> +++ b/drivers/net/wireless/ath5k/base.h
> @@ -112,7 +112,7 @@ struct ath5k_softc {
>        struct ieee80211_supported_band sbands[IEEE80211_NUM_BANDS];
>        struct ieee80211_channel channels[ATH_CHAN_MAX];
>        struct ieee80211_rate   rates[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
> -       u8                      rate_idx[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
> +       s8                      rate_idx[IEEE80211_NUM_BANDS][AR5K_MAX_RATES];
>        enum nl80211_iftype     opmode;
>        struct ath5k_hw         *ah;            /* Atheros HW */
>
> --
> 1.6.0.6
>

Another thought...

According to docs the rate field is only valid if more flag is clear
(we have the last descriptor) and only if the receive ok flag is set
or both receive ok and phy error flags are cleared. We never do such
checks so we might actually try to process this field when we already
know we shouldn't...

Also the following rate codes are reserved (except XR codes that we
already know):

0x00
0x04 -> the short preamble flag
0x05
0x10 - 0x17
0x1f

and i don't believe i've ever seen them, so we can warn on them too
and print something like "Reserved rate code: %x", also it would be
nice to warn on XR rates (1,2,3,6,7) in case we want to debug this in
the future.


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

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Bob Copeland]

On Tue, Mar 03, 2009 at 06:31:05AM +0200, Nick Kossifidis wrote:
> 
> According to docs the rate field is only valid if more flag is clear
> (we have the last descriptor) and only if the receive ok flag is set
> or both receive ok and phy error flags are cleared. We never do such
> checks so we might actually try to process this field when we already
> know we shouldn't...

Well, we do skip rs_more packets without getting the rate, hopefully 
phy error packets too.  The warning would definitely show if we have 
any bugs there.

> Also the following rate codes are reserved (except XR codes that we
> already know):
[snip]
> and i don't believe i've ever seen them, so we can warn on them too
> and print something like "Reserved rate code: %x", also it would be
> nice to warn on XR rates (1,2,3,6,7) in case we want to debug this in
> the future.

Good idea, though I'm somewhat of the mind that we should let the 
current patch go in for a bit and see if any of these pop up.  But 
that's because I'm lazy :)  

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 1/1] ath5k: fix hw rate index condition

[Bob Copeland]

On Tue, Mar 31, 2009 at 8:23 AM, Bob Copeland <me@bobcopeland.com> wrote:
> Ok - that is a useful data point.  Perhaps something to do with the rates
> the peer supports; it would help if you could grab a scan next time you
> are in the area.  Turn off auto-connect to open networks, then do:

Hi Dhaval,

Would you mind trying this patch and report the warnings it triggers?

http://marc.info/?l=linux-kernel&m=123915183521347&q=raw

-- 
Bob Copeland %% www.bobcopeland.com