Re: [PATCH v2] mac80211: minstrel: avoid accessing negative indices in rix_to_ndx()

[Michael Buesch <mb@bu3sch.de>]

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.