diff mbox

[v4,01/25] Remove WPA per-VLAN groups when all stations left on rekeying

Message ID 20130727195431.17627.12831.stgit@ray-controller
State Superseded
Headers show

Commit Message

michael-dev July 27, 2013, 7:54 p.m. UTC 
This adds a references counter to struct wpa_group and frees
a group if it is unused.

This is useful when extending the number of VLANs supported.

Signed-hostap: Michael Braun <michael-dev@fami-braun.de>

V3: Freeing is not done in rekeying timer anymore.
---
 src/ap/wpa_auth.c   |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ap/wpa_auth_i.h |    2 +
 2 files changed, 73 insertions(+)

Comments

Jouni Malinen Aug. 4, 2013, 6:33 p.m. UTC | #1 
On Sat, Jul 27, 2013 at 09:54:31PM +0200, Michael Braun wrote:
> This adds a references counter to struct wpa_group and frees
> a group if it is unused.
> 
> This is useful when extending the number of VLANs supported.
> 
> Signed-hostap: Michael Braun <michael-dev@fami-braun.de>
> 
> V3: Freeing is not done in rekeying timer anymore.

Could you please clarify why there is an eloop timeout used in this
design? The commit log does not mention that at all and the code looks
overly complex as-is unless there is some clear need for this five
second timeout. Is the goal here to try to avoid removing the group if
another STA starts using it in five seconds? Is this really worth the
extra complexity?
michael-dev Aug. 4, 2013, 7:15 p.m. UTC | #2 
Hi,

Am 04.08.2013 20:33, schrieb Jouni Malinen:
> On Sat, Jul 27, 2013 at 09:54:31PM +0200, Michael Braun wrote:
>> This adds a references counter to struct wpa_group and frees
>> a group if it is unused.
> Could you please clarify why there is an eloop timeout used in this
> design?

basically its there because I was afraid of causing a NULL pointer 
dereference while writing that code. That was because I was unsure if 
that station vlan removal code could ever happen to be called indirectly 
while somebody is still accessing that very group - for example on of 
the iterators in wpa_auth.c (i.e. wpa_rekey_gtk). Using a timeout makes 
sure that the function is called directly from the eloop, so there 
wouldn't be any other context around.
On the other hand, such a call seems unlikely to me now - though the 
wpa_auth_sta_set_vlan code might need to be updated.

I'll send a refreshed version.

Regards,
  M. Braun
michael-dev Aug. 4, 2013, 9:21 p.m. UTC | #3 
-------- Ursprüngliche Nachricht --------
Von: michael-dev <michael-dev@fami-braun.de>
Gesendet: Sun Aug 04 21:15:30 MESZ 2013
An: hostap@lists.shmoo.com, projekt-wlan@fem.tu-ilmenau.de
Betreff: Re: [Projekt-wlan] [PATCH v4 01/25] Remove WPA per-VLAN groups when all stations left on rekeying

Hi,

> a NULL pointer dereference

that ought to be a virtual use-after-free I seeked to avoid.

Regards,
M. Braun
diff mbox

Patch

diff --git a/src/ap/wpa_auth.c b/src/ap/wpa_auth.c
index 83cc857..af7250e 100644
--- a/src/ap/wpa_auth.c
+++ b/src/ap/wpa_auth.c
@@ -41,6 +41,11 @@  static int wpa_gtk_update(struct wpa_authenticator *wpa_auth,
 			  struct wpa_group *group);
 static int wpa_group_config_group_keys(struct wpa_authenticator *wpa_auth,
 				       struct wpa_group *group);
+static void wpa_group_free(void *eloop_ctx, void *timeout_ctx);
+static void wpa_group_get(struct wpa_authenticator *wpa_auth,
+                          struct wpa_group *group);
+static void wpa_group_put(struct wpa_authenticator *wpa_auth,
+                          struct wpa_group *group);
 
 static const u32 dot11RSNAConfigGroupUpdateCount = 4;
 static const u32 dot11RSNAConfigPairwiseUpdateCount = 4;
@@ -52,6 +57,7 @@  static const u32 eapol_key_timeout_first_group = 500; /* ms */
 static const int dot11RSNAConfigPMKLifetime = 43200;
 static const int dot11RSNAConfigPMKReauthThreshold = 70;
 static const int dot11RSNAConfigSATimeout = 60;
