[PATCH 2.6.34] mac80211: Fix auth retries if AP sends temporary deauth

[PATCH 2.6.34] mac80211: Fix auth retries if AP sends temporary deauth

[Paul Stewart]

This bypasses destruction of BSS state if a temporary DEAUTH packet is
received while performing an AUTH.  This will allow the retry mechanism
(which runs regardless of this patch) to succeed, since we do not remove
the BSS state which is required to complete authentication on the client
side in cfg80211_send_rx_auth().

The specific case handled here is "Previous authentication no longer
valid", which is usually generated by an AP if the AP still has saved
state of the STA being authenticated.  Usually a retry will be successful.

Signed-off-by: Paul Stewart <pstew@google.com>
---
net/mac80211/work.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/work.c b/net/mac80211/work.c
index 15e1ba9..800929e 100644
--- a/net/mac80211/work.c
+++ b/net/mac80211/work.c
@@ -1006,7 +1006,7 @@ ieee80211_rx_result
ieee80211_work_rx_mgmt(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_mgmt *mgmt;
 	struct ieee80211_work *wk;
-	u16 fc;
+	u16 fc, reason_code;

 	if (skb->len < 24)
 		return RX_DROP_MONITOR;
@@ -1030,6 +1030,25 @@ ieee80211_rx_result
ieee80211_work_rx_mgmt(struct ieee80211_sub_if_data *sdata,
 			skb_queue_tail(&local->work_skb_queue, skb);
 			ieee80211_queue_work(&local->hw, &local->work_work);
 			return RX_QUEUED;
+		case IEEE80211_STYPE_DEAUTH:
+			/*
+			 * If we get sent a DEAUTH while we are
+			 * actively trying to authenticate to this
+			 * station, we shoot ourselves in the foot if
+			 * we fall through using RX_CONTINUE and allow
+			 * the bss context to disappear
+			 * (ieee80211_sta_rx_mgmt()).  This is
+			 * especially true if the reason for the
+			 * DEAUTH was a negative but temporary direct
+			 * response to an AUTH attempt. Let the retry
+			 * mechanism run its course instead.
+			 */
+                        reason_code = le16_to_cpu(mgmt->u.deauth.reason_code);
+			if (wk->type == IEEE80211_WORK_AUTH &&
+                            reason_code == WLAN_REASON_PREV_AUTH_NOT_VALID) {
+				return RX_DROP_MONITOR;
+			}
+			break;
 		}
 	}

Re: [PATCH 2.6.34] mac80211: Fix auth retries if AP sends temporary deauth

[Johannes Berg]

On Thu, 2010-07-01 at 10:21 -0700, Paul Stewart wrote:
> This bypasses destruction of BSS state if a temporary DEAUTH packet is
> received while performing an AUTH.  This will allow the retry mechanism
> (which runs regardless of this patch) to succeed, since we do not remove
> the BSS state which is required to complete authentication on the client
> side in cfg80211_send_rx_auth().
> 
> The specific case handled here is "Previous authentication no longer
> valid", which is usually generated by an AP if the AP still has saved
> state of the STA being authenticated.  Usually a retry will be successful.

You don't usually send patches to stable, you send them to the current
tree with a cc stable tag, and then they get picked up.

I'm going to look at the problem now, I'm still not really convinced by
this.

johannes

Re: [PATCH 2.6.34] mac80211: Fix auth retries if AP sends temporary deauth

[Johannes Berg]

