diff mbox

P2P: Fix inconsistency in displaying wps pin

Message ID 6C370B347C3FE8438C9692873287D2E1195AFB30EC@SJEXCHCCR01.corp.ad.broadcom.com
State Superseded, archived
Headers show

Commit Message

Jithu Jance Dec. 9, 2011, 11:28 a.m. UTC
When "p2p_connect <devaddr> pin display join" command is issued, the pin generated in the p2p_connect context and the SHOW-PIN event are

Comments

Jouni Malinen Dec. 10, 2011, 10:16 a.m. UTC | #1
On Fri, Dec 09, 2011 at 03:28:35AM -0800, Jithu Jance wrote:
> When "p2p_connect <devaddr> pin display join" command is issued, the pin generated in the p2p_connect context and the SHOW-PIN event are 
> different. The PIN generated in the p2p_connect context is the one in use. The provision discovery response pin generation is useful when explicit
> provision discovery is done and then the generated PIN is entered via the subsequent p2p_connect command. The attached patch doesn't generate PIN,
> if the pin is already generated in the p2p_connect context. Kindly see whether the patch is okay.

This is a bit problematic since wpa_s->p2p_pin does not get cleared and
the same PIN would be used in number of operations where it was not
supposed to be used.

> > p2p_connect 02:90:4c:12:34:56 pin display join
> 78292024 <<<<<<<<<<<<<<<<<<<<<<<<<<<<< PIN Generated in the connect context
> <3>P2P-PROV-DISC-SHOW-PIN 02:90:4c:12:34:56 23612945 <<<<<<<< Show Pin event.

In this particular sequence, I don't really see much point in even
displaying the P2P-PROV-DISC-SHOW-PIN event. In fact, it does not
necessarily even get shown if the Provision Discovery Response is not
received (there is no strict requirement on waiting for it in the case
of p2p_connect join). As such, it could be better to just make this
event conditional in a way that it is not indicated for p2p_connect join
sequence.

> In case the below copy is whitespace damaged, please use the attached patch file.

The inline version was fine - thanks.
Jithu Jance Dec. 12, 2011, 5:35 a.m. UTC | #2
Hi Jouni,


>> > p2p_connect 02:90:4c:12:34:56 pin display join
>> 78292024 <<<<<<<<<<<<<<<<<<<<<<<<<<<<< PIN Generated in the connect context
>> <3>P2P-PROV-DISC-SHOW-PIN 02:90:4c:12:34:56 23612945 <<<<<<<< Show Pin event.

> In this particular sequence, I don't really see much point in even
> displaying the P2P-PROV-DISC-SHOW-PIN event. In fact, it does not
> necessarily even get shown if the Provision Discovery Response is not
> received (there is no strict requirement on waiting for it in the case
> of p2p_connect join). As such, it could be better to just make this
> event conditional in a way that it is not indicated for p2p_connect join
> sequence.

Sorry that I didn't understand the above context properly. UI Application listens 
on P2P-PROV-DISC-SHOW-PIN ctrl event to display the pin to user and the peer
uses this pin to activate wps pin session on his device. So without SHOW-PIN ctrl event, 
is there a way to let the application know the about wps_pin to be displayed?

Do you mean that the pin returned by "p2p_connect pin display join" command should be used 
instead of waiting for SHOW-PIN event?? 
But this approach is inconsistent with the explicit provision discovery req - resp sequence used when the peer
is using keypad. In this case local device should display the pin and this display is done on SHOW-PIN event.

> (there is no strict requirement on waiting for it in the case
> of p2p_connect join). 

I understand that strict waiting for provision disc resp is not done during "p2p_connect join". This waiting is not necessary
when local device uses keypad/pbc option. But consider a scenario where local device chooses pin display 
method where the peer is expected to enter the PIN which local device displays., Here local device requires some event to
let the application know about the PIN it uses. Does it make sense to mandate pd response if the local device uses display
method??


>> The attached patch doesn't generate PIN,
>> if the pin is already generated in the p2p_connect context.

> This is a bit problematic since wpa_s->p2p_pin does not get cleared and
> the same PIN would be used in number of operations where it was not
> supposed to be used.
 
Would the patch make sense, if the p2p_pin is cleared properly after 
wpas_start_wps_enrollee.?? 




- Jithu Jance


-----Original Message-----
From: hostap-bounces@lists.shmoo.com [mailto:hostap-bounces@lists.shmoo.com] On Behalf Of Jouni Malinen
Sent: Saturday, December 10, 2011 3:47 PM
To: hostap@lists.shmoo.com
Subject: Re: [PATCH] P2P: Fix inconsistency in displaying wps pin

On Fri, Dec 09, 2011 at 03:28:35AM -0800, Jithu Jance wrote:
> When "p2p_connect <devaddr> pin display join" command is issued, the pin generated in the p2p_connect context and the SHOW-PIN event are 
> different. The PIN generated in the p2p_connect context is the one in use. The provision discovery response pin generation is useful when explicit
> provision discovery is done and then the generated PIN is entered via the subsequent p2p_connect command. The attached patch doesn't generate PIN,
> if the pin is already generated in the p2p_connect context. Kindly see whether the patch is okay.

This is a bit problematic since wpa_s->p2p_pin does not get cleared and
the same PIN would be used in number of operations where it was not
supposed to be used.

> > p2p_connect 02:90:4c:12:34:56 pin display join
> 78292024 <<<<<<<<<<<<<<<<<<<<<<<<<<<<< PIN Generated in the connect context
> <3>P2P-PROV-DISC-SHOW-PIN 02:90:4c:12:34:56 23612945 <<<<<<<< Show Pin event.

