[PATCH v2 2/4] HS20: Add support for configuring frame filters

[Jouni Malinen]

On Tue, Mar 08, 2016 at 01:25:41PM +0200, Ilan Peer wrote:
> When a station has associated to a HS2.0 network, request
> the driver to do the following, based on the BSS capabilities:
> 
> 1. Enable gratuitous ARP filtering
> 2. Enable unsolicited Neighbor Advertisement filtering
> 3. Enable GTK filtering if DGAF disabled bit is zero
> 
> HS2.0 requires the filtering to start only after an IP
> address is obtained. However, the configuration is done
> regardless of obtaining an IP address, so this might
> not be good enough.

That "and obtained an IP address" part looks a bit strange.. Why would
this filtering not be enabled before IP address assignment completes?

> diff --git a/wpa_supplicant/hs20_supplicant.c b/wpa_supplicant/hs20_supplicant.c
> +void hs20_configure_frame_filters(struct wpa_supplicant *wpa_s)
> +	if (filter)
> +		wpa_drv_configure_frame_filters(wpa_s, filter);

Why is this call skipped for filter == 0? Wouldn't it be fine and
potentially even preferred to clear the setting here if for any reason
the driver setting was not cleared previously?

> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
> @@ -751,6 +751,9 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
>  		wpas_connect_work_done(wpa_s);
>  		/* Reinitialize normal_scan counter */
>  		wpa_s->normal_scans = 0;
> +#ifdef CONFIG_HS20
> +		hs20_configure_frame_filters(wpa_s);
> +#endif /* CONFIG_HS20 */

While this might be closer to the "once .. obtained an IP address",
wouldn't it be more robust to set these filter before even starting the
association? There might be a race condition here (even if very unlikely
to be hit) of IP packets getting exchanged before this call is executed.

> @@ -816,8 +819,15 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
>  	if (state == WPA_COMPLETED)
>  		wpa_supplicant_start_bgscan(wpa_s);
> -	else if (state < WPA_ASSOCIATED)
> +	else if (state < WPA_ASSOCIATED) {
>  		wpa_supplicant_stop_bgscan(wpa_s);
> +
> +#ifdef CONFIG_HS20
> +		/* clear possibly configured frame filters */
> +		if (old_state == WPA_COMPLETED)
> +			wpa_drv_configure_frame_filters(wpa_s, 0);
> +#endif /* CONFIG_HS20 */
> +	}

What is this trying to do by requiring state < WPA_ASSOCIATED and
old_state == WPA_COMPLETED? What if the STA gets disconnected when going
through rekeying (state is WPA_4WAY_HANDSHAKE or WPA_GROUP_HANDSHAKE)?
I'm not sure wpa_suppliant_set_state() is really the best location for
either of these frame filter calls. In other words, these sound more
like items that should be done when starting an association and when
processing a disassociation. In addition, there should likely be code to
clear the configured values when deinitializing the interface.
 
-- 
Jouni Malinen                                            PGP id EFC895FA