| Message ID | CAMJBoFPHY8FKyW+h+VaYTf5n5H66ebHkaokCZzA9=mwMtbGjzw@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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); >
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.
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
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
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..
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
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.
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);