On Thu, 2010-07-01 at 10:21 -0700, Paul Stewart wrote:
> @@ -1030,6 +1030,25 @@ ieee80211_rx_result
> ieee80211_work_rx_mgmt(struct ieee80211_sub_if_data *sdata,
>  			skb_queue_tail(&local->work_skb_queue, skb);
>  			ieee80211_queue_work(&local->hw, &local->work_work);
>  			return RX_QUEUED;
> +		case IEEE80211_STYPE_DEAUTH:
> +			/*
> +			 * If we get sent a DEAUTH while we are
> +			 * actively trying to authenticate to this
> +			 * station, we shoot ourselves in the foot if
> +			 * we fall through using RX_CONTINUE and allow
> +			 * the bss context to disappear
> +			 * (ieee80211_sta_rx_mgmt()).  This is
> +			 * especially true if the reason for the
> +			 * DEAUTH was a negative but temporary direct
> +			 * response to an AUTH attempt. Let the retry
> +			 * mechanism run its course instead.
> +			 */
> +                        reason_code = le16_to_cpu(mgmt->u.deauth.reason_code);
> +			if (wk->type == IEEE80211_WORK_AUTH &&
> +                            reason_code == WLAN_REASON_PREV_AUTH_NOT_VALID) {
> +				return RX_DROP_MONITOR;
> +			}
> +			break;

Ok, wow, I finally understand this patch, but is it weird!! You're
modifying work.c to avoid having the mlme.c code send this frame to
cfg80211? That's really confusing.

The real reason for this is that we send up the deauth frame even when
we're not even authenticated. This happens in mlme.c. Therefore, we
should improve the logic in ieee80211_sta_rx_queued_mgmt() to make sure
it only triggers when we're authenticated with the BSS?

Alternatively, since cfg80211 tracks this, it would be easier to modify
cfg80211_send_rx_auth() to not send the event to userspace in the !done
case I guess.

johannes

Re: [PATCH 2.6.34] mac80211: Fix auth retries if AP sends temporary deauth

[Paul Stewart]

It may be a weird patch, but probably just because I'm still wrapping
my head around how things work.  The problem is ultimately the call to
__cfg80211_send_deauth() in wireless/mlme.c that is triggered by
reception of the DEAUTH.  That function removes wdev->auth_bsses[i],
which is needed in order for an auth to succeed.  The code path that
gets us there is:

    mac80211/rx.c: ieee80211_rx_h_mgmt()
    mac80211/mlme.c: ieee80211_sta_rx_mgmt()
    ...then through the queued work and...
    ieee80211_sta_rx_queued_mgmt()

At the bottom of the latter function, outside of the block that checks
for our authentication state, we call cfg80211_send_deauth() in
response to IEEE80211_STYPE_DEAUTH, which quite arguably should never
be called if we're authenticated.  The only time this issue touches
cfg80211 is that final call to send_deauth() which I believe is done
in error.  I think the fix should be in mac80211 somewhere.

I didn't find a way to tell where we were in the authentication proces
from within ieee80211_sta_rx_queued_mgmt(), so I swallowed the packet
much earlier in the process from within ieee80211_work_rx_mgmt(),
which has access to that state, and can indeed claim packets for
itself it it believes it knows best what to do with them.

I hope this clears up my thinking on this.  I'd be happy to change the
patch in whatever way makes sense.

--
Paul

On Fri, Jul 2, 2010 at 10:29 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2010-07-01 at 10:21 -0700, Paul Stewart wrote:
>> @@ -1030,6 +1030,25 @@ ieee80211_rx_result
>> ieee80211_work_rx_mgmt(struct ieee80211_sub_if_data *sdata,
>>                       skb_queue_tail(&local->work_skb_queue, skb);
>>                       ieee80211_queue_work(&local->hw, &local->work_work);
>>                       return RX_QUEUED;
>> +             case IEEE80211_STYPE_DEAUTH:
>> +                     /*
>> +                      * If we get sent a DEAUTH while we are
>> +                      * actively trying to authenticate to this
>> +                      * station, we shoot ourselves in the foot if
>> +                      * we fall through using RX_CONTINUE and allow
>> +                      * the bss context to disappear
>> +                      * (ieee80211_sta_rx_mgmt()).  This is
>> +                      * especially true if the reason for the
>> +                      * DEAUTH was a negative but temporary direct
>> +                      * response to an AUTH attempt. Let the retry
>> +                      * mechanism run its course instead.
>> +                      */
>> +                        reason_code = le16_to_cpu(mgmt->u.deauth.reason_code);
>> +                     if (wk->type == IEEE80211_WORK_AUTH &&
>> +                            reason_code == WLAN_REASON_PREV_AUTH_NOT_VALID) {
>> +                             return RX_DROP_MONITOR;
>> +                     }
>> +                     break;
>
> Ok, wow, I finally understand this patch, but is it weird!! You're
> modifying work.c to avoid having the mlme.c code send this frame to
> cfg80211? That's really confusing.
>
> The real reason for this is that we send up the deauth frame even when
> we're not even authenticated. This happens in mlme.c. Therefore, we
> should improve the logic in ieee80211_sta_rx_queued_mgmt() to make sure
> it only triggers when we're authenticated with the BSS?
>
> Alternatively, since cfg80211 tracks this, it would be easier to modify
> cfg80211_send_rx_auth() to not send the event to userspace in the !done
> case I guess.
>
> johannes
>
>

Re: [PATCH 2.6.34] mac80211: Fix auth retries if AP sends temporary deauth

[Johannes Berg]

On Fri, 2010-07-02 at 11:09 -0700, Paul Stewart wrote:
> It may be a weird patch, but probably just because I'm still wrapping
> my head around how things work.  

Sure.

> The problem is ultimately the call to
> __cfg80211_send_deauth() in wireless/mlme.c that is triggered by
> reception of the DEAUTH.  

Is it? I thought it was without double underscore from another code
path?

> That function removes wdev->auth_bsses[i],

No, the BSS can't be in auth_bsses yet. I think the problem is more
complex, and wpa_supplicant itself will remove it from auth_bsses
because the kernel erroneously told it we got deauthenticated.

> which is needed in order for an auth to succeed.  The code path that
> gets us there is:
> 
>     mac80211/rx.c: ieee80211_rx_h_mgmt()
>     mac80211/mlme.c: ieee80211_sta_rx_mgmt()
>     ...then through the queued work and...
>     ieee80211_sta_rx_queued_mgmt()
> 
> At the bottom of the latter function, outside of the block that checks
> for our authentication state, we call cfg80211_send_deauth() in
> response to IEEE80211_STYPE_DEAUTH, which quite arguably should never
> be called if we're authenticated.  The only time this issue touches
> cfg80211 is that final call to send_deauth() which I believe is done
> in error.  I think the fix should be in mac80211 somewhere.
> 
> I didn't find a way to tell where we were in the authentication proces
> from within ieee80211_sta_rx_queued_mgmt(), so I swallowed the packet
> much earlier in the process from within ieee80211_work_rx_mgmt(),
> which has access to that state, and can indeed claim packets for
> itself it it believes it knows best what to do with them.
> 
> I hope this clears up my thinking on this.  I'd be happy to change the
> patch in whatever way makes sense.

Can you try the patch below instead of yours? I'll explain it a bit more
later, but my church wedding ceremony is tomorrow :)

johannes

--- wireless-testing.orig/net/wireless/mlme.c	2010-07-02 20:12:19.000000000 +0200
+++ wireless-testing/net/wireless/mlme.c	2010-07-02 20:12:43.000000000 +0200
@@ -44,10 +44,10 @@ void cfg80211_send_rx_auth(struct net_de
 		}
 	}
 
