Patchwork Skip WPS PBC overlap detection if P2P address is the same

On Sat, Dec 10, 2011 at 03:16:02PM +0100, Vitaly Wool wrote:
> WPS overlap detection can detect false overlap if a P2P peer
> changes UUID while authentication is ongoing. Changing UUID
> is of course wrong but this is what some popular devices do
> so we need to work it around in order to keep compatibility
> with these devices. There already is a mechanism in WPS
> registrar to skip overlap detection if P2P addresses of two
> sessions match but it wasn't really triggered because the
> address wasn't filled in in the caller function.

There is a use case for P2P where this could be an issue, i.e., headless
P2P Device using push button. With this patch, that would depend on
something external doing session overlap detection. I don't exactly like
that, but I'm fine with the change for a case when a user explicitly
selects a specific peer device. Unfortunately, I do not know how exactly
wpa_supplicant is being used in headless devices, so it is difficult to
say, whether this would really result in functionality issues in
practice. Being able to interoperate with the deployed devices is likely
enough to justify this change, though.

> Let's fill in this address and also clean up WPS PBC sessions
> on WSC process completion if UUID was changed.

This part did not even compile cleanly.. And well, the patch did not
apply either due to some whitespace damage. Did this miss some changes
since one of the wps_registrar_remove_pbc_session() calls was not
updated with the new argument. I would assume you mean to do something
like this:

Hi Jouni,

On Sat, Dec 10, 2011 at 8:51 PM, Jouni Malinen <j@w1.fi> wrote:

>
> > Let's fill in this address and also clean up WPS PBC sessions
> > on WSC process completion if UUID was changed.
>
> This part did not even compile cleanly.. And well, the patch did not
> apply either due to some whitespace damage. Did this miss some changes
> since one of the wps_registrar_remove_pbc_session() calls was not
> updated with the new argument. I would assume you mean to do something
> like this:
>
>
> diff --git a/src/wps/wps_registrar.c b/src/wps/wps_registrar.c
> index e59edb8..eda1c70 100644
> --- a/src/wps/wps_registrar.c
> +++ b/src/wps/wps_registrar.c
> @@ -310,13 +310,17 @@ 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 ||
> +                   (p2p_dev_addr &&
> !is_zero_ether_addr(reg->p2p_dev_addr) &&
> +                    os_memcmp(reg->p2p_dev_addr, p2p_dev_addr, ETH_ALEN)
> ==
> +                    0)) {
>                         if (prev)
>                                prev->next = pbc->next;
>                        else
> @@ -945,7 +949,7 @@ void wps_registrar_complete(struct wps_registrar
> *registrar, const u8 *uuid_e)
>  {
>        if (registrar->pbc) {
>                wps_registrar_remove_pbc_session(registrar,
> -                                                uuid_e);
> +                                                uuid_e, NULL);
>                wps_registrar_pbc_completed(registrar);
>        } else {
>                wps_registrar_pin_completed(registrar);
> @@ -3047,7 +3051,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 c2095ea..a1c8791 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);
>
>
Right. Sorry about that. I've used the patch I had as an RFC and I totally
forgot to check if there were changes to this code since Nov 29, and now I
can see that there was the commit on Nov 30 adding one more call
to wpa_supplicant_ap_wps_pbc().

So once again sorry for the mess and thanks for taking care of fixing it,

Thanks,
   Vitaly

On Sat, Dec 10, 2011 at 09:13:29PM +0100, Vitaly Wool wrote:
> Right. Sorry about that. I've used the patch I had as an RFC and I totally
> forgot to check if there were changes to this code since Nov 29, and now I
> can see that there was the commit on Nov 30 adding one more call
> to wpa_supplicant_ap_wps_pbc().

Thanks, applied.