Patchwork P2P: WFd: Add wfd_dev_info= field for device found event

login
register
mail settings
Submitter Dmitry Shmidt
Date Nov. 23, 2013, 12:39 a.m.
Message ID <20131123004132.B2FE013FDEA@ushik.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/293615/
State Changes Requested
Headers show

Comments

Dmitry Shmidt - Nov. 23, 2013, 12:39 a.m.
Signed-off-by: Dmitry Shmidt <dimitrysh@google.com>
---
 wpa_supplicant/p2p_supplicant.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)
Jouni Malinen - Nov. 23, 2013, 10:42 p.m.
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.)
Dmitry Shmidt - Nov. 25, 2013, 11:24 p.m.
On Sat, Nov 23, 2013 at 2:42 PM, Jouni Malinen <j@w1.fi> wrote:
> 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.

You are right, issue should be addressed.

>
>>       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.

Done:
http://patchwork.ozlabs.org/patch/294137/

>
> 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.)

Adding this info allows to see if WFD exists without using P2P_PEER call per
found P2P device.

>
> --
> Jouni Malinen                                            PGP id EFC895FA
> _______________________________________________
> HostAP mailing list
> HostAP@lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap

Patch

diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c
index fecc259..1ef0391 100644
--- 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);
+	}
+#endif /* CONFIG_WIFI_DISPLAY */
 	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);
 #endif /* CONFIG_NO_STDOUT_DEBUG */
 
 	wpas_notify_p2p_device_found(ctx, info->p2p_device_addr, new_device);