Libertas: Association request to the driver failed

Libertas: Association request to the driver failed

[Daniel Mack]

Since a recent merge of the wireless tree to Linus' master, my SDIO
connected libertas module fails to connect to our WLAN.

'iwlist scanning' works fine, but wpa_supplicant keeps on spitting out
the following message:

  CTRL-EVENT-SCAN-RESULTS 
  Trying to associate with 00:04:0e:4b:c7:b3 (SSID='caiaq' freq=2437 MHz)
  Association request to the driver failed

Haven't done any bisect or deeper inspection of recent changes yet. Can
anyone point me in some direction maybe? The change must have been
introduced between -rc3 and -rc5. My userspace did not change since
then.

Thanks,
Daniel

Re: Libertas: Association request to the driver failed

[John W. Linville]

On Fri, Aug 07, 2009 at 09:11:56PM +0200, Daniel Mack wrote:
> Since a recent merge of the wireless tree to Linus' master, my SDIO
> connected libertas module fails to connect to our WLAN.
> 
> 'iwlist scanning' works fine, but wpa_supplicant keeps on spitting out
> the following message:
> 
>   CTRL-EVENT-SCAN-RESULTS 
>   Trying to associate with 00:04:0e:4b:c7:b3 (SSID='caiaq' freq=2437 MHz)
>   Association request to the driver failed
> 
> Haven't done any bisect or deeper inspection of recent changes yet. Can
> anyone point me in some direction maybe? The change must have been
> introduced between -rc3 and -rc5. My userspace did not change since
> then.

Normally a bisect is exactly what you would do.  In this case, there
is only this on libertas patch in the range you mention:

commit 154839962a582b8eb661cde94ef3af0e03b374d7
Author: Marek Vasut <marek.vasut@gmail.com>
Date:   Thu Jul 16 19:19:53 2009 +0200

    libertas: Fix problem with broken V4 firmware on CF8381
    
    Firmware V4 on CF8381 reports region code shifted by 1 byte to left.
    The following patch checks for this and handles it properly.
    
    Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>

If you revert that, does your problem disappear?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: Libertas: Association request to the driver failed

[Marek Vasut]

Dne Pá 7. srpna 2009 21:36:10 John W. Linville napsal(a):
> On Fri, Aug 07, 2009 at 09:11:56PM +0200, Daniel Mack wrote:
> > Since a recent merge of the wireless tree to Linus' master, my SDIO
> > connected libertas module fails to connect to our WLAN.
> >
> > 'iwlist scanning' works fine, but wpa_supplicant keeps on spitting out
> > the following message:
> >
> >   CTRL-EVENT-SCAN-RESULTS
> >   Trying to associate with 00:04:0e:4b:c7:b3 (SSID='caiaq' freq=2437 MHz)
> >   Association request to the driver failed
> >
> > Haven't done any bisect or deeper inspection of recent changes yet. Can
> > anyone point me in some direction maybe? The change must have been
> > introduced between -rc3 and -rc5. My userspace did not change since
> > then.
>
> Normally a bisect is exactly what you would do.  In this case, there
> is only this on libertas patch in the range you mention:
>
> commit 154839962a582b8eb661cde94ef3af0e03b374d7
> Author: Marek Vasut <marek.vasut@gmail.com>
> Date:   Thu Jul 16 19:19:53 2009 +0200
>
>     libertas: Fix problem with broken V4 firmware on CF8381
>
>     Firmware V4 on CF8381 reports region code shifted by 1 byte to left.
>     The following patch checks for this and handles it properly.
>
>     Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>     Signed-off-by: John W. Linville <linville@tuxdriver.com>
>
> If you revert that, does your problem disappear?
>
> John
Hi!

Those SDIO cards should use FWv9 or something so this patch should have no 
effect.

Re: Libertas: Association request to the driver failed

[Daniel Mack]

On Fri, Aug 07, 2009 at 03:36:10PM -0400, John W. Linville wrote:
> On Fri, Aug 07, 2009 at 09:11:56PM +0200, Daniel Mack wrote:
> > Since a recent merge of the wireless tree to Linus' master, my SDIO
> > connected libertas module fails to connect to our WLAN.
> > 
> > 'iwlist scanning' works fine, but wpa_supplicant keeps on spitting out
> > the following message:
> > 
> >   CTRL-EVENT-SCAN-RESULTS 
> >   Trying to associate with 00:04:0e:4b:c7:b3 (SSID='caiaq' freq=2437 MHz)
> >   Association request to the driver failed
> > 
> > Haven't done any bisect or deeper inspection of recent changes yet. Can
> > anyone point me in some direction maybe? The change must have been
> > introduced between -rc3 and -rc5. My userspace did not change since
> > then.
> 
> Normally a bisect is exactly what you would do.

Hmm, I know. However, in my case, this is not possible as I keep a
number of patches I'm actively working on on top of the history by
rebasing my git tree constantly. And without these patches, the platform
won't boot, so bisect will almost certainly come up with an unbootable
image. But that's just a sidenote :)

> In this case, there
> is only this on libertas patch in the range you mention:

I was wrong with the range I provided. The change came in after -rc5,
and I found 57921c31 ("libertas: Read buffer overflow") to be the
culprit. Reverting it brings my libertas device back to life. I copied
the author.

Thanks,
Daniel

Re: Libertas: Association request to the driver failed

[Roel Kluin]

>>> Since a recent merge of the wireless tree to Linus' master, my SDIO
>>> connected libertas module fails to connect to our WLAN.
>>>
>>> 'iwlist scanning' works fine, but wpa_supplicant keeps on spitting out
>>> the following message:
>>>
>>>   CTRL-EVENT-SCAN-RESULTS 
>>>   Trying to associate with 00:04:0e:4b:c7:b3 (SSID='caiaq' freq=2437 MHz)
>>>   Association request to the driver failed

