diff mbox

P2P: Race condition in GO-NEG process if both peers enter p2p_connect at the same time

Message ID F764D07D6734DF489C733939E1A6F85A060C49@SJEXCHMB12.corp.ad.broadcom.com
State Accepted
Commit a1d2ab329e839838a623e1dd8c9fc77e97cd6355
Headers show

Commit Message

Neeraj Garg May 21, 2012, 6:28 a.m. UTC 
Hello,

We hit a case where both the peers assumed that other peer will be GO. Let us assume that p2p_connect command was given on both the peers using a script at the same time. Also assume that P1 has higher mac address than P2. 
1. P1 will send a GO-NEG-REQ and P2 will also send a GO-NEG-REQ.
2. Before P2 could get a callback p2p_go_neg_req_cb to update the variable go_neg_req_sent, P2 receives a GO-NEG request of P1 in the dwell time of its own request.
3. So P2 prepares the GO-NEG-RSP and send it even though its mac address is lower than P1 because go_neg_req_sent variable is NOT yet incremented.
4. Now P1 will get P2's GO-NEG-REQ and will reply it since it has higher mac address.
5. Both peers end up in sending GO-CONF frame.

To resolve this race, we propose that we increment go_neg_req_sent as soon as p2p_send_action is called for GO-NEG-REQ. And then decrement go_neg_req_sent if because of some reason the success is not reported in the callback p2p_go_neg_req_cb.

From 65193927d34fbbb9e84109b450268150867adbe5 Mon Sep 17 00:00:00 2001
From: Neeraj Garg <neerajkg@broadcom.com>
Date: Mon, 21 May 2012 11:53:46 +0530
Subject: [PATCH] P2P: Race condition in GO-NEG process if both peers enter p2p_connect at the same time
 Signed-off-by: Neeraj Garg <neerajkg@broadcom.com>

---
 src/p2p/p2p.c        |    4 +++-
 src/p2p/p2p_go_neg.c |    1 +
 2 files changed, 4 insertions(+), 1 deletions(-)
 mode change 100644 => 100755 src/p2p/p2p.c
 mode change 100644 => 100755 src/p2p/p2p_go_neg.c

Comments

Jouni Malinen June 9, 2012, 3:43 p.m. UTC | #1 
On Mon, May 21, 2012 at 06:28:55AM +0000, Neeraj Kumar Garg wrote:
> We hit a case where both the peers assumed that other peer will be GO. Let us assume that p2p_connect command was given on both the peers using a script at the same time. Also assume that P1 has higher mac address than P2. 

Thanks for bringing this up! I'm pretty sure I've seen similar things
reported in the past and getting this address will hopefully make GO
Negotiation more robust.

> 1. P1 will send a GO-NEG-REQ and P2 will also send a GO-NEG-REQ.
> 2. Before P2 could get a callback p2p_go_neg_req_cb to update the variable go_neg_req_sent, P2 receives a GO-NEG request of P1 in the dwell time of its own request.
> 3. So P2 prepares the GO-NEG-RSP and send it even though its mac address is lower than P1 because go_neg_req_sent variable is NOT yet incremented.

It was a bit difficult to reproduce this, but I managed to build a
hacked version of wpa_supplicant to force this to happen more easily.. I
think that the race condition would actually be possible even with the
proposed change even though this makes it less likely to happen. As
such, we better be prepared for the unexpected GO Negotiation Response
frame regardless.

> 4. Now P1 will get P2's GO-NEG-REQ and will reply it since it has higher mac address.
> 5. Both peers end up in sending GO-CONF frame.

This part is the main issue that needs to be prevented since it can
result in getting different results from the GO negotiation. I think
I've now fixed this with the following commit:
http://w1.fi/gitweb/gitweb.cgi?p=hostap.git;a=commitdiff;h=198f82a1e3f78fd478e879185c43965c25840e0d

> To resolve this race, we propose that we increment go_neg_req_sent as soon as p2p_send_action is called for GO-NEG-REQ. And then decrement go_neg_req_sent if because of some reason the success is not reported in the callback p2p_go_neg_req_cb.

This sounds fine as an additional cleanup even though the other commit
should prevent the issue of getting conflicting negotiation results. I
applied this with some more robust design to avoid incrementing the
counter if sending of the action frame fails.
diff mbox

Patch

diff --git a/src/p2p/p2p.c b/src/p2p/p2p.c
old mode 100644
new mode 100755
index deccfc0..37a33dd
--- a/src/p2p/p2p.c
+++ b/src/p2p/p2p.c
@@ -2704,12 +2704,14 @@  static void p2p_go_neg_req_cb(struct p2p_data *p2p, int success)
 	}
 
 	if (success) {
-		dev->go_neg_req_sent++;
 		if (dev->flags & P2P_DEV_USER_REJECTED) {
 			p2p_set_state(p2p, P2P_IDLE);
 			return;
 		}
 	}
+	else {
+		dev->go_neg_req_sent--;
+	}
 
 	if (!success &&
 	    (dev->info.dev_capab & P2P_DEV_CAPAB_CLIENT_DISCOVERABILITY) &&
diff --git a/src/p2p/p2p_go_neg.c b/src/p2p/p2p_go_neg.c
old mode 100644
new mode 100755
index 2bf48b3..727d445
--- a/src/p2p/p2p_go_neg.c
+++ b/src/p2p/p2p_go_neg.c
@@ -220,6 +220,7 @@  int p2p_connect_send(struct p2p_data *p2p, struct p2p_device *dev)
 	p2p->go_neg_peer = dev;
 	dev->flags |= P2P_DEV_WAIT_GO_NEG_RESPONSE;
 	dev->connect_reqs++;
+	dev->go_neg_req_sent++;
 	if (p2p_send_action(p2p, freq, dev->info.p2p_device_addr,
 			    p2p->cfg->dev_addr, dev->info.p2p_device_addr,
 			    wpabuf_head(req), wpabuf_len(req), 200) < 0) {