-	WARN_ON(!done);
-
-	nl80211_send_rx_auth(rdev, dev, buf, len, GFP_KERNEL);
-	cfg80211_sme_rx_auth(dev, buf, len);
+	if (done) {
+		nl80211_send_rx_auth(rdev, dev, buf, len, GFP_KERNEL);
+		cfg80211_sme_rx_auth(dev, buf, len);
+	}
 
 	wdev_unlock(wdev);
 }

Re: [PATCH 2.6.34] mac80211: Fix auth retries if AP sends temporary deauth

[Paul Stewart]

Sure enough, your patch works well.  The end effect is correct, in
that despite the reception of the DEAUTH, we are successfully able to
process a successful auth response on the second try.  Do I take it
that this change will make it upstream since this is the case?  Have a
happy wedding, and no git merge until after the ceremony! :-)

--
Paul

On Fri, Jul 2, 2010 at 11:13 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2010-07-02 at 11:09 -0700, Paul Stewart wrote:
> Can you try the patch below instead of yours? I'll explain it a bit more
> later, but my church wedding ceremony is tomorrow :)
>
> johannes
>
> --- wireless-testing.orig/net/wireless/mlme.c   2010-07-02 20:12:19.000000000 +0200
> +++ wireless-testing/net/wireless/mlme.c        2010-07-02 20:12:43.000000000 +0200
> @@ -44,10 +44,10 @@ void cfg80211_send_rx_auth(struct net_de
>                }
>        }
>
> -       WARN_ON(!done);
> -
> -       nl80211_send_rx_auth(rdev, dev, buf, len, GFP_KERNEL);
> -       cfg80211_sme_rx_auth(dev, buf, len);
> +       if (done) {
> +               nl80211_send_rx_auth(rdev, dev, buf, len, GFP_KERNEL);
> +               cfg80211_sme_rx_auth(dev, buf, len);
> +       }
>
>        wdev_unlock(wdev);
>  }
>
>
>

