linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Williams <dcbw@redhat.com>
To: "John W. Linville" <linville@tuxdriver.com>
Cc: Roel Kluin <roel.kluin@gmail.com>, Daniel Mack <daniel@caiaq.de>,
	libertas-dev@lists.infradead.org, linux-wireless@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: Libertas: Association request to the driver failed
Date: Wed, 12 Aug 2009 12:34:17 -0500	[thread overview]
Message-ID: <1250098457.31650.79.camel@localhost.localdomain> (raw)
In-Reply-To: <1250093737.31650.17.camel@localhost.localdomain>

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



  reply	other threads:[~2009-08-12 17:34 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-08-07 19:11 Daniel Mack
2009-08-07 19:36 ` John W. Linville
2009-08-07 20:50   ` Marek Vasut
2009-08-08 12:35   ` Daniel Mack
2009-08-08 14:24     ` Roel Kluin
2009-08-09  9:23       ` Roel Kluin
2009-08-09 10:24         ` Daniel Mack
2009-08-09 11:11           ` Roel Kluin
2009-08-09 11:10             ` Michael Buesch
2009-08-09 19:13               ` Cyrill Gorcunov
2009-08-10 10:37                 ` Roel Kluin
2009-08-10 14:04                   ` Cyrill Gorcunov
2009-08-10 17:47                   ` Daniel Mack
2009-08-12  8:17                     ` Jonathan Cameron
2009-08-12  8:47                   ` Jonathan Cameron
2009-08-12 16:16                     ` Dan Williams
2009-08-10 17:59             ` John W. Linville
2009-08-11  7:02               ` Roel Kluin
2009-08-11 18:24                 ` John W. Linville
2009-08-12 16:15                   ` Dan Williams
2009-08-12 17:34                     ` Dan Williams [this message]
2009-08-07 21:21 ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1250098457.31650.79.camel@localhost.localdomain \
    --to=dcbw@redhat.com \
    --cc=daniel@caiaq.de \
    --cc=libertas-dev@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=linville@tuxdriver.com \
    --cc=roel.kluin@gmail.com \
    --subject='Re: Libertas: Association request to the driver failed' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).