>> In this case, there
>> is only this on libertas patch in the range you mention:
> 
> I was wrong with the range I provided. The change came in after -rc5,
> and I found 57921c31 ("libertas: Read buffer overflow") to be the
> culprit. Reverting it brings my libertas device back to life. I copied
> the author.
> 
> Thanks,
> Daniel

Ah, I think I made an error, I think tmp is too small and should be

u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates) - 1];

Can we revert and does the patch below work? 

Sorry and thanks for testing,

Roel

commit 57921c312e8cef72ba35a4cfe870b376da0b1b87
Author: Roel Kluin <roel.kluin@gmail.com>
Date:   Tue Jul 28 12:05:00 2009 +0200

    libertas: Read buffer overflow
    
    Several arrays were read before checking whether the index was within
    bounds. ARRAY_SIZE() should be used to determine the size of arrays.
    
    rates->rates has an arraysize of 1, so calling get_common_rates()
    with a rates_size of MAX_RATES (14) was causing reads out of bounds.
    
    tmp_size can increment at most to (ARRAY_SIZE(lbs_bg_rates) - 1) *
    (*rates_size - 1), so that should be the number of elements of tmp[].
    
    A goto can be eliminated: ret was already set upon its declaration.
    
    Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>

diff --git a/drivers/net/wireless/libertas/assoc.c b/drivers/net/wireless/libertas/assoc.c
index b9b3741..d699737 100644
--- a/drivers/net/wireless/libertas/assoc.c
+++ b/drivers/net/wireless/libertas/assoc.c
@@ -1,6 +1,7 @@
 /* Copyright (C) 2006, Red Hat, Inc. */
 
 #include <linux/types.h>
+#include <linux/kernel.h>
 #include <linux/etherdevice.h>
 #include <linux/ieee80211.h>
 #include <linux/if_arp.h>
@@ -43,21 +44,21 @@ static int get_common_rates(struct lbs_private *priv,
 	u16 *rates_size)
 {
 	u8 *card_rates = lbs_bg_rates;
-	size_t num_card_rates = sizeof(lbs_bg_rates);
 	int ret = 0, i, j;
-	u8 tmp[30];
+	u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates) - 1];
 	size_t tmp_size = 0;
 
 	/* For each rate in card_rates that exists in rate1, copy to tmp */
-	for (i = 0; card_rates[i] && (i < num_card_rates); i++) {
-		for (j = 0; rates[j] && (j < *rates_size); j++) {
+	for (i = 0; i < ARRAY_SIZE(lbs_bg_rates) && card_rates[i]; i++) {
+		for (j = 0; j < *rates_size && rates[j]; j++) {
 			if (rates[j] == card_rates[i])
 				tmp[tmp_size++] = card_rates[i];
 		}
 	}
 
 	lbs_deb_hex(LBS_DEB_JOIN, "AP rates    ", rates, *rates_size);
-	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates, num_card_rates);
+	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates,
+			ARRAY_SIZE(lbs_bg_rates));
 	lbs_deb_hex(LBS_DEB_JOIN, "common rates", tmp, tmp_size);
 	lbs_deb_join("TX data rate 0x%02x\n", priv->cur_rate);
 
@@ -69,10 +70,7 @@ static int get_common_rates(struct lbs_private *priv,
 		lbs_pr_alert("Previously set fixed data rate %#x isn't "
 		       "compatible with the network.\n", priv->cur_rate);
 		ret = -1;
-		goto done;
 	}
-	ret = 0;
-
 done:
 	memset(rates, 0, *rates_size);
 	*rates_size = min_t(int, tmp_size, *rates_size);
@@ -322,7 +320,7 @@ 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 = MAX_RATES;
+	tmplen = min_t(u16, ARRAY_SIZE(rates->rates), MAX_RATES);
 	if (get_common_rates(priv, rates->rates, &tmplen)) {
 		ret = -1;
 		goto done;
@@ -598,7 +596,7 @@ static int lbs_adhoc_join(struct lbs_private *priv,
 
 	/* Copy Data rates from the rates recorded in scan response */
 	memset(cmd.bss.rates, 0, sizeof(cmd.bss.rates));
-	ratesize = min_t(u16, sizeof(cmd.bss.rates), MAX_RATES);
+	ratesize = min_t(u16, ARRAY_SIZE(cmd.bss.rates), MAX_RATES);
 	memcpy(cmd.bss.rates, bss->rates, ratesize);
 	if (get_common_rates(priv, cmd.bss.rates, &ratesize)) {
 		lbs_deb_join("ADHOC_JOIN: get_common_rates returned error.\n");

Re: Libertas: Association request to the driver failed

[Roel Kluin]

>> The change came in after -rc5,
>> and I found 57921c31 ("libertas: Read buffer overflow") to be the
>> culprit. Reverting it brings my libertas device back to life. I copied
>> the author.
>>
>> Thanks,
>> Daniel
> 
> Ah, I think I made an error, I think tmp is too small and should be
> 
> u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates) - 1];

After some sleep I realized it should be:

u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates)];


> Sorry and thanks for testing,
> 
> Roel

Re: Libertas: Association request to the driver failed

[Daniel Mack]

On Sun, Aug 09, 2009 at 11:23:34AM +0200, Roel Kluin wrote:
> >> The change came in after -rc5,
> >> and I found 57921c31 ("libertas: Read buffer overflow") to be the
> >> culprit. Reverting it brings my libertas device back to life. I copied
> >> the author.
> >>
> >> Thanks,
> >> Daniel
> > 
> > Ah, I think I made an error, I think tmp is too small and should be
> > 
> > u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates) - 1];
> 
> After some sleep I realized it should be:
> 
> u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates)];

I'll test that tomorrow. Would be easier if you send in a new patch I
can ack directly in case it works :)

Thanks,
Daniel

Re: Libertas: Association request to the driver failed

[Roel Kluin]

Several arrays were read before checking whether the index was within
bounds. ARRAY_SIZE() should be used to determine the size of arrays.

