Patchwork [PATCH/RFC] improve overlap detection and handling for P2P PBC

login
register
mail settings
Submitter Vitaly Wool
Date Nov. 29, 2011, 4:36 p.m.
Message ID <CAMJBoFPHY8FKyW+h+VaYTf5n5H66ebHkaokCZzA9=mwMtbGjzw@mail.gmail.com>
Download mbox | patch
Permalink /patch/128302/
State Superseded
Headers show

Comments

Vitaly Wool - Nov. 29, 2011, 4:36 p.m.
Hi,

I've been struggling to get my prototype P2P device to work with Samsung
Galaxy SII. The connection establishment kept failing with WPS_FAILURE and
it turned out to be due to overlap detected:

01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: Requested UUID -
hexdump(len=16): 22 21 02 03 04 05 06 07 08 09 1a 1b 1c 1d 1e 1f
01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: Consider PBC session with
06:46:65:d3:4a:54
01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: UUID-E - hexdump(len=16):
22 21 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: New Enrollee
01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: 2 active PBC session(s)
found
01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: PBC overlap - deny PBC
negotiation

So Galaxy changes UUID for PBC negotiation but the thing is, it could have
been considered to be the same session because the MAC address is the same.
There is a mechanism to do so for P2P connections in wpa_supplicant but it
doesn't work because P2P MAC address is not passed over
to wps_registrar_skip_overlap(). This patch adds that and also fixes the
PBC session removal after the negotiation (the current version leaves the
session in the list if it doesn't match UUID, I suggest that we remove all
sessions for the given MAC).
Vitaly Wool - Nov. 29, 2011, 5:11 p.m.
Looks like tabs have been messed up here but I hope it's okay since this is
an RFC anyway. If it's fine, I'll make a respin with tabs unexpanded.

Thanks,
   Vitaly
Den 29 nov 2011 17:36 skrev "Vitaly Wool" <vitalywool@gmail.com>:

