Fix a possible NULL pointer dereference in ieee80211_compatible_rates
introduced in the patch "mac80211: fix association with some APs".
Signed-off-by: Helmut Schaa <[email protected]>
---
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 76ad4ed..2642551 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -664,15 +664,22 @@ static int ieee80211_compatible_rates(struct
ieee80211_sta_bss *bss,
int i, j, count;
*rates = 0;
count = 0;
- for (i = 0; i < bss->supp_rates_len; i++) {
- int rate = (bss->supp_rates[i] & 0x7F) * 5;
- for (j = 0; j < sband->n_bitrates; j++)
- if (sband->bitrates[j].bitrate == rate) {
- *rates |= BIT(j);
- count++;
- break;
- }
+ if (bss) {
+ for (i = 0; i < bss->supp_rates_len; i++) {
+ int rate = (bss->supp_rates[i] & 0x7F) * 5;
+
+ for (j = 0; j < sband->n_bitrates; j++)
+ if (sband->bitrates[j].bitrate == rate) {
+ *rates |= BIT(j);
+ count++;
+ break;
+ }
+ }
+ } else {
+ for (i = 0; i < sband->n_bitrates; i++)
+ *rates |= BIT(i);
+ count = sband->n_bitrates;
}
return count;
On Sat, May 17, 2008 at 9:35 PM, Helmut Schaa <[email protected]> wrote:
> Fix a possible NULL pointer dereference in ieee80211_compatible_rates
> introduced in the patch "mac80211: fix association with some APs".
>
> Signed-off-by: Helmut Schaa <[email protected]>
> ---
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 76ad4ed..2642551 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -664,15 +664,22 @@ static int ieee80211_compatible_rates(struct
> ieee80211_sta_bss *bss,
> int i, j, count;
> *rates = 0;
> count = 0;
> - for (i = 0; i < bss->supp_rates_len; i++) {
> - int rate = (bss->supp_rates[i] & 0x7F) * 5;
>
> - for (j = 0; j < sband->n_bitrates; j++)
> - if (sband->bitrates[j].bitrate == rate) {
> - *rates |= BIT(j);
> - count++;
> - break;
> - }
> + if (bss) {
> + for (i = 0; i < bss->supp_rates_len; i++) {
> + int rate = (bss->supp_rates[i] & 0x7F) * 5;
> +
> + for (j = 0; j < sband->n_bitrates; j++)
> + if (sband->bitrates[j].bitrate == rate) {
> + *rates |= BIT(j);
> + count++;
> + break;
> + }
> + }
> + } else {
> + for (i = 0; i < sband->n_bitrates; i++)
> + *rates |= BIT(i);
> + count = sband->n_bitrates;
> }
>
> return count;
>
Again, mac should rather solve the problem that we do not connect to
AP's that are not in the BSS list
Personally I would NACK this as it's just hiding the real problem
The other issue is that we call ieee80211_rx_bss_put(dev, bss); before
this call in ieee80211_send_assoc.
Tomas
Dan Williams wrote:
> On Sun, 2008-05-18 at 13:37 +0300, Tomas Winkler wrote:
>> Again, mac should rather solve the problem that we do not connect to
>> AP's that are not in the BSS list
>> Personally I would NACK this as it's just hiding the real problem
>
> Ugh; is that what's happening? If the AP isn't in the BSS list of the
> card, then the card shouldn't be connecting to it. It should have been
> probe-scanned already and thus exist in the BSS list, right?
>
> Dan
>
>> The other issue is that we call ieee80211_rx_bss_put(dev, bss); before
>> this call in ieee80211_send_assoc.
This AP certainly should have been in the BSS list. As I stated in the
original post, the system had been connected to that AP for about 18 hours
before the NULL bss NULL event occurred.
Clearly the sequence of events is improbable and it may be some time before
it happens again. What diagnostic information would be useful to capture
if/when it occurs? Is there any other location that I should monitor?
Larry
On Sun, 2008-05-18 at 10:14 -0500, Larry Finger wrote:
> Dan Williams wrote:
> > On Sun, 2008-05-18 at 13:37 +0300, Tomas Winkler wrote:
> >> Again, mac should rather solve the problem that we do not connect to
> >> AP's that are not in the BSS list
> >> Personally I would NACK this as it's just hiding the real problem
> >
> > Ugh; is that what's happening? If the AP isn't in the BSS list of the
> > card, then the card shouldn't be connecting to it. It should have been
> > probe-scanned already and thus exist in the BSS list, right?
> >
> > Dan
> >
> >> The other issue is that we call ieee80211_rx_bss_put(dev, bss); before
> >> this call in ieee80211_send_assoc.
>
> This AP certainly should have been in the BSS list. As I stated in the
> original post, the system had been connected to that AP for about 18 hours
> before the NULL bss NULL event occurred.
>
> Clearly the sequence of events is improbable and it may be some time before
> it happens again. What diagnostic information would be useful to capture
> if/when it occurs? Is there any other location that I should monitor?
I'd think a printk whenever a unique BSS item is removed from the BSS
list; and the location of that removal in the code using __func__ and
__LINE__ in the output.
Dan
On Sun, 2008-05-18 at 13:37 +0300, Tomas Winkler wrote:
> On Sat, May 17, 2008 at 9:35 PM, Helmut Schaa <[email protected]> wrote:
> > Fix a possible NULL pointer dereference in ieee80211_compatible_rates
> > introduced in the patch "mac80211: fix association with some APs".
> >
> > Signed-off-by: Helmut Schaa <[email protected]>
> > ---
> >
> > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> > index 76ad4ed..2642551 100644
> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -664,15 +664,22 @@ static int ieee80211_compatible_rates(struct
> > ieee80211_sta_bss *bss,
> > int i, j, count;
> > *rates = 0;
> > count = 0;
> > - for (i = 0; i < bss->supp_rates_len; i++) {
> > - int rate = (bss->supp_rates[i] & 0x7F) * 5;
> >
> > - for (j = 0; j < sband->n_bitrates; j++)
> > - if (sband->bitrates[j].bitrate == rate) {
> > - *rates |= BIT(j);
> > - count++;
> > - break;
> > - }
> > + if (bss) {
> > + for (i = 0; i < bss->supp_rates_len; i++) {
> > + int rate = (bss->supp_rates[i] & 0x7F) * 5;
> > +
> > + for (j = 0; j < sband->n_bitrates; j++)
> > + if (sband->bitrates[j].bitrate == rate) {
> > + *rates |= BIT(j);
> > + count++;
> > + break;
> > + }
> > + }
> > + } else {
> > + for (i = 0; i < sband->n_bitrates; i++)
> > + *rates |= BIT(i);
> > + count = sband->n_bitrates;
> > }
> >
> > return count;
> >
>
> Again, mac should rather solve the problem that we do not connect to
> AP's that are not in the BSS list
> Personally I would NACK this as it's just hiding the real problem
Ugh; is that what's happening? If the AP isn't in the BSS list of the
card, then the card shouldn't be connecting to it. It should have been
probe-scanned already and thus exist in the BSS list, right?
Dan
> The other issue is that we call ieee80211_rx_bss_put(dev, bss); before
> this call in ieee80211_send_assoc.
>
> Tomas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 19, 2008 at 3:41 PM, Johannes Berg
<[email protected]> wrote:
>
>> Again, mac should rather solve the problem that we do not connect to
>> AP's that are not in the BSS list
>
> You're talking about this a lot without showing any willingness to work
> on it. Personally, I don't care so I'm not going to work on making this
> a valid assertion, obviously it is not.
You are absolutely right I can imagine how it sounds, just I wish I
had more time. So for now I'm just talking. If nobody will fix it till
I clean my table I will fix it.
>> The other issue is that we call ieee80211_rx_bss_put(dev, bss); before
>> this call in ieee80211_send_assoc.
>
> Indeed, we (i.e. not just me!) should have caught this at the time the
> original patch was merged.
:)
> Helmut, can you please make a patch that addresses both issues by
> instead moving the compatible rates check into the "if (bss)" part in
> ieee80211_send_assoc() right after ieee80211_rx_bss_get()? And you can
> make "u64 rates = ~0;" the default to address the !bss case.
>
Still not sure in what flow the bss can be removed from the list.
send_assoc() is called after mac has received successful
authentication response. I would like to see a trace of this. Is this
easily reproducible ?
Thanks
Tomas
> johannes
>
Am Sonntag, 18. Mai 2008 12:37:23 schrieb Tomas Winkler:
> Again, mac should rather solve the problem that we do not connect to
> AP's that are not in the BSS list
I just tried that approach (see patch below) and could not find a regression
at a first glance. Nevertheless I'm not sure in which situations a bss is
removed from the list and if this approach would cause a regression?
Regards,
Helmut
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 76ad4ed..8595bfd 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -678,7 +678,7 @@ static int ieee80211_compatible_rates(struct
ieee80211_sta_bss *bss,
return count;
}
-static void ieee80211_send_assoc(struct net_device *dev,
+static int ieee80211_send_assoc(struct net_device *dev,
struct ieee80211_if_sta *ifsta)
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
@@ -716,6 +716,10 @@ static void ieee80211_send_assoc(struct net_device *dev,
bss = ieee80211_rx_bss_get(dev, ifsta->bssid,
local->hw.conf.channel->center_freq,
ifsta->ssid, ifsta->ssid_len);
+
+ if (!bss)
+ return 1;
+
if (bss) {
if (bss->capability & WLAN_CAPABILITY_PRIVACY)
capab |= WLAN_CAPABILITY_PRIVACY;
@@ -829,6 +833,8 @@ static void ieee80211_send_assoc(struct net_device *dev,
memcpy(ifsta->assocreq_ies, ies, ifsta->assocreq_ies_len);
ieee80211_sta_tx(dev, skb, 0);
+
+ return 0;
}
@@ -945,7 +951,11 @@ static void ieee80211_associate(struct net_device *dev,
return;
}
- ieee80211_send_assoc(dev, ifsta);
+ if (ieee80211_send_assoc(dev, ifsta)) {
+ printk(KERN_DEBUG "%s: send association request failed", dev->name);
+ ifsta->state = IEEE80211_DISABLED;
+ return;
+ }
mod_timer(&ifsta->timer, jiffies + IEEE80211_ASSOC_TIMEOUT);
}
> Again, mac should rather solve the problem that we do not connect to
> AP's that are not in the BSS list
You're talking about this a lot without showing any willingness to work
on it. Personally, I don't care so I'm not going to work on making this
a valid assertion, obviously it is not.
> The other issue is that we call ieee80211_rx_bss_put(dev, bss); before
> this call in ieee80211_send_assoc.
Indeed, we (i.e. not just me!) should have caught this at the time the
original patch was merged.
Helmut, can you please make a patch that addresses both issues by
instead moving the compatible rates check into the "if (bss)" part in
ieee80211_send_assoc() right after ieee80211_rx_bss_get()? And you can
make "u64 rates = ~0;" the default to address the !bss case.
johannes