svn commit: r259175 - stable/10/sys/net80211

[Gavin Atkinson]

Author: gavin
Date: Tue Dec 10 13:42:59 2013
New Revision: 259175
URL: http://svnweb.freebsd.org/changeset/base/259175

Log:
  Merge r257065 (by adrian) from head:
  
    Fix a use-after-free node reference issue when waiting for a return
    from a management frame transmission.
  
    This bug is a bit loopy, so here goes.
  
    The underlying cause is pretty easy to understand - the node isn't
    referenced before passing into the callout, so if the node is deleted
    before the callout fires, it'll dereference free'd memory.
  
    The code path however is slightly more convoluted.
  
    The functions _say_ mgt_tx - ie management transmit - which is partially
    true.  Yes, that callback is attached to the mbuf for some management
    frames.  However, it's only for frames relating to scanning and
    authentication attempts.  It helpfully drives the VAP state back to
    "SCAN" if the transmission fails _OR_ (as I subsequently found out!)
    if the transmission succeeds but the state machine doesn't make progress
    towards being authenticated and active.
  
    Now, the code itself isn't terribly clear about this.
  
    It _looks_ like it's just handling the transmit failure case.
  
    However, when you look at what goes on in the transmit success case, it's
    moving the VAP state back to SCAN if it hasn't changed state since
    the time the callback was scheduled.  Ie, if it's in ASSOC or AUTH still,
    it'll go back to SCAN.  But if it has transitioned to the RUN state,
    the comparison will fail and it'll not transition things back to the
    SCAN state.
  
    So, to fix this, I decided to leave everything the way it is and merely
    fix the locking and remove the node reference.
  
    The _better_ fix would be to turn this callout into a "assoc/auth request"
    timeout callback and make the callout locked, thus eliminating all races.
    However, until all the drivers have been fixed so that transmit completions
    occur outside of any locking that's going on, it's going to be impossible
    to do this without introducing LORs.  So, I leave some of the evilness
    in there.
  
  Candidate for 10.0.

Modified:
  stable/10/sys/net80211/ieee80211_output.c
  stable/10/sys/net80211/ieee80211_proto.c
  stable/10/sys/net80211/ieee80211_proto.h
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/sys/net80211/ieee80211_output.c
==============================================================================
--- stable/10/sys/net80211/ieee80211_output.c	Tue Dec 10 13:38:39 2013	(r259174)
+++ stable/10/sys/net80211/ieee80211_output.c	Tue Dec 10 13:42:59 2013	(r259175)
@@ -2736,20 +2736,35 @@ ieee80211_alloc_cts(struct ieee80211com 
 static void
 ieee80211_tx_mgt_timeout(void *arg)
 {
-	struct ieee80211_node *ni = arg;
-	struct ieee80211vap *vap = ni->ni_vap;
+	struct ieee80211vap *vap = arg;
 
+	IEEE80211_LOCK(vap->iv_ic);
 	if (vap->iv_state != IEEE80211_S_INIT &&
 	    (vap->iv_ic->ic_flags & IEEE80211_F_SCAN) == 0) {
 		/*
 		 * NB: it's safe to specify a timeout as the reason here;
 		 *     it'll only be used in the right state.
 		 */
-		ieee80211_new_state(vap, IEEE80211_S_SCAN,
+		ieee80211_new_state_locked(vap, IEEE80211_S_SCAN,
 			IEEE80211_SCAN_FAIL_TIMEOUT);
 	}
+	IEEE80211_UNLOCK(vap->iv_ic);
 }
 
+/*
+ * This is the callback set on net80211-sourced transmitted
+ * authentication request frames.
+ *
+ * This does a couple of things:
+ *
+ * + If the frame transmitted was a success, it schedules a future
+ *   event which will transition the interface to scan.
+ *   If a state transition _then_ occurs before that event occurs,
+ *   said state transition will cancel this callout.
+ *
+ * + If the frame transmit was a failure, it immediately schedules
+ *   the transition back to scan.
+ */
 static void
 ieee80211_tx_mgt_cb(struct ieee80211_node *ni, void *arg, int status)
 {
@@ -2767,10 +2782,11 @@ ieee80211_tx_mgt_cb(struct ieee80211_nod
 	 *
 	 * XXX what happens if !acked but response shows up before callback?
 	 */
-	if (vap->iv_state == ostate)
+	if (vap->iv_state == ostate) {
 		callout_reset(&vap->iv_mgtsend,
 			status == 0 ? IEEE80211_TRANS_WAIT*hz : 0,
-			ieee80211_tx_mgt_timeout, ni);
+			ieee80211_tx_mgt_timeout, vap);
+	}
 }
 
 static void

Modified: stable/10/sys/net80211/ieee80211_proto.c
==============================================================================
--- stable/10/sys/net80211/ieee80211_proto.c	Tue Dec 10 13:38:39 2013	(r259174)
+++ stable/10/sys/net80211/ieee80211_proto.c	Tue Dec 10 13:42:59 2013	(r259175)
@@ -107,8 +107,6 @@ static void update_promisc(void *, int);
 static void update_channel(void *, int);
 static void update_chw(void *, int);
 static void ieee80211_newstate_cb(void *, int);
-static int ieee80211_new_state_locked(struct ieee80211vap *,
-	enum ieee80211_state, int);
 
 static int
 null_raw_xmit(struct ieee80211_node *ni, struct mbuf *m,
@@ -1834,7 +1832,7 @@ done:
  * is usually a mistake and indicates lack of proper integration
  * with the net80211 layer.
  */
-static int
+int
 ieee80211_new_state_locked(struct ieee80211vap *vap,
 	enum ieee80211_state nstate, int arg)
 {

Modified: stable/10/sys/net80211/ieee80211_proto.h
==============================================================================
--- stable/10/sys/net80211/ieee80211_proto.h	Tue Dec 10 13:38:39 2013	(r259174)
+++ stable/10/sys/net80211/ieee80211_proto.h	Tue Dec 10 13:42:59 2013	(r259175)
@@ -332,6 +332,8 @@ void	ieee80211_dturbo_switch(struct ieee
 void	ieee80211_swbmiss(void *arg);
 void	ieee80211_beacon_miss(struct ieee80211com *);
 int	ieee80211_new_state(struct ieee80211vap *, enum ieee80211_state, int);
+int	ieee80211_new_state_locked(struct ieee80211vap *, enum ieee80211_state,
+		int);
 void	ieee80211_print_essid(const uint8_t *, int);
 void	ieee80211_dump_pkt(struct ieee80211com *,
 		const uint8_t *, int, int, int);