diff mbox

[6/7] TDLS: remove peer from global peer-list on free

Message ID 1402424350-19261-6-git-send-email-ilan.peer@intel.com
State Changes Requested
Headers show

Commit Message

Ilan Peer June 10, 2014, 6:19 p.m. UTC 
From: Arik Nemtsov <arik@wizery.com>

Also fix a small bug where a peer was used after free.

Signed-off-by: Arik Nemtsov <arikx.nemtsov@intel.com>
---
 src/rsn_supp/tdls.c |   32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

Comments

Jouni Malinen June 16, 2014, 8:53 p.m. UTC | #1 
On Tue, Jun 10, 2014 at 09:19:09PM +0300, Ilan Peer wrote:
> From: Arik Nemtsov <arik@wizery.com>
> 
> Also fix a small bug where a peer was used after free.

Hmm.. Could you please clarify where that bug is? I'd assume this was
referring to the addition of the tmp pointer here:

>  void wpa_tdls_teardown_peers(struct wpa_sm *sm)
>  {
> -	struct wpa_tdls_peer *peer;
> +	struct wpa_tdls_peer *peer, *tmp;
>  
>  	peer = sm->tdls;
>  
>  	wpa_printf(MSG_DEBUG, "TDLS: Tear down peers");
>  
>  	while (peer) {
> +		tmp = peer->next;
>  		wpa_printf(MSG_DEBUG, "TDLS: Tear down peer " MACSTR,
>  			   MAC2STR(peer->addr));
>  		if (sm->tdls_external_setup)
> @@ -2634,7 +2660,7 @@ void wpa_tdls_teardown_peers(struct wpa_sm *sm)
>  		else
>  			wpa_sm_tdls_oper(sm, TDLS_TEARDOWN, peer->addr);
>  
> -		peer = peer->next;
> +		peer = tmp;
>  	}

But that would not be use after free before the other parts of this
patch were applied (wpa_tdls_peer_free() does not currently free the
peer data, it only clears number of items in it).

Did I miss something else that would be using freed memory?
Arik Nemtsov June 17, 2014, 6:25 a.m. UTC | #2 
On Mon, Jun 16, 2014 at 11:53 PM, Jouni Malinen <j@w1.fi> wrote:
> On Tue, Jun 10, 2014 at 09:19:09PM +0300, Ilan Peer wrote:
>> From: Arik Nemtsov <arik@wizery.com>
>>
>> Also fix a small bug where a peer was used after free.
>
> Hmm.. Could you please clarify where that bug is? I'd assume this was
> referring to the addition of the tmp pointer here:
>
>>  void wpa_tdls_teardown_peers(struct wpa_sm *sm)
>>  {
>> -     struct wpa_tdls_peer *peer;
>> +     struct wpa_tdls_peer *peer, *tmp;
>>
>>       peer = sm->tdls;
>>
>>       wpa_printf(MSG_DEBUG, "TDLS: Tear down peers");
>>
>>       while (peer) {
>> +             tmp = peer->next;
>>               wpa_printf(MSG_DEBUG, "TDLS: Tear down peer " MACSTR,
>>                          MAC2STR(peer->addr));
>>               if (sm->tdls_external_setup)
>> @@ -2634,7 +2660,7 @@ void wpa_tdls_teardown_peers(struct wpa_sm *sm)
>>               else
>>                       wpa_sm_tdls_oper(sm, TDLS_TEARDOWN, peer->addr);
>>
>> -             peer = peer->next;
>> +             peer = tmp;
>>       }
>
> But that would not be use after free before the other parts of this
> patch were applied (wpa_tdls_peer_free() does not currently free the
> peer data, it only clears number of items in it).
>
> Did I miss something else that would be using freed memory?

No you're correct. Before, it wasn't a use-after-free per-se, since
data wasn't freed.
My wording was not accurate. But I'd argue that it's nicer to use "tmp" anyway..

Arik
Jouni Malinen June 17, 2014, 2:21 p.m. UTC | #3 
On Tue, Jun 17, 2014 at 09:25:31AM +0300, Arik Nemtsov wrote:
> No you're correct. Before, it wasn't a use-after-free per-se, since
> data wasn't freed.

OK, thanks.

> My wording was not accurate. But I'd argue that it's nicer to use "tmp" anyway..

Sure, that's fine. However, this patch introduces number of cases were
freed memory is accessed. Have you tried running this against the hwsim
test cases? I would strongly recommend doing so for new contributions
especially when changing allocation style. As an example, wpa_supplicant
for wlan1 would crash in ap_wpa2_tdls_concurrent_init. More generally,
any path where wpa_tdls_disable_peer_link(sm, peer) is followed by
anything dereferencing the peer point will break. There are multiple
such cases in tdls.c.
Arik Nemtsov June 17, 2014, 2:50 p.m. UTC | #4 
On Tue, Jun 17, 2014 at 5:21 PM, Jouni Malinen <j@w1.fi> wrote:
> On Tue, Jun 17, 2014 at 09:25:31AM +0300, Arik Nemtsov wrote:
>> No you're correct. Before, it wasn't a use-after-free per-se, since
>> data wasn't freed.
>
> OK, thanks.
>
>> My wording was not accurate. But I'd argue that it's nicer to use "tmp" anyway..
>
> Sure, that's fine. However, this patch introduces number of cases were
> freed memory is accessed. Have you tried running this against the hwsim
> test cases? I would strongly recommend doing so for new contributions
> especially when changing allocation style. As an example, wpa_supplicant
> for wlan1 would crash in ap_wpa2_tdls_concurrent_init. More generally,
> any path where wpa_tdls_disable_peer_link(sm, peer) is followed by
> anything dereferencing the peer point will break. There are multiple
> such cases in tdls.c.

You're right. I actually have an internal patch for that, but we'll do
some more testing to make sure we didn't miss any of the cases.

Arik
diff mbox

Patch

diff --git a/src/rsn_supp/tdls.c b/src/rsn_supp/tdls.c
index d8f7a47..96417d2 100644
--- a/src/rsn_supp/tdls.c
+++ b/src/rsn_supp/tdls.c
@@ -635,6 +635,28 @@  static void wpa_tdls_tpk_timeout(void *eloop_ctx, void *timeout_ctx)
 }
 
 
+static void wpa_tdls_peer_remove_from_list(struct wpa_sm *sm,
+					   struct wpa_tdls_peer *peer)
+{
+	struct wpa_tdls_peer *cur, *prev;
+
+	for (cur = sm->tdls, prev = NULL; cur && cur != peer;
+	     prev = cur, cur = cur->next)
+		;
+
+	if (cur != peer) {
+		wpa_printf(MSG_ERROR, "TDLS: could not find peer " MACSTR,
+			   MAC2STR(peer->addr));
+		return;
+	}
+
+	if (prev)
+		prev->next = peer->next;
+	else
+		sm->tdls = peer->next;
+}
+
+
 static void wpa_tdls_peer_free(struct wpa_sm *sm, struct wpa_tdls_peer *peer)
 {
 	wpa_printf(MSG_DEBUG, "TDLS: Clear state for peer " MACSTR,
@@ -664,6 +686,9 @@  static void wpa_tdls_peer_free(struct wpa_sm *sm, struct wpa_tdls_peer *peer)
 	os_memset(&peer->tpk, 0, sizeof(peer->tpk));
 	os_memset(peer->inonce, 0, WPA_NONCE_LEN);
 	os_memset(peer->rnonce, 0, WPA_NONCE_LEN);
+
+	wpa_tdls_peer_remove_from_list(sm, peer);
+	os_free(peer);
 }
 
 
@@ -2619,13 +2644,14 @@  int wpa_tdls_init(struct wpa_sm *sm)
 
 void wpa_tdls_teardown_peers(struct wpa_sm *sm)
 {
-	struct wpa_tdls_peer *peer;
+	struct wpa_tdls_peer *peer, *tmp;
 
 	peer = sm->tdls;
 
 	wpa_printf(MSG_DEBUG, "TDLS: Tear down peers");
 
 	while (peer) {
+		tmp = peer->next;
 		wpa_printf(MSG_DEBUG, "TDLS: Tear down peer " MACSTR,
 			   MAC2STR(peer->addr));
 		if (sm->tdls_external_setup)
@@ -2634,7 +2660,7 @@  void wpa_tdls_teardown_peers(struct wpa_sm *sm)
 		else
 			wpa_sm_tdls_oper(sm, TDLS_TEARDOWN, peer->addr);
 
-		peer = peer->next;
+		peer = tmp;
 	}
 }
 
@@ -2644,7 +2670,6 @@  static void wpa_tdls_remove_peers(struct wpa_sm *sm)
 	struct wpa_tdls_peer *peer, *tmp;
 
 	peer = sm->tdls;
-	sm->tdls = NULL;
 
 	while (peer) {
 		int res;
@@ -2653,7 +2678,6 @@  static void wpa_tdls_remove_peers(struct wpa_sm *sm)
 		wpa_printf(MSG_DEBUG, "TDLS: Remove peer " MACSTR " (res=%d)",
 			   MAC2STR(peer->addr), res);
 		wpa_tdls_peer_free(sm, peer);
-		os_free(peer);
 		peer = tmp;
 	}
 }