[PATCH] cfg80211: ignore spurious deauth

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Ever since mac80211/drivers are no longer
fully in charge of keeping track of the
auth status, trying to make them do so will
fail. Instead of warning and reporting the
deauthentication to userspace, cfg80211 must
simply ignore it so that spurious
deauthentications, e.g. before starting
authentication, aren't seen by userspace as
actual deauthentications.

Cc: stable@kernel.org
Reported-by: Paul Stewart <pstew@google.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/mlme.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- wireless-testing.orig/net/wireless/mlme.c	2010-07-12 14:34:22.000000000 +0200
+++ wireless-testing/net/wireless/mlme.c	2010-07-12 14:42:27.000000000 +0200
@@ -44,10 +44,10 @@ void cfg80211_send_rx_auth(struct net_de
 		}
 	}
 
-	WARN_ON(!done);
-
-	nl80211_send_rx_auth(rdev, dev, buf, len, GFP_KERNEL);
-	cfg80211_sme_rx_auth(dev, buf, len);
+	if (done) {
+		nl80211_send_rx_auth(rdev, dev, buf, len, GFP_KERNEL);
+		cfg80211_sme_rx_auth(dev, buf, len);
+	}
 
 	wdev_unlock(wdev);
 }

Re: [PATCH] cfg80211: ignore spurious deauth

[Paul Stewart]

As stated in the other thread, this patch solves the issue I was
running into.  Thanks!

--
Paul

On Mon, Jul 12, 2010 at 5:46 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Ever since mac80211/drivers are no longer
> fully in charge of keeping track of the
> auth status, trying to make them do so will
> fail. Instead of warning and reporting the
> deauthentication to userspace, cfg80211 must
> simply ignore it so that spurious
> deauthentications, e.g. before starting
> authentication, aren't seen by userspace as
> actual deauthentications.
>
> Cc: stable@kernel.org
> Reported-by: Paul Stewart <pstew@google.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  net/wireless/mlme.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> --- wireless-testing.orig/net/wireless/mlme.c   2010-07-12 14:34:22.000000000 +0200
> +++ wireless-testing/net/wireless/mlme.c        2010-07-12 14:42:27.000000000 +0200
> @@ -44,10 +44,10 @@ void cfg80211_send_rx_auth(struct net_de
>                }
>        }
>
> -       WARN_ON(!done);
> -
> -       nl80211_send_rx_auth(rdev, dev, buf, len, GFP_KERNEL);
> -       cfg80211_sme_rx_auth(dev, buf, len);
> +       if (done) {
> +               nl80211_send_rx_auth(rdev, dev, buf, len, GFP_KERNEL);
> +               cfg80211_sme_rx_auth(dev, buf, len);
> +       }
>
>        wdev_unlock(wdev);
>  }
>
>
>