[PATCH] P2P: WFd: Add wfd_dev_info= field for device found event

[Jouni Malinen]

On Fri, Nov 22, 2013 at 04:39:37PM -0800, Dmitry Shmidt wrote:
> diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
> @@ -1442,16 +1442,27 @@ void wpas_dev_found(void *ctx, const u8 *addr,
>  #ifndef CONFIG_NO_STDOUT_DEBUG
>  	struct wpa_supplicant *wpa_s = ctx;
>  	char devtype[WPS_DEV_TYPE_BUFSIZE];
> -
> +#define WFD_DEV_INFO_SIZE 9
> +	char wfd_dev_info_hex[2 * WFD_DEV_INFO_SIZE + 1];
> +	os_memset(wfd_dev_info_hex, 0, sizeof(wfd_dev_info_hex));
> +#ifdef CONFIG_WIFI_DISPLAY
> +	if (info->wfd_subelems) {
> +		wpa_snprintf_hex(wfd_dev_info_hex, sizeof(wfd_dev_info_hex),
> +				 wpabuf_head(info->wfd_subelems),
> +				 WFD_DEV_INFO_SIZE);

Where does this assumption of there always being 9 octets of data in the
full set of WFD elements come from? This also seem to assume that the
device info would always be the first subelement. This does not look
safe since the elements parser in p2p_parse_ies() does no such
validation. This sounds like something that could result in buffer
overflow based on data received over air and/or result in truncated or
unexpected information being included in the event.

>  	wpa_msg_global(wpa_s, MSG_INFO, P2P_EVENT_DEVICE_FOUND MACSTR
>  		       " p2p_dev_addr=" MACSTR
>  		       " pri_dev_type=%s name='%s' config_methods=0x%x "
> -		       "dev_capab=0x%x group_capab=0x%x",
> +		       "dev_capab=0x%x group_capab=0x%x%s%s",
>  		       MAC2STR(addr), MAC2STR(info->p2p_device_addr),
>  		       wps_dev_type_bin2str(info->pri_dev_type, devtype,
>  					    sizeof(devtype)),
> -		info->device_name, info->config_methods,
> -		info->dev_capab, info->group_capab);
> +		       info->device_name, info->config_methods,
> +		       info->dev_capab, info->group_capab,
> +		       wfd_dev_info_hex[0] ? " wfd_dev_info=0x" : "",
> +		       wfd_dev_info_hex);

Why would this information be needed in the P2P-DEVICE-FOUND event? It
is already available from the P2P_PEER command and that information is
actually provided in full rather than this type of fixed length of 9
assumption.

If there is real need to get the Wi-Fi Display information added to the
P2P-DEVICE-FOUND event, this would need to be fixed to validate the
data, parse the subelements, and only include the needed information. I
guess that length 9 is referring to the current total size of the WFD
Device Information subelement. If that is the information you want, more
appropriate operation would be to parse that element and include just
the six octets of payload in it.

If you want to maintain this wasteful format with 0x prefix and id+len
fields included for backwards compatibility with existing consumers of
such data, the implementation here would need to verify that the first
sublement is indeed WFD Device Info and only in that case, add this
data. (Or for more completeness, find the WFD Device Info even if it is
not the first subelement.)

-- 
Jouni Malinen                                            PGP id EFC895FA