Re: Libertas: Association request to the driver failed

[Dan Williams <dcbw@redhat.com>]

On Tue, 2009-08-11 at 14:24 -0400, John W. Linville wrote:
> Comments from the libertas crowd?  This seems a bit long for this
> part of the cycle.
> 
> Should we just revert the original patch, then reapply it with this
> one for 2.6.32?

I'd feel more comfortable with that.  Roel did find a real problem, but
we need to make sure the fix doesn't break stuff since it appears the
rate code is more complicated than we thought.

Dan


> John
> 
> On Tue, Aug 11, 2009 at 09:02:14AM +0200, Roel Kluin wrote:
> > The size of the tmp buffer was too small, causing a regression
> > 
> > rates->rates has an arraysize of 1, so a memcpy with
> > MAX_RATES (14) was already causing reads out of bounds.
> > 
> > In get_common_rates() the memset/memcpy can be moved upwards.
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > Tested-by: Daniel Mack <daniel@caiaq.de>
> > ---
> > > Delta patch, please...
> > 
> > Here,
> > 
> > diff --git a/drivers/net/wireless/libertas/assoc.c b/drivers/net/wireless/libertas/assoc.c
> > index d699737..ba0164a 100644
> > --- a/drivers/net/wireless/libertas/assoc.c
> > +++ b/drivers/net/wireless/libertas/assoc.c
> > @@ -44,8 +44,8 @@ static int get_common_rates(struct lbs_private *priv,
> >  	u16 *rates_size)
> >  {
> >  	u8 *card_rates = lbs_bg_rates;
> > -	int ret = 0, i, j;
> > -	u8 tmp[(ARRAY_SIZE(lbs_bg_rates) - 1) * (*rates_size - 1)];
> > +	int i, j;
> > +	u8 tmp[MAX_RATES * ARRAY_SIZE(lbs_bg_rates)];
> >  	size_t tmp_size = 0;
> >  
> >  	/* For each rate in card_rates that exists in rate1, copy to tmp */
> > @@ -62,20 +62,23 @@ static int get_common_rates(struct lbs_private *priv,
> >  	lbs_deb_hex(LBS_DEB_JOIN, "common rates", tmp, tmp_size);
> >  	lbs_deb_join("TX data rate 0x%02x\n", priv->cur_rate);
> >  
> > +	memset(rates, 0, *rates_size);
> > +	*rates_size = min_t(u16, tmp_size, *rates_size);
> > +	memcpy(rates, tmp, *rates_size);
> > +
> >  	if (!priv->enablehwauto) {
> >  		for (i = 0; i < tmp_size; i++) {
> >  			if (tmp[i] == priv->cur_rate)
> > -				goto done;
> > +				break;
> > +		}
> > +		if (i == tmp_size) {
> > +			lbs_pr_alert("Previously set fixed data rate %#x isn't "
> > +					"compatible with the network.\n",
> > +					priv->cur_rate);
> > +			return -1;
> >  		}
> > -		lbs_pr_alert("Previously set fixed data rate %#x isn't "
> > -		       "compatible with the network.\n", priv->cur_rate);
> > -		ret = -1;
> >  	}
> > -done:
> > -	memset(rates, 0, *rates_size);
> > -	*rates_size = min_t(int, tmp_size, *rates_size);
> > -	memcpy(rates, tmp, *rates_size);
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  
> > @@ -319,8 +322,8 @@ static int lbs_associate(struct lbs_private *priv,
> >  
> >  	rates = (struct mrvl_ie_rates_param_set *) pos;
> >  	rates->header.type = cpu_to_le16(TLV_TYPE_RATES);
> > -	memcpy(&rates->rates, &bss->rates, MAX_RATES);
> >  	tmplen = min_t(u16, ARRAY_SIZE(rates->rates), MAX_RATES);
> > +	memcpy(&rates->rates, &bss->rates, tmplen);
> >  	if (get_common_rates(priv, rates->rates, &tmplen)) {
> >  		ret = -1;
> >  		goto done;
> > 
>