diff mbox

[2/7] TDLS: bail on STA add failure in tpk_m1 processing

Message ID 1402424350-19261-2-git-send-email-ilan.peer@intel.com
State Accepted
Headers show

Commit Message

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

The driver might not be able to add the TDLS STA. Fail if this happens.
Also fix the error path to always reset the TDLS peer data.

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

Comments

Jouni Malinen June 16, 2014, 11 p.m. UTC | #1 
On Tue, Jun 10, 2014 at 09:19:05PM +0300, Ilan Peer wrote:
> From: Arik Nemtsov <arik@wizery.com>
> The driver might not be able to add the TDLS STA. Fail if this happens.
> Also fix the error path to always reset the TDLS peer data.

> diff --git a/src/rsn_supp/tdls.c b/src/rsn_supp/tdls.c
> index c08d2f9..e712a4d 100644
> --- a/src/rsn_supp/tdls.c
> +++ b/src/rsn_supp/tdls.c
> @@ -1919,6 +1920,7 @@ skip_rsn_check:
>  error:
>  	wpa_tdls_send_error(sm, src_addr, WLAN_TDLS_SETUP_RESPONSE, dtoken,
>  			    status);
> +	wpa_tdls_peer_free(sm, peer);
>  	return -1;

I should have noticed that before pushing the commits, but well, didn't.
Thankfully static analyzers are more alert at this hour, so this got
fixed quickly.. That's a NULL pointer dereference on peer if the first
goto error case is hit (unlikely, but possible).
Arik Nemtsov June 17, 2014, 6:28 a.m. UTC | #2 
On Tue, Jun 17, 2014 at 2:00 AM, Jouni Malinen <j@w1.fi> wrote:
> On Tue, Jun 10, 2014 at 09:19:05PM +0300, Ilan Peer wrote:
>> From: Arik Nemtsov <arik@wizery.com>
>> The driver might not be able to add the TDLS STA. Fail if this happens.
>> Also fix the error path to always reset the TDLS peer data.
>
>> diff --git a/src/rsn_supp/tdls.c b/src/rsn_supp/tdls.c
>> index c08d2f9..e712a4d 100644
>> --- a/src/rsn_supp/tdls.c
>> +++ b/src/rsn_supp/tdls.c
>> @@ -1919,6 +1920,7 @@ skip_rsn_check:
>>  error:
>>       wpa_tdls_send_error(sm, src_addr, WLAN_TDLS_SETUP_RESPONSE, dtoken,
>>                           status);
>> +     wpa_tdls_peer_free(sm, peer);
>>       return -1;
>
> I should have noticed that before pushing the commits, but well, didn't.
> Thankfully static analyzers are more alert at this hour, so this got
> fixed quickly.. That's a NULL pointer dereference on peer if the first
> goto error case is hit (unlikely, but possible).

Right. Thanks.

Looking at the patch again made me realized I forgot to handle the
wpa_sm_tdls_peer_addset call where we initiate the connection. I'll
fix it.
I also have some more patches in the pipe for QoS/HT TDLS with mac80211.

Arik
Peer, Ilan June 17, 2014, 2:26 p.m. UTC | #3 
Hi Jouni,

> 
> I should have noticed that before pushing the commits, but well, didn't.
> Thankfully static analyzers are more alert at this hour, so this got fixed
> quickly.. That's a NULL pointer dereference on peer if the first goto error case
> is hit (unlikely, but possible).
> 

What static analysis tool are u using? We are using sparse and KW and somehow they both missed this.

Thanks in advance,

Ilan.
Jouni Malinen June 17, 2014, 2:45 p.m. UTC | #4 
On Tue, Jun 17, 2014 at 02:26:49PM +0000, Peer, Ilan wrote:
> What static analysis tool are u using? We are using sparse and KW and somehow they both missed this.

I use scan-build/Clang, sparse, Coverity, Klocwork, and every now and
then some other tools semi-randomly when checking what new is available
out there. This specific report was from Coverity, but it looks like
clang (at least v3.5) reports this as well.
diff mbox

Patch

diff --git a/src/rsn_supp/tdls.c b/src/rsn_supp/tdls.c
index c08d2f9..e712a4d 100644
--- a/src/rsn_supp/tdls.c
+++ b/src/rsn_supp/tdls.c
@@ -1848,7 +1848,6 @@  skip_rsn:
 		if (os_get_random(peer->rnonce, WPA_NONCE_LEN)) {
 			wpa_msg(sm->ctx->ctx, MSG_WARNING,
 				"TDLS: Failed to get random data for responder nonce");
-			wpa_tdls_peer_free(sm, peer);
 			goto error;
 		}
 	}
@@ -1904,8 +1903,10 @@  skip_rsn:
 
 skip_rsn_check:
 	/* add the peer to the driver as a "setup in progress" peer */
-	wpa_sm_tdls_peer_addset(sm, peer->addr, 1, 0, 0, NULL, 0, NULL, NULL, 0,
-				NULL, 0, NULL, 0, NULL, 0);
+	if (wpa_sm_tdls_peer_addset(sm, peer->addr, 1, 0, 0, NULL, 0, NULL,
+				    NULL, 0, NULL, 0, NULL, 0, NULL, 0))
+		goto error;
+
 	peer->tpk_in_progress = 1;
 
 	wpa_printf(MSG_DEBUG, "TDLS: Sending TDLS Setup Response / TPK M2");
@@ -1919,6 +1920,7 @@  skip_rsn_check:
 error:
 	wpa_tdls_send_error(sm, src_addr, WLAN_TDLS_SETUP_RESPONSE, dtoken,
 			    status);
+	wpa_tdls_peer_free(sm, peer);
 	return -1;
 }