rates->rates has an arraysize of 1, so calling get_common_rates()
with a rates_size of MAX_RATES (14) was causing reads out of bounds.

tmp_size can increment at most to *rates_size * ARRAY_SIZE(lbs_bg_rates),
so that should be the number of elements of tmp[].

A goto can be eliminated: ret was already set upon its declaration.

---
>>>> The change came in after -rc5,
>>>> and I found 57921c31 ("libertas: Read buffer overflow") to be the
>>>> culprit. Reverting it brings my libertas device back to life. I copied
>>>> the author.
>>>>
>>>> Thanks,
>>>> Daniel
>>> Ah, I think I made an error, I think tmp is too small and should be
>>>
>>> u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates) - 1];
>> After some sleep I realized it should be:
>>
>> u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates)];
> 
> I'll test that tomorrow. Would be easier if you send in a new patch I
> can ack directly in case it works :)

This is the patch that should be applied after the revert, or do you want a
delta patch?

And does this give your libertas back?

Thanks for testing,

Roel

diff --git a/drivers/net/wireless/libertas/assoc.c b/drivers/net/wireless/libertas/assoc.c
index b9b3741..e9efc4c 100644
--- a/drivers/net/wireless/libertas/assoc.c
+++ b/drivers/net/wireless/libertas/assoc.c
@@ -1,6 +1,7 @@
 /* Copyright (C) 2006, Red Hat, Inc. */
 
 #include <linux/types.h>
+#include <linux/kernel.h>
 #include <linux/etherdevice.h>
 #include <linux/ieee80211.h>
 #include <linux/if_arp.h>
@@ -43,21 +44,21 @@ static int get_common_rates(struct lbs_private *priv,
 	u16 *rates_size)
 {
 	u8 *card_rates = lbs_bg_rates;
-	size_t num_card_rates = sizeof(lbs_bg_rates);
 	int ret = 0, i, j;
-	u8 tmp[30];
+	u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates)];
 	size_t tmp_size = 0;
 
 	/* For each rate in card_rates that exists in rate1, copy to tmp */
-	for (i = 0; card_rates[i] && (i < num_card_rates); i++) {
-		for (j = 0; rates[j] && (j < *rates_size); j++) {
+	for (i = 0; i < ARRAY_SIZE(lbs_bg_rates) && card_rates[i]; i++) {
+		for (j = 0; j < *rates_size && rates[j]; j++) {
 			if (rates[j] == card_rates[i])
 				tmp[tmp_size++] = card_rates[i];
 		}
 	}
 
 	lbs_deb_hex(LBS_DEB_JOIN, "AP rates    ", rates, *rates_size);
-	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates, num_card_rates);
+	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates,
+			ARRAY_SIZE(lbs_bg_rates));
 	lbs_deb_hex(LBS_DEB_JOIN, "common rates", tmp, tmp_size);
 	lbs_deb_join("TX data rate 0x%02x\n", priv->cur_rate);
 
@@ -69,10 +70,7 @@ static int get_common_rates(struct lbs_private *priv,
 		lbs_pr_alert("Previously set fixed data rate %#x isn't "
 		       "compatible with the network.\n", priv->cur_rate);
 		ret = -1;
-		goto done;
 	}
-	ret = 0;
-
 done:
 	memset(rates, 0, *rates_size);
 	*rates_size = min_t(int, tmp_size, *rates_size);
