If rix is not found in mi->r[], i will become -1 after the loop. This value
is eventually used to access arrays, so we were accessing arrays with a
negative index, which is obviously not what we want to do. This patch fixes
this potential problem.
Signed-off-by: Luciano Coelho <[email protected]>
Acked-by: Felix Fietkau <[email protected]>
---
net/mac80211/rc80211_minstrel.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index b218b98..37771ab 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -66,7 +66,7 @@ rix_to_ndx(struct minstrel_sta_info *mi, int rix)
for (i = rix; i >= 0; i--)
if (mi->r[i].rix == rix)
break;
- WARN_ON(mi->r[i].rix != rix);
+ WARN_ON(i < 0);
return i;
}
@@ -181,6 +181,9 @@ minstrel_tx_status(void *priv, struct ieee80211_supported_band *sband,
break;
ndx = rix_to_ndx(mi, ar[i].idx);
+ if (ndx < 0)
+ continue;
+
mi->r[ndx].attempts += ar[i].count;
if ((i != IEEE80211_TX_MAX_RATES - 1) && (ar[i + 1].idx < 0))
--
1.6.0.4
ext Johannes Berg wrote:
> On Fri, 2009-07-03 at 08:25 +0300, Luciano Coelho wrote:
>
>> If rix is not found in mi->r[], i will become -1 after the loop. This value
>> is eventually used to access arrays, so we were accessing arrays with a
>> negative index, which is obviously not what we want to do. This patch fixes
>> this potential problem.
>>
>
> This seems odd -- are you or are you not saying that this can happen in
> normal operation?
>
This should *not* happen in normal operation, but it was happening due
to a bug elsewhere (which has already been fixed).
>> @@ -66,7 +66,7 @@ rix_to_ndx(struct minstrel_sta_info *mi, int rix)
>> for (i = rix; i >= 0; i--)
>> if (mi->r[i].rix == rix)
>> break;
>> - WARN_ON(mi->r[i].rix != rix);
>> + WARN_ON(i < 0);
>> return i;
>>
>
> If it can, this warning seems wrong.
>
We were reaching this WARN_ON on our device some time ago, before we
backported some other minstrel fixes from upstream. So this patch
doesn't fix an *active* bug, but it makes things more robust in case
there is a bug elsewhere. It is not good at all to access mi->r[-1],
especially the code later which writes to this array and could
potentially overwrite somebody else's memory.
--
Cheers,
Luca.
On Fri, 2009-07-03 at 08:25 +0300, Luciano Coelho wrote:
> If rix is not found in mi->r[], i will become -1 after the loop. This value
> is eventually used to access arrays, so we were accessing arrays with a
> negative index, which is obviously not what we want to do. This patch fixes
> this potential problem.
This seems odd -- are you or are you not saying that this can happen in
normal operation?
> @@ -66,7 +66,7 @@ rix_to_ndx(struct minstrel_sta_info *mi, int rix)
> for (i = rix; i >= 0; i--)
> if (mi->r[i].rix == rix)
> break;
> - WARN_ON(mi->r[i].rix != rix);
> + WARN_ON(i < 0);
> return i;
If it can, this warning seems wrong.
johannes
On Friday 03 July 2009 10:29:02 Johannes Berg wrote:
> On Fri, 2009-07-03 at 08:25 +0300, Luciano Coelho wrote:
> > If rix is not found in mi->r[], i will become -1 after the loop. This value
> > is eventually used to access arrays, so we were accessing arrays with a
> > negative index, which is obviously not what we want to do. This patch fixes
> > this potential problem.
>
> This seems odd -- are you or are you not saying that this can happen in
> normal operation?
>
> > @@ -66,7 +66,7 @@ rix_to_ndx(struct minstrel_sta_info *mi, int rix)
> > for (i = rix; i >= 0; i--)
> > if (mi->r[i].rix == rix)
> > break;
> > - WARN_ON(mi->r[i].rix != rix);
> > + WARN_ON(i < 0);
> > return i;
>
> If it can, this warning seems wrong.
Well, the old WARN_ON seems wrong anyway, because it accesses the array
out of bounds. In case the loop did not find the entry, the warn on will look like this:
WARN_ON(mi->r[-1].rix != rix);
So I do think it's correct to replace the WARN_ON with WARN_ON(i < 0), if this can't
happen in normal operation. If it can happen in normal op, the warning should be removed
and the callers of rix_to_ndx() need to be checked.
--
Greetings, Michael.