[PATCHv2] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[PATCHv2] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Helmut Schaa]

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 <hschaa@suse.de>
---

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;

Re: [PATCHv2] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Sat, May 17, 2008 at 9:35 PM, Helmut Schaa <hschaa@suse.de> 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 <hschaa@suse.de>
> ---
>
> 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

Re: [PATCHv2] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Dan Williams]

On Sun, 2008-05-18 at 13:37 +0300, Tomas Winkler wrote:
> On Sat, May 17, 2008 at 9:35 PM, Helmut Schaa <hschaa@suse.de> 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 <hschaa@suse.de>
> > ---
> >
> > 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Larry Finger]

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

Re: [PATCHv2] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Dan Williams]

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

Re: [PATCHv2] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Helmut Schaa]

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

Re: [PATCHv2] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]


> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCHv2] mac80211: fix NULL pointer dereference in ieee80211_compatible_rates

[Tomas Winkler]

On Mon, May 19, 2008 at 3:41 PM, Johannes Berg
<johannes@sipsolutions.net> 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
>