In this particular sequence, I don't really see much point in even
displaying the P2P-PROV-DISC-SHOW-PIN event. In fact, it does not
necessarily even get shown if the Provision Discovery Response is not
received (there is no strict requirement on waiting for it in the case
of p2p_connect join). As such, it could be better to just make this
event conditional in a way that it is not indicated for p2p_connect join
sequence.

> In case the below copy is whitespace damaged, please use the attached patch file.

The inline version was fine - thanks.
Jouni Malinen Dec. 18, 2011, 6:18 p.m. UTC | #3
On Sun, Dec 11, 2011 at 09:35:58PM -0800, Jithu Jance wrote:
> Sorry that I didn't understand the above context properly. UI Application listens 
> on P2P-PROV-DISC-SHOW-PIN ctrl event to display the pin to user and the peer
> uses this pin to activate wps pin session on his device. So without SHOW-PIN ctrl event, 
> is there a way to let the application know the about wps_pin to be displayed?
> 
> Do you mean that the pin returned by "p2p_connect pin display join" command should be used 
> instead of waiting for SHOW-PIN event?? 

Yes.

> But this approach is inconsistent with the explicit provision discovery req - resp sequence used when the peer
> is using keypad. In this case local device should display the pin and this display is done on SHOW-PIN event.

Sure, this is different sequence, but the UI program that issued the
p2p_connect command in the first place better know which end is going to
be displaying the PIN and it should be able to figure out easily that
the value returned by p2p_connect is this.

> I understand that strict waiting for provision disc resp is not done during "p2p_connect join". This waiting is not necessary
> when local device uses keypad/pbc option. But consider a scenario where local device chooses pin display 
> method where the peer is expected to enter the PIN which local device displays., Here local device requires some event to
> let the application know about the PIN it uses. Does it make sense to mandate pd response if the local device uses display
> method??

Waiting for the Provision Discovery Response frame is not really
required for any case - displaying the correct PIN is. The correct PIN
is the one that was used (or returned) with p2p_connect.

> > This is a bit problematic since wpa_s->p2p_pin does not get cleared and
> > the same PIN would be used in number of operations where it was not
> > supposed to be used.
>  
> Would the patch make sense, if the p2p_pin is cleared properly after 
> wpas_start_wps_enrollee.?? 

It could still be problematic in some sequences. I don't have anything
against making the P2P-PROV-DISC-SHOW-PIN value match with the
p2p_connect return value, but it needs to be understood that there is no
guarantee on this event being shown in this join-a-running-group case.
Since the real PIN that gets used during the provisioning step is the
one used with p2p_connect, any change to the provision discovery events
needs to be done with the main use case with p2p_connect in mind. It is
much more important for that to work than any partial improvement for
the provision discovery events.
diff mbox

Patch

different. The PIN generated in the p2p_connect context is the one in use. The provision discovery response pin generation is useful when explicit
provision discovery is done and then the generated PIN is entered via the subsequent p2p_connect command. The attached patch doesn't generate PIN,
if the pin is already generated in the p2p_connect context. Kindly see whether the patch is okay.


> p2p_connect 02:90:4c:12:34:56 pin display join
78292024 <<<<<<<<<<<<<<<<<<<<<<<<<<<<< PIN Generated in the connect context
<3>P2P-PROV-DISC-SHOW-PIN 02:90:4c:12:34:56 23612945 <<<<<<<< Show Pin event.
<3>CTRL-EVENT-SCAN-RESULTS 
<3>WPS-AP-AVAILABLE 
<3>CTRL-EVENT-SCAN-RESULTS 

In case the below copy is whitespace damaged, please use the attached patch file.


Signed-hostap: Jithu Jance <jithu@broadcom.com>

---
 wpa_supplicant/p2p_supplicant.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
index 4ce0432..43eff2f 100644
--- a/wpa_supplicant/p2p_supplicant.c
+++ b/wpa_supplicant/p2p_supplicant.c
@@ -1727,7 +1727,12 @@  void wpas_prov_disc_req(void *ctx, const u8 *peer, u16 config_methods,
 	params[sizeof(params) - 1] = '\0';
 
 	if (config_methods & WPS_CONFIG_DISPLAY) {
-		generated_pin = wps_generate_pin();
+		/* If p2p_pin is already populated, use it*/
+		if(wpa_s->p2p_pin[0] == '\0') {
+			generated_pin = wps_generate_pin();
+		} else {
+			generated_pin = atoi(wpa_s->p2p_pin);
+		}
 		wpas_prov_disc_local_display(wpa_s, peer, params,
 					     generated_pin);
 	} else if (config_methods & WPS_CONFIG_KEYPAD)
@@ -1750,7 +1755,12 @@  void wpas_prov_disc_resp(void *ctx, const u8 *peer, u16 config_methods)
 	if (config_methods & WPS_CONFIG_DISPLAY)
 		wpas_prov_disc_local_keypad(wpa_s, peer, "");
 	else if (config_methods & WPS_CONFIG_KEYPAD) {
-		generated_pin = wps_generate_pin();
+		/* If p2p_pin is already populated, use it*/
+		if(wpa_s->p2p_pin[0] == '\0') {
+			generated_pin = wps_generate_pin();
+		} else {
+			generated_pin = atoi(wpa_s->p2p_pin);
+		}
 		wpas_prov_disc_local_display(wpa_s, peer, "", generated_pin);
 	} else if (config_methods & WPS_CONFIG_PUSHBUTTON)
 		wpa_msg(wpa_s, MSG_INFO, P2P_EVENT_PROV_DISC_PBC_RESP MACSTR,