+static const int groupFreeTimeout = 5;
 
 
 static inline int wpa_auth_mic_failure_report(
@@ -467,6 +473,7 @@  void wpa_deinit(struct wpa_authenticator *wpa_auth)
 	while (group) {
 		prev = group;
 		group = group->next;
+		eloop_cancel_timeout(wpa_group_free, wpa_auth, group);
 		os_free(prev);
 	}
 
@@ -519,6 +526,7 @@  wpa_auth_sta_init(struct wpa_authenticator *wpa_auth, const u8 *addr)
 
 	sm->wpa_auth = wpa_auth;
 	sm->group = wpa_auth->group;
+	wpa_group_get(sm->wpa_auth, sm->group);
 
 	return sm;
 }
@@ -581,6 +589,7 @@  static void wpa_free_sta_sm(struct wpa_state_machine *sm)
 #endif /* CONFIG_IEEE80211R */
 	os_free(sm->last_rx_eapol_key);
 	os_free(sm->wpa_ie);
+	wpa_group_put(sm->wpa_auth, sm->group);
 	os_free(sm);
 }
 
@@ -2960,6 +2969,65 @@  void wpa_auth_pmksa_remove(struct wpa_authenticator *wpa_auth,
 }
 
 
+/**
+ * Remove and free the group from wpa_authenticator.
+ * This is triggered by a callback to make sure nobody
+ * is currently iterating the group list while it gets modified.
+ */
+static void wpa_group_free(void *eloop_ctx, void *timeout_ctx)
+{
+	struct wpa_authenticator *wpa_auth = eloop_ctx;
+	struct wpa_group *group = timeout_ctx;
+	struct wpa_group *prev = wpa_auth->group;
+
+	wpa_printf(MSG_DEBUG, "WPA: Remove group state machine for VLAN-ID %d",
+		   group->vlan_id);
+
+	while (prev) {
+		if (prev->next == group) {
+			/* This never frees the special first group as needed */
+			prev->next = group->next;
+			os_free(group);
+			break;
+		}
+		prev = prev->next;
+	}
+
+}
+
+
+static void
+wpa_group_get(struct wpa_authenticator *wpa_auth, struct wpa_group *group)
+{
+	// skip the special first group
+	if (wpa_auth->group == group)
+		return;
+
+	if (group->references == -1) {
+		eloop_cancel_timeout(wpa_group_free, wpa_auth, group);
+		group->references = 0;
+	}
+
+	group->references++;
+}
+
+
+static void
+wpa_group_put(struct wpa_authenticator *wpa_auth, struct wpa_group *group)
+{
+	// skip the special first group
+	if (wpa_auth->group == group)
+		return;
+
+	group->references--;
+	if (group->references != 0)
+		return;
+	eloop_register_timeout(groupFreeTimeout, 0, wpa_group_free, wpa_auth,
+	                       group);
+	group->references = -1;
+}
+
+
 static struct wpa_group *
 wpa_auth_add_group(struct wpa_authenticator *wpa_auth, int vlan_id)
 {
@@ -3007,7 +3075,10 @@  int wpa_auth_sta_set_vlan(struct wpa_state_machine *sm, int vlan_id)
 	wpa_printf(MSG_DEBUG, "WPA: Moving STA " MACSTR " to use group state "
 		   "machine for VLAN ID %d", MAC2STR(sm->addr), vlan_id);
 
+	wpa_group_put(sm->wpa_auth, sm->group);
 	sm->group = group;
+	wpa_group_get(sm->wpa_auth, sm->group);
+
 	return 0;
 }
 
diff --git a/src/ap/wpa_auth_i.h b/src/ap/wpa_auth_i.h
index 97489d3..82e6e3a 100644
--- a/src/ap/wpa_auth_i.h
+++ b/src/ap/wpa_auth_i.h
@@ -151,6 +151,8 @@  struct wpa_group {
 	u8 IGTK[2][WPA_IGTK_LEN];
 	int GN_igtk, GM_igtk;
 #endif /* CONFIG_IEEE80211W */
+	/* number of references except those in struct wpa_group->next */
+	int references;
 };