> Hi,
>
> I've been struggling to get my prototype P2P device to work with Samsung
> Galaxy SII. The connection establishment kept failing with WPS_FAILURE and
> it turned out to be due to overlap detected:
>
> 01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: Requested UUID -
> hexdump(len=16): 22 21 02 03 04 05 06 07 08 09 1a 1b 1c 1d 1e 1f
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: Consider PBC session with
> 06:46:65:d3:4a:54
> 01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: UUID-E - hexdump(len=16):
> 22 21 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: New Enrollee
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: 2 active PBC session(s)
> found
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: PBC overlap - deny PBC
> negotiation
>
> So Galaxy changes UUID for PBC negotiation but the thing is, it could have
> been considered to be the same session because the MAC address is the same.
> There is a mechanism to do so for P2P connections in wpa_supplicant but it
> doesn't work because P2P MAC address is not passed over
> to wps_registrar_skip_overlap(). This patch adds that and also fixes the
> PBC session removal after the negotiation (the current version leaves the
> session in the list if it doesn't match UUID, I suggest that we remove all
> sessions for the given MAC).
>
> diff --git a/src/wps/wps_registrar.c b/src/wps/wps_registrar.c
> index 4a49197..eed29e2 100644
> --- a/src/wps/wps_registrar.c
> +++ b/src/wps/wps_registrar.c
> @@ -310,13 +310,16 @@ static void wps_registrar_add_pbc_session(struct
> wps_registrar *reg,
>
>
>  static void wps_registrar_remove_pbc_session(struct wps_registrar *reg,
> -     const u8 *uuid_e)
> +     const u8 *uuid_e,
> +     const u8 *p2p_dev_addr)
>  {
>   struct wps_pbc_session *pbc, *prev = NULL, *tmp;
>
>   pbc = reg->pbc_sessions;
>   while (pbc) {
> - if (os_memcmp(pbc->uuid_e, uuid_e, WPS_UUID_LEN) == 0) {
> + if (os_memcmp(pbc->uuid_e, uuid_e, WPS_UUID_LEN) == 0 ||
> +    (!is_zero_ether_addr(reg->p2p_dev_addr) &&
> +     !os_memcmp(reg->p2p_dev_addr, p2p_dev_addr, ETH_ALEN))) {
>   if (prev)
>   prev->next = pbc->next;
>   else
> @@ -3035,7 +3038,8 @@ static enum wps_process_res
> wps_process_wsc_done(struct wps_data *wps,
>
>   if (wps->pbc) {
>   wps_registrar_remove_pbc_session(wps->wps->registrar,
> - wps->uuid_e);
> + wps->uuid_e,
> + wps->p2p_dev_addr);
>   wps_registrar_pbc_completed(wps->wps->registrar);
>   } else {
>   wps_registrar_pin_completed(wps->wps->registrar);
> diff --git a/wpa_supplicant/p2p_supplicant.c
> b/wpa_supplicant/p2p_supplicant.c
> index c6484af..1700ace 100644
> --- a/wpa_supplicant/p2p_supplicant.c
> +++ b/wpa_supplicant/p2p_supplicant.c
> @@ -689,7 +689,7 @@ static void p2p_go_configured(void *ctx, void *data)
>   }
>   if (params->wps_method == WPS_PBC)
>   wpa_supplicant_ap_wps_pbc(wpa_s, params->peer_interface_addr,
> -  NULL);
> +  params->peer_device_addr);
>   else if (wpa_s->p2p_pin[0])
>   wpa_supplicant_ap_wps_pin(wpa_s, params->peer_interface_addr,
>    wpa_s->p2p_pin, NULL, 0);
>
Jouni Malinen - Nov. 29, 2011, 6:20 p.m.
On Tue, Nov 29, 2011 at 05:36:26PM +0100, Vitaly Wool wrote:
> I've been struggling to get my prototype P2P device to work with Samsung
> Galaxy SII. The connection establishment kept failing with WPS_FAILURE and
> it turned out to be due to overlap detected:
> 
> 01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: Requested UUID -
> hexdump(len=16): 22 21 02 03 04 05 06 07 08 09 1a 1b 1c 1d 1e 1f
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: Consider PBC session with
> 06:46:65:d3:4a:54
> 01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: UUID-E - hexdump(len=16):
> 22 21 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: New Enrollee
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: 2 active PBC session(s)
> found
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: PBC overlap - deny PBC
> negotiation

Argh.. Is that the way this device works with a deployed software? That
is a known bug in that specific WPS implementation and I was hoping that
it would never get released in a real end user product.

This is so broken on multiple levels.. Those UUIDs are supposed to be
unique for each device (good luck with those hardcoded values being
unique) and only a single UUID can be used by the device, but this
device is using two different ones..

> So Galaxy changes UUID for PBC negotiation but the thing is, it could have
> been considered to be the same session because the MAC address is the same.

Well, yes, it could have, but this is so horribly broken that I would
like to just not allow it to use PBC.

> There is a mechanism to do so for P2P connections in wpa_supplicant but it
> doesn't work because P2P MAC address is not passed over
> to wps_registrar_skip_overlap(). This patch adds that and also fixes the
> PBC session removal after the negotiation (the current version leaves the
> session in the list if it doesn't match UUID, I suggest that we remove all
> sessions for the given MAC).

Could you please confirm that you are seeing this broken behavior with a
deployed end user product and there are large number of those deployed?
I don't think I would agree with all these changes since they break the
way PBC overlap detection is supposed to work. If this bad behavior
shows up in huge number of end user devices, it may be justifiable to
add a workaround for it, but I want to limit the scope of how far the
workaround goes in disabling overlap detection.
Vitaly Wool - Nov. 29, 2011, 9:46 p.m.
Hi Jouni,

On Tue, Nov 29, 2011 at 7:20 PM, Jouni Malinen <j@w1.fi> wrote:

> On Tue, Nov 29, 2011 at 05:36:26PM +0100, Vitaly Wool wrote:
> > I've been struggling to get my prototype P2P device to work with Samsung
> > Galaxy SII. The connection establishment kept failing with WPS_FAILURE
> and
> > it turned out to be due to overlap detected:
> >
> > 01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: Requested UUID -
> > hexdump(len=16): 22 21 02 03 04 05 06 07 08 09 1a 1b 1c 1d 1e 1f
> > 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: Consider PBC session
> with
> > 06:46:65:d3:4a:54
> > 01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: UUID-E -
> hexdump(len=16):
> > 22 21 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> > 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: New Enrollee
> > 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: 2 active PBC session(s)
> > found
> > 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: PBC overlap - deny PBC
> > negotiation
>
> Argh.. Is that the way this device works with a deployed software? That
> is a known bug in that specific WPS implementation and I was hoping that
> it would never get released in a real end user product.
>
> This is so broken on multiple levels.. Those UUIDs are supposed to be
> unique for each device (good luck with those hardcoded values being
> unique) and only a single UUID can be used by the device, but this
> device is using two different ones..
>
> > So Galaxy changes UUID for PBC negotiation but the thing is, it could
> have
> > been considered to be the same session because the MAC address is the
> same.
>
> Well, yes, it could have, but this is so horribly broken that I would
> like to just not allow it to use PBC.
>

Right, but OTOH, if we have this overlap detection skipping mechanism is
present in wpa_supplicant, let's either use it or drop it. I'd suggest that
we used it, maybe under a configuration option of some kind, maybe
producing warning messages in logs but still.


>
> > There is a mechanism to do so for P2P connections in wpa_supplicant but
> it
> > doesn't work because P2P MAC address is not passed over
> > to wps_registrar_skip_overlap(). This patch adds that and also fixes the
> > PBC session removal after the negotiation (the current version leaves the
> > session in the list if it doesn't match UUID, I suggest that we remove
> all
> > sessions for the given MAC).
>
> Could you please confirm that you are seeing this broken behavior with a
> deployed end user product and there are large number of those deployed?
> I don't think I would agree with all these changes since they break the
> way PBC overlap detection is supposed to work. If this bad behavior
> shows up in huge number of end user devices, it may be justifiable to
> add a workaround for it, but I want to limit the scope of how far the
> workaround goes in disabling overlap detection.
>
>
This broken behavior is seen with Samsung Galaxy SII, which is one of the
top 5 smartphone bestsellers all over the world, and it's gotten all the
latest updates. I'm not 100% sure yet but it looks like LG970 has gotten
the same problem, so it's fair to say that all the Wi-Fi Direct enabled
smartphones currently widely available on the market have got this problem.

Thanks,
   Vitaly
Vitaly Wool - Nov. 30, 2011, 9:46 a.m.
Hi Jouni,

On Tue, Nov 29, 2011 at 10:46 PM, Vitaly Wool <vitalywool@gmail.com> wrote:

>
> This broken behavior is seen with Samsung Galaxy SII, which is one of the
> top 5 smartphone bestsellers all over the world, and it's gotten all the
> latest updates. I'm not 100% sure yet but it looks like LG970 has gotten
> the same problem, so it's fair to say that all the Wi-Fi Direct enabled
> smartphones currently widely available on the market have got this problem.
>
>
just confirming the same behavior of LG970, and thus my last statement from
the previous mail is correct. Basically it means that we need this
workaround, even though I can surely see how ugly it is.

Thanks,
   Vitaly
Jouni Malinen - Dec. 6, 2011, 6:10 p.m.
On Tue, Nov 29, 2011 at 05:36:26PM +0100, Vitaly Wool wrote:
> I've been struggling to get my prototype P2P device to work with Samsung
> Galaxy SII. The connection establishment kept failing with WPS_FAILURE and
> it turned out to be due to overlap detected:
> 
> 01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: Requested UUID -
> hexdump(len=16): 22 21 02 03 04 05 06 07 08 09 1a 1b 1c 1d 1e 1f
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: Consider PBC session with
> 06:46:65:d3:4a:54
> 01-03 01:27:26.140 E/wpa_supplicant( 2455): WPS: UUID-E - hexdump(len=16):
> 22 21 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: New Enrollee
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: 2 active PBC session(s)
> found
> 01-03 01:27:26.140 D/wpa_supplicant( 2455): WPS: PBC overlap - deny PBC
> negotiation

Could you please confirm whether the Probe Response frames with these
two UUIDs are using the same MAC address as the sender? I've seen a case
where both the MAC address and the UUID used different values even when
they came from a single device..
Vitaly Wool - Dec. 10, 2011, 11 a.m.
Hi Jouni,

On Tue, Dec 6, 2011 at 7:10 PM, Jouni Malinen <j@w1.fi> wrote:

>
> Could you please confirm whether the Probe Response frames with these
> two UUIDs are using the same MAC address as the sender? I've seen a case
> where both the MAC address and the UUID used different values even when
> they came from a single device..
>
>
MAC addresses are the same, and P2P device address is the same as well
(though different from the MAC) and well, otherwise my patch wouldn't work.

FWIW, the patch just provides a symmetry, one can run "wps_pbc
xx:xx:xx:xx:xx:xx" from wpa_cli which will result in skipping overlap
detection, so why don't we do that for P2P?

Thanks,
   Vitaly
Jouni Malinen - Dec. 10, 2011, 11:36 a.m.
On Sat, Dec 10, 2011 at 12:00:21PM +0100, Vitaly Wool wrote:
> MAC addresses are the same, and P2P device address is the same as well
> (though different from the MAC) and well, otherwise my patch wouldn't work.

Thanks. I've seen a much worse case of this with the same UUID values,
but different MAC addresses..

> FWIW, the patch just provides a symmetry, one can run "wps_pbc
> xx:xx:xx:xx:xx:xx" from wpa_cli which will result in skipping overlap
> detection, so why don't we do that for P2P?

There is a long history behind this, but yes, it sounds reasonable to
allow this. I don't exactly agree with this being an "improvement" of
overlap detection, but well, that is easy to change for the commit
message ;-). I have actually looked at working around the worse case in
the past and just had to gave up there. That's why I wanted to make sure
this is a smaller subset of the issue and something that can actually be
resolved without completely disabling overlap detection.

Patch

diff --git a/src/wps/wps_registrar.c b/src/wps/wps_registrar.c
index 4a49197..eed29e2 100644
--- a/src/wps/wps_registrar.c
+++ b/src/wps/wps_registrar.c
@@ -310,13 +310,16 @@  static void wps_registrar_add_pbc_session(struct
wps_registrar *reg,


 static void wps_registrar_remove_pbc_session(struct wps_registrar *reg,
-     const u8 *uuid_e)
+     const u8 *uuid_e,
+     const u8 *p2p_dev_addr)
 {
  struct wps_pbc_session *pbc, *prev = NULL, *tmp;

  pbc = reg->pbc_sessions;
  while (pbc) {
- if (os_memcmp(pbc->uuid_e, uuid_e, WPS_UUID_LEN) == 0) {
+ if (os_memcmp(pbc->uuid_e, uuid_e, WPS_UUID_LEN) == 0 ||
+    (!is_zero_ether_addr(reg->p2p_dev_addr) &&
+     !os_memcmp(reg->p2p_dev_addr, p2p_dev_addr, ETH_ALEN))) {
  if (prev)
  prev->next = pbc->next;
  else
@@ -3035,7 +3038,8 @@  static enum wps_process_res
wps_process_wsc_done(struct wps_data *wps,

  if (wps->pbc) {
  wps_registrar_remove_pbc_session(wps->wps->registrar,
- wps->uuid_e);
+ wps->uuid_e,
+ wps->p2p_dev_addr);
  wps_registrar_pbc_completed(wps->wps->registrar);
  } else {
  wps_registrar_pin_completed(wps->wps->registrar);
diff --git a/wpa_supplicant/p2p_supplicant.c
b/wpa_supplicant/p2p_supplicant.c
index c6484af..1700ace 100644
--- a/wpa_supplicant/p2p_supplicant.c
+++ b/wpa_supplicant/p2p_supplicant.c
@@ -689,7 +689,7 @@  static void p2p_go_configured(void *ctx, void *data)
  }
  if (params->wps_method == WPS_PBC)
  wpa_supplicant_ap_wps_pbc(wpa_s, params->peer_interface_addr,
-  NULL);
+  params->peer_device_addr);
  else if (wpa_s->p2p_pin[0])
  wpa_supplicant_ap_wps_pin(wpa_s, params->peer_interface_addr,
   wpa_s->p2p_pin, NULL, 0);