@@ -322,7 +320,7 @@ 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 = MAX_RATES;
+	tmplen = min_t(u16, ARRAY_SIZE(rates->rates), MAX_RATES);
 	if (get_common_rates(priv, rates->rates, &tmplen)) {
 		ret = -1;
 		goto done;
@@ -598,7 +596,7 @@ static int lbs_adhoc_join(struct lbs_private *priv,
 
 	/* Copy Data rates from the rates recorded in scan response */
 	memset(cmd.bss.rates, 0, sizeof(cmd.bss.rates));
-	ratesize = min_t(u16, sizeof(cmd.bss.rates), MAX_RATES);
+	ratesize = min_t(u16, ARRAY_SIZE(cmd.bss.rates), MAX_RATES);
 	memcpy(cmd.bss.rates, bss->rates, ratesize);
 	if (get_common_rates(priv, cmd.bss.rates, &ratesize)) {
 		lbs_deb_join("ADHOC_JOIN: get_common_rates returned error.\n");

Re: Libertas: Association request to the driver failed

[Michael Buesch]

On Sunday 09 August 2009 13:11:20 Roel Kluin wrote:
> @@ -43,21 +44,21 @@ static int get_common_rates(struct lbs_private *priv,
>  	u16 *rates_size)
>  {
>  	u8 *card_rates = lbs_bg_rates;
> -	size_t num_card_rates = sizeof(lbs_bg_rates);
>  	int ret = 0, i, j;
> -	u8 tmp[30];
> +	u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates)];

Is it a good idea to use dynamic stack arrays in the kernel?
What about kmalloc for dynamic allocations?

-- 
Greetings, Michael.

Re: Libertas: Association request to the driver failed

[Cyrill Gorcunov]

[Michael Buesch - Sun, Aug 09, 2009 at 01:10:56PM +0200]
| On Sunday 09 August 2009 13:11:20 Roel Kluin wrote:
| > @@ -43,21 +44,21 @@ static int get_common_rates(struct lbs_private *priv,
| >  	u16 *rates_size)
| >  {
| >  	u8 *card_rates = lbs_bg_rates;
| > -	size_t num_card_rates = sizeof(lbs_bg_rates);
| >  	int ret = 0, i, j;
| > -	u8 tmp[30];
| > +	u8 tmp[*rates_size * ARRAY_SIZE(lbs_bg_rates)];
| 
| Is it a good idea to use dynamic stack arrays in the kernel?
| What about kmalloc for dynamic allocations?
| 
| -- 
| Greetings, Michael.

I saw one pattern in trace code (not sure if it's
still there) but personally don't like dynamic
stack arrays (though at moment the max value
being passed into routine is known maybe just
use MAX_RATES instead of (*rates_size)?). Hmm?

	-- Cyrill

Re: Libertas: Association request to the driver failed

[Roel Kluin]

Several arrays were read before checking whether the index was within
bounds. ARRAY_SIZE() should be used to determine the size of arrays.

rates->rates has an arraysize of 1, so calling get_common_rates()
with a rates_size of MAX_RATES (14) was causing reads out of bounds.

tmp_size can increment at most to MAX_RATES * ARRAY_SIZE(lbs_bg_rates),
so that should be the number of elements of tmp[].

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---

> | Is it a good idea to use dynamic stack arrays in the kernel?
> | What about kmalloc for dynamic allocations?
> | 
> | -- 
> | Greetings, Michael.
> 
> I saw one pattern in trace code (not sure if it's
> still there) but personally don't like dynamic
> stack arrays (though at moment the max value
> being passed into routine is known maybe just
> use MAX_RATES instead of (*rates_size)?). Hmm?

Good point.

> 	-- Cyrill

Thanks,

I think there was another problem in lbs_associate(),
the memcpy already affected rates->rates.

Also in get_common_rates() I think we can safely move the
memset/memcpy, originally after label done, upwards.

The patch below, if correct, is to be applied after the revert

Roel

diff --git a/drivers/net/wireless/libertas/assoc.c b/drivers/net/wireless/libertas/assoc.c
index b9b3741..ba0164a 100644
--- a/drivers/net/wireless/libertas/assoc.c
+++ b/drivers/net/wireless/libertas/assoc.c
@@ -1,6 +1,7 @@
 /* Copyright (C) 2006, Red Hat, Inc. */
 
 #include <linux/types.h>
+#include <linux/kernel.h>
 #include <linux/etherdevice.h>
 #include <linux/ieee80211.h>
 #include <linux/if_arp.h>
@@ -43,41 +44,41 @@ static int get_common_rates(struct lbs_private *priv,
 	u16 *rates_size)
 {
 	u8 *card_rates = lbs_bg_rates;
-	size_t num_card_rates = sizeof(lbs_bg_rates);
-	int ret = 0, i, j;
-	u8 tmp[30];
+	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 */
-	for (i = 0; card_rates[i] && (i < num_card_rates); i++) {
-		for (j = 0; rates[j] && (j < *rates_size); j++) {
+	for (i = 0; i < ARRAY_SIZE(lbs_bg_rates) && card_rates[i]; i++) {
+		for (j = 0; j < *rates_size && rates[j]; j++) {
 			if (rates[j] == card_rates[i])
 				tmp[tmp_size++] = card_rates[i];
 		}
 	}
 
 	lbs_deb_hex(LBS_DEB_JOIN, "AP rates    ", rates, *rates_size);
-	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates, num_card_rates);
+	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates,
+			ARRAY_SIZE(lbs_bg_rates));
 	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;
-		goto done;
 	}
-	ret = 0;
-
-done:
-	memset(rates, 0, *rates_size);
-	*rates_size = min_t(int, tmp_size, *rates_size);
-	memcpy(rates, tmp, *rates_size);
-	return ret;
+	return 0;
 }
 
 
@@ -321,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 = 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;
@@ -598,7 +599,7 @@ static int lbs_adhoc_join(struct lbs_private *priv,
 
 	/* Copy Data rates from the rates recorded in scan response */
 	memset(cmd.bss.rates, 0, sizeof(cmd.bss.rates));
-	ratesize = min_t(u16, sizeof(cmd.bss.rates), MAX_RATES);
+	ratesize = min_t(u16, ARRAY_SIZE(cmd.bss.rates), MAX_RATES);
 	memcpy(cmd.bss.rates, bss->rates, ratesize);
 	if (get_common_rates(priv, cmd.bss.rates, &ratesize)) {
 		lbs_deb_join("ADHOC_JOIN: get_common_rates returned error.\n");

Re: Libertas: Association request to the driver failed

[Cyrill Gorcunov]

[Roel Kluin - Mon, Aug 10, 2009 at 12:37:00PM +0200]
...
| > I saw one pattern in trace code (not sure if it's
| > still there) but personally don't like dynamic
| > stack arrays (though at moment the max value
| > being passed into routine is known maybe just
| > use MAX_RATES instead of (*rates_size)?). Hmm?
| 
| Good point.
| 
| > 	-- Cyrill
| 
| Thanks,
| 
| I think there was another problem in lbs_associate(),
| the memcpy already affected rates->rates.
| 

Yeah, something like that. Note that I was only cared about
stack so I didn't dive into details of this code :)

I suppose wireless mainteiners will review it more
precisely. Thanks Roel!

	-- Cyrill

Re: Libertas: Association request to the driver failed

[Daniel Mack]

On Mon, Aug 10, 2009 at 12:37:00PM +0200, Roel Kluin wrote:
> I think there was another problem in lbs_associate(),
> the memcpy already affected rates->rates.
> 
> Also in get_common_rates() I think we can safely move the
> memset/memcpy, originally after label done, upwards.
> 
> The patch below, if correct, is to be applied after the revert

I tested that and the driver still works fine for me. Thanks :)

Feel free to add my Tested-by: if you like.

Daniel


> diff --git a/drivers/net/wireless/libertas/assoc.c b/drivers/net/wireless/libertas/assoc.c
> index b9b3741..ba0164a 100644
> --- a/drivers/net/wireless/libertas/assoc.c
> +++ b/drivers/net/wireless/libertas/assoc.c
> @@ -1,6 +1,7 @@
>  /* Copyright (C) 2006, Red Hat, Inc. */
>  
>  #include <linux/types.h>
> +#include <linux/kernel.h>
>  #include <linux/etherdevice.h>
>  #include <linux/ieee80211.h>
>  #include <linux/if_arp.h>
> @@ -43,41 +44,41 @@ static int get_common_rates(struct lbs_private *priv,
>  	u16 *rates_size)
>  {
>  	u8 *card_rates = lbs_bg_rates;
> -	size_t num_card_rates = sizeof(lbs_bg_rates);
> -	int ret = 0, i, j;
> -	u8 tmp[30];
> +	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 */
> -	for (i = 0; card_rates[i] && (i < num_card_rates); i++) {
> -		for (j = 0; rates[j] && (j < *rates_size); j++) {
> +	for (i = 0; i < ARRAY_SIZE(lbs_bg_rates) && card_rates[i]; i++) {
> +		for (j = 0; j < *rates_size && rates[j]; j++) {
>  			if (rates[j] == card_rates[i])
>  				tmp[tmp_size++] = card_rates[i];
>  		}
>  	}
>  
>  	lbs_deb_hex(LBS_DEB_JOIN, "AP rates    ", rates, *rates_size);
> -	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates, num_card_rates);
> +	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates,
> +			ARRAY_SIZE(lbs_bg_rates));
>  	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;
> -		goto done;
>  	}
> -	ret = 0;
> -
> -done:
> -	memset(rates, 0, *rates_size);
> -	*rates_size = min_t(int, tmp_size, *rates_size);
> -	memcpy(rates, tmp, *rates_size);
> -	return ret;
> +	return 0;
>  }
>  
>  
> @@ -321,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 = 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;
> @@ -598,7 +599,7 @@ static int lbs_adhoc_join(struct lbs_private *priv,
>  
>  	/* Copy Data rates from the rates recorded in scan response */
>  	memset(cmd.bss.rates, 0, sizeof(cmd.bss.rates));
> -	ratesize = min_t(u16, sizeof(cmd.bss.rates), MAX_RATES);
> +	ratesize = min_t(u16, ARRAY_SIZE(cmd.bss.rates), MAX_RATES);
>  	memcpy(cmd.bss.rates, bss->rates, ratesize);
>  	if (get_common_rates(priv, cmd.bss.rates, &ratesize)) {
>  		lbs_deb_join("ADHOC_JOIN: get_common_rates returned error.\n");

Re: Libertas: Association request to the driver failed

[Jonathan Cameron]

Just as a heads up, even with this patch I'm still getting
problems with association that didn't occur before. Just reverted
and all works fine.

I'll try and pin them down and report back later this afternoon.

Jonathan
> On Mon, Aug 10, 2009 at 12:37:00PM +0200, Roel Kluin wrote:
>> I think there was another problem in lbs_associate(),
>> the memcpy already affected rates->rates.
>>
>> Also in get_common_rates() I think we can safely move the
>> memset/memcpy, originally after label done, upwards.
>>
>> The patch below, if correct, is to be applied after the revert
> 
> I tested that and the driver still works fine for me. Thanks :)
> 
> Feel free to add my Tested-by: if you like.
> 
> Daniel
> 
> 
>> diff --git a/drivers/net/wireless/libertas/assoc.c b/drivers/net/wireless/libertas/assoc.c
>> index b9b3741..ba0164a 100644
>> --- a/drivers/net/wireless/libertas/assoc.c
>> +++ b/drivers/net/wireless/libertas/assoc.c
>> @@ -1,6 +1,7 @@
>>  /* Copyright (C) 2006, Red Hat, Inc. */
>>  
>>  #include <linux/types.h>
>> +#include <linux/kernel.h>
>>  #include <linux/etherdevice.h>
>>  #include <linux/ieee80211.h>
>>  #include <linux/if_arp.h>
>> @@ -43,41 +44,41 @@ static int get_common_rates(struct lbs_private *priv,
>>  	u16 *rates_size)
>>  {
>>  	u8 *card_rates = lbs_bg_rates;
>> -	size_t num_card_rates = sizeof(lbs_bg_rates);
>> -	int ret = 0, i, j;
>> -	u8 tmp[30];
>> +	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 */
>> -	for (i = 0; card_rates[i] && (i < num_card_rates); i++) {
>> -		for (j = 0; rates[j] && (j < *rates_size); j++) {
>> +	for (i = 0; i < ARRAY_SIZE(lbs_bg_rates) && card_rates[i]; i++) {
>> +		for (j = 0; j < *rates_size && rates[j]; j++) {
>>  			if (rates[j] == card_rates[i])
>>  				tmp[tmp_size++] = card_rates[i];
>>  		}
>>  	}
>>  
>>  	lbs_deb_hex(LBS_DEB_JOIN, "AP rates    ", rates, *rates_size);
>> -	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates, num_card_rates);
>> +	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates,
>> +			ARRAY_SIZE(lbs_bg_rates));
>>  	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;
>> -		goto done;
>>  	}
>> -	ret = 0;
>> -
>> -done:
>> -	memset(rates, 0, *rates_size);
>> -	*rates_size = min_t(int, tmp_size, *rates_size);
>> -	memcpy(rates, tmp, *rates_size);
>> -	return ret;
>> +	return 0;
>>  }
>>  
>>  
>> @@ -321,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 = 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;
>> @@ -598,7 +599,7 @@ static int lbs_adhoc_join(struct lbs_private *priv,
>>  
>>  	/* Copy Data rates from the rates recorded in scan response */
>>  	memset(cmd.bss.rates, 0, sizeof(cmd.bss.rates));
>> -	ratesize = min_t(u16, sizeof(cmd.bss.rates), MAX_RATES);
>> +	ratesize = min_t(u16, ARRAY_SIZE(cmd.bss.rates), MAX_RATES);
>>  	memcpy(cmd.bss.rates, bss->rates, ratesize);
>>  	if (get_common_rates(priv, cmd.bss.rates, &ratesize)) {
>>  		lbs_deb_join("ADHOC_JOIN: get_common_rates returned error.\n");
> 
> _______________________________________________
> libertas-dev mailing list
> libertas-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev
> 

Re: Libertas: Association request to the driver failed

[Jonathan Cameron]

Hi All, 

After applying this patch I've been receiving 0x12 response from
an access point (association failed: not all rates supported)
to association requests.

See below for queries on what is happening,
> Several arrays were read before checking whether the index was within
> bounds. ARRAY_SIZE() should be used to determine the size of arrays.
> 
> rates->rates has an arraysize of 1, so calling get_common_rates()
> with a rates_size of MAX_RATES (14) was causing reads out of bounds.
> 
> tmp_size can increment at most to MAX_RATES * ARRAY_SIZE(lbs_bg_rates),
> so that should be the number of elements of tmp[].
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> 
>> | Is it a good idea to use dynamic stack arrays in the kernel?
>> | What about kmalloc for dynamic allocations?
>> | 
>> | -- 
>> | Greetings, Michael.
>>
>> I saw one pattern in trace code (not sure if it's
>> still there) but personally don't like dynamic
>> stack arrays (though at moment the max value
>> being passed into routine is known maybe just
>> use MAX_RATES instead of (*rates_size)?). Hmm?
> 
> Good point.
> 
>> 	-- Cyrill
> 
> Thanks,
> 
> I think there was another problem in lbs_associate(),
> the memcpy already affected rates->rates.
> 
> Also in get_common_rates() I think we can safely move the
> memset/memcpy, originally after label done, upwards.
> 
> The patch below, if correct, is to be applied after the revert
> 
> Roel
> 
> diff --git a/drivers/net/wireless/libertas/assoc.c b/drivers/net/wireless/libertas/assoc.c
> index b9b3741..ba0164a 100644
> --- a/drivers/net/wireless/libertas/assoc.c
> +++ b/drivers/net/wireless/libertas/assoc.c
> @@ -1,6 +1,7 @@
>  /* Copyright (C) 2006, Red Hat, Inc. */
>  
>  #include <linux/types.h>
> +#include <linux/kernel.h>
>  #include <linux/etherdevice.h>
>  #include <linux/ieee80211.h>
>  #include <linux/if_arp.h>
> @@ -43,41 +44,41 @@ static int get_common_rates(struct lbs_private *priv,
>  	u16 *rates_size)
>  {
>  	u8 *card_rates = lbs_bg_rates;
> -	size_t num_card_rates = sizeof(lbs_bg_rates);
> -	int ret = 0, i, j;
> -	u8 tmp[30];
> +	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 */
> -	for (i = 0; card_rates[i] && (i < num_card_rates); i++) {
> -		for (j = 0; rates[j] && (j < *rates_size); j++) {
> +	for (i = 0; i < ARRAY_SIZE(lbs_bg_rates) && card_rates[i]; i++) {
> +		for (j = 0; j < *rates_size && rates[j]; j++) {
>  			if (rates[j] == card_rates[i])
>  				tmp[tmp_size++] = card_rates[i];
>  		}
>  	}
>  
>  	lbs_deb_hex(LBS_DEB_JOIN, "AP rates    ", rates, *rates_size);
> -	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates, num_card_rates);
> +	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates,
> +			ARRAY_SIZE(lbs_bg_rates));
>  	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;
> -		goto done;
>  	}
> -	ret = 0;
> -
> -done:
> -	memset(rates, 0, *rates_size);
> -	*rates_size = min_t(int, tmp_size, *rates_size);
> -	memcpy(rates, tmp, *rates_size);
> -	return ret;
> +	return 0;
>  }
>  
>  
> @@ -321,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 = MAX_RATES;
> +	tmplen = min_t(u16, ARRAY_SIZE(rates->rates), MAX_RATES);
Isn't this always going to be 1? Switching back to original version
allows association to work for me.

As is, it only allows one rate to be tested as ARRAY_SIZE(rates->rates)
is always 1 as it stands.  

If this is the desired behaviour please explain why?
I'll admit I'm not really sure what should be happening, I've merely
been bisecting looking for what was causing a regression for me.

> +	memcpy(&rates->rates, &bss->rates, tmplen);
>  	if (get_common_rates(priv, rates->rates, &tmplen)) {
>  		ret = -1;
>  		goto done;
> @@ -598,7 +599,7 @@ static int lbs_adhoc_join(struct lbs_private *priv,
>  
>  	/* Copy Data rates from the rates recorded in scan response */
>  	memset(cmd.bss.rates, 0, sizeof(cmd.bss.rates));
> -	ratesize = min_t(u16, sizeof(cmd.bss.rates), MAX_RATES);
> +	ratesize = min_t(u16, ARRAY_SIZE(cmd.bss.rates), MAX_RATES);
>  	memcpy(cmd.bss.rates, bss->rates, ratesize);
>  	if (get_common_rates(priv, cmd.bss.rates, &ratesize)) {
>  		lbs_deb_join("ADHOC_JOIN: get_common_rates returned error.\n");
> 
> _______________________________________________
> libertas-dev mailing list
> libertas-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/libertas-dev
> 

Re: Libertas: Association request to the driver failed

[Dan Williams]

On Wed, 2009-08-12 at 08:47 +0000, Jonathan Cameron wrote:
> Hi All, 
> 
> After applying this patch I've been receiving 0x12 response from
> an access point (association failed: not all rates supported)
> to association requests.
> 
> See below for queries on what is happening,
> > Several arrays were read before checking whether the index was within
> > bounds. ARRAY_SIZE() should be used to determine the size of arrays.
> > 
> > rates->rates has an arraysize of 1, so calling get_common_rates()
> > with a rates_size of MAX_RATES (14) was causing reads out of bounds.
> > 
> > tmp_size can increment at most to MAX_RATES * ARRAY_SIZE(lbs_bg_rates),
> > so that should be the number of elements of tmp[].
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > 
> >> | Is it a good idea to use dynamic stack arrays in the kernel?
> >> | What about kmalloc for dynamic allocations?
> >> | 
> >> | -- 
> >> | Greetings, Michael.
> >>
> >> I saw one pattern in trace code (not sure if it's
> >> still there) but personally don't like dynamic
> >> stack arrays (though at moment the max value
> >> being passed into routine is known maybe just
> >> use MAX_RATES instead of (*rates_size)?). Hmm?
> > 
> > Good point.
> > 
> >> 	-- Cyrill
> > 
> > Thanks,
> > 
> > I think there was another problem in lbs_associate(),
> > the memcpy already affected rates->rates.
> > 
> > Also in get_common_rates() I think we can safely move the
> > memset/memcpy, originally after label done, upwards.
> > 
> > The patch below, if correct, is to be applied after the revert
> > 
> > Roel
> > 
> > diff --git a/drivers/net/wireless/libertas/assoc.c b/drivers/net/wireless/libertas/assoc.c
> > index b9b3741..ba0164a 100644
> > --- a/drivers/net/wireless/libertas/assoc.c
> > +++ b/drivers/net/wireless/libertas/assoc.c
> > @@ -1,6 +1,7 @@
> >  /* Copyright (C) 2006, Red Hat, Inc. */
> >  
> >  #include <linux/types.h>
> > +#include <linux/kernel.h>
> >  #include <linux/etherdevice.h>
> >  #include <linux/ieee80211.h>
> >  #include <linux/if_arp.h>
> > @@ -43,41 +44,41 @@ static int get_common_rates(struct lbs_private *priv,
> >  	u16 *rates_size)
> >  {
> >  	u8 *card_rates = lbs_bg_rates;
> > -	size_t num_card_rates = sizeof(lbs_bg_rates);
> > -	int ret = 0, i, j;
> > -	u8 tmp[30];
> > +	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 */
> > -	for (i = 0; card_rates[i] && (i < num_card_rates); i++) {
> > -		for (j = 0; rates[j] && (j < *rates_size); j++) {
> > +	for (i = 0; i < ARRAY_SIZE(lbs_bg_rates) && card_rates[i]; i++) {
> > +		for (j = 0; j < *rates_size && rates[j]; j++) {
> >  			if (rates[j] == card_rates[i])
> >  				tmp[tmp_size++] = card_rates[i];
> >  		}
> >  	}
> >  
> >  	lbs_deb_hex(LBS_DEB_JOIN, "AP rates    ", rates, *rates_size);
> > -	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates, num_card_rates);
> > +	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates,
> > +			ARRAY_SIZE(lbs_bg_rates));
> >  	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;
> > -		goto done;
> >  	}
> > -	ret = 0;
> > -
> > -done:
> > -	memset(rates, 0, *rates_size);
> > -	*rates_size = min_t(int, tmp_size, *rates_size);
> > -	memcpy(rates, tmp, *rates_size);
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  
> > @@ -321,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 = MAX_RATES;
> > +	tmplen = min_t(u16, ARRAY_SIZE(rates->rates), MAX_RATES);
> Isn't this always going to be 1? Switching back to original version
> allows association to work for me.
> 
> As is, it only allows one rate to be tested as ARRAY_SIZE(rates->rates)
> is always 1 as it stands.  

No, it was basically supposed to be either the # of rates in rates, or
MAX_RATES (or something like that).  Basically, it should *never* be 1,
it should be either the # of 802.11b rates (which is like 5) or the # of
802.11g rates (which is like 12 or 13 or something).  But never 1.

Dan

> If this is the desired behaviour please explain why?
> I'll admit I'm not really sure what should be happening, I've merely
> been bisecting looking for what was causing a regression for me.
> 
> > +	memcpy(&rates->rates, &bss->rates, tmplen);
> >  	if (get_common_rates(priv, rates->rates, &tmplen)) {
> >  		ret = -1;
> >  		goto done;
> > @@ -598,7 +599,7 @@ static int lbs_adhoc_join(struct lbs_private *priv,
> >  
> >  	/* Copy Data rates from the rates recorded in scan response */
> >  	memset(cmd.bss.rates, 0, sizeof(cmd.bss.rates));
> > -	ratesize = min_t(u16, sizeof(cmd.bss.rates), MAX_RATES);
> > +	ratesize = min_t(u16, ARRAY_SIZE(cmd.bss.rates), MAX_RATES);
> >  	memcpy(cmd.bss.rates, bss->rates, ratesize);
> >  	if (get_common_rates(priv, cmd.bss.rates, &ratesize)) {
> >  		lbs_deb_join("ADHOC_JOIN: get_common_rates returned error.\n");
> > 
> > _______________________________________________
> > libertas-dev mailing list
> > libertas-dev@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/libertas-dev
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Libertas: Association request to the driver failed

[John W. Linville]

On Sun, Aug 09, 2009 at 01:11:20PM +0200, Roel Kluin wrote:
> > I'll test that tomorrow. Would be easier if you send in a new patch I
> > can ack directly in case it works :)
> 
> This is the patch that should be applied after the revert, or do you want a
> delta patch?

Delta patch, please...

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: Libertas: Association request to the driver failed

[Roel Kluin]

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;

Re: Libertas: Association request to the driver failed

[John W. Linville]

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?

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;
> 

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: Libertas: Association request to the driver failed

[Dan Williams]

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;
> > 
> 

Re: Libertas: Association request to the driver failed

[Dan Williams]

On Wed, 2009-08-12 at 11:15 -0500, Dan Williams wrote:
> 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.

Well, OK, it's not complicated, just obfuscated.
mrvl_ie_rates_param_set is a TLV structure, and the size of the overall
structure from 'header' will tell how many rates there actually are
following the header.  The [1] is left over from the vendor driver.  If
that's confusing things, can we just use [0] here or does the scanner
that found this need to be fixed?  We'll certainly be pointing past the
end of the mrvl_ie_rates_param_set, but we won't be accessing beyond
memory we don't control, since the mrvl_ie_rates_param_set will always
point into a buffer (from kzalloc) that's large enough.  Rates is also
never used late enough in the command spacing to be at risk of
overrunning the end of the command buffer into which it points.

The following (not runtime tested) should make it clearer what's going
on, though it doesn't address the [1]/[0] issue:

diff --git a/drivers/net/wireless/libertas/assoc.c b/drivers/net/wireless/libertas/assoc.c
index 1902b6f..8c05388 100644
--- a/drivers/net/wireless/libertas/assoc.c
+++ b/drivers/net/wireless/libertas/assoc.c
@@ -35,7 +35,8 @@ static const u8 bssid_off[ETH_ALEN]  __attribute__ ((aligned (2))) =
  *
  *  @param priv     A pointer to struct lbs_private structure
  *  @param rates       the buffer which keeps input and output
- *  @param rates_size  the size of rate1 buffer; new size of buffer on return
+ *  @param rates_size  the size of rates buffer; new size of buffer on return,
+ *                     which will be less than or equal to original rates_size
  *
  *  @return            0 on success, or -1 on error
  */
@@ -43,39 +44,42 @@ static int get_common_rates(struct lbs_private *priv,
 	u8 *rates,
 	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)];
-	size_t tmp_size = 0;
-
-	/* For each rate in card_rates that exists in rate1, copy to tmp */
-	for (i = 0; i < ARRAY_SIZE(lbs_bg_rates) && card_rates[i]; i++) {
-		for (j = 0; j < *rates_size && rates[j]; j++) {
-			if (rates[j] == card_rates[i])
-				tmp[tmp_size++] = card_rates[i];
+	int i, j;
+	u8 intersection[MAX_RATES];
+	u16 intersection_size;
+	u16 num_rates = 0;
+
+	intersection_size = min_t(u16, *rates_size, ARRAY_SIZE(intersection));
+
+	/* Allow each rate from 'rates' that is supported by the hardware */
+	for (i = 0; i < ARRAY_SIZE(lbs_bg_rates) && lbs_bg_rates[i]; i++) {
+		for (j = 0; j < intersection_size && rates[j]; j++) {
+			if (rates[j] == lbs_bg_rates[i])
+				intersection[num_rates++] = rates[j];
 		}
 	}
 
 	lbs_deb_hex(LBS_DEB_JOIN, "AP rates    ", rates, *rates_size);
-	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", card_rates,
+	lbs_deb_hex(LBS_DEB_JOIN, "card rates  ", lbs_bg_rates,
 			ARRAY_SIZE(lbs_bg_rates));
-	lbs_deb_hex(LBS_DEB_JOIN, "common rates", tmp, tmp_size);
+	lbs_deb_hex(LBS_DEB_JOIN, "common rates", intersection, num_rates);
 	lbs_deb_join("TX data rate 0x%02x\n", priv->cur_rate);
 
 	if (!priv->enablehwauto) {
-		for (i = 0; i < tmp_size; i++) {
-			if (tmp[i] == priv->cur_rate)
+		for (i = 0; i < num_rates; i++) {
+			if (intersection[i] == priv->cur_rate)
 				goto done;
 		}
 		lbs_pr_alert("Previously set fixed data rate %#x isn't "
 		       "compatible with the network.\n", priv->cur_rate);
-		ret = -1;
+		return -1;
 	}
+
 done:
 	memset(rates, 0, *rates_size);
-	*rates_size = min_t(int, tmp_size, *rates_size);
-	memcpy(rates, tmp, *rates_size);
-	return ret;
+	*rates_size = num_rates;
+	memcpy(rates, intersection, num_rates);
+	return 0;
 }
 
 
@@ -317,8 +321,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);
+	tmplen = min_t(u16, ARRAY_SIZE(bss->rates), MAX_RATES);
+	memcpy(&rates->rates, &bss->rates, tmplen);
 	if (get_common_rates(priv, rates->rates, &tmplen)) {
 		ret = -1;
 		goto done;
@@ -592,7 +596,7 @@ static int lbs_adhoc_join(struct lbs_private *priv,
 
 	/* Copy Data rates from the rates recorded in scan response */
 	memset(cmd.bss.rates, 0, sizeof(cmd.bss.rates));
-	ratesize = min_t(u16, ARRAY_SIZE(cmd.bss.rates), MAX_RATES);
+	ratesize = min_t(u16, ARRAY_SIZE(cmd.bss.rates), ARRAY_SIZE (bss->rates));
 	memcpy(cmd.bss.rates, bss->rates, ratesize);
 	if (get_common_rates(priv, cmd.bss.rates, &ratesize)) {
 		lbs_deb_join("ADHOC_JOIN: get_common_rates returned error.\n");

Re: Libertas: Association request to the driver failed

[Dan Williams]

On Fri, 2009-08-07 at 21:11 +0200, Daniel Mack wrote:
> Since a recent merge of the wireless tree to Linus' master, my SDIO
> connected libertas module fails to connect to our WLAN.
> 
> 'iwlist scanning' works fine, but wpa_supplicant keeps on spitting out
> the following message:
> 
>   CTRL-EVENT-SCAN-RESULTS 
>   Trying to associate with 00:04:0e:4b:c7:b3 (SSID='caiaq' freq=2437 MHz)
>   Association request to the driver failed
> 
> Haven't done any bisect or deeper inspection of recent changes yet. Can
> anyone point me in some direction maybe? The change must have been
> introduced between -rc3 and -rc5. My userspace did not change since
> then.

That error actually means nothing and I believe the log message has been
removed in recent supplicant versions.  It simply means that the driver
doesn't support some of the random ioctls that the supplicant tries, but
most of the time, that's not a problem.

What you can do for the time being is increase the association timeout
(which gets lowered to like 5 seconds if that message is printed because
the driver returned -1 from wpa_drv_associate()), and see if the card
does in fact associate after 10 or 15 seconds.  That is a useful point
of debugging information.

Dan