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

[Dmitry Shmidt]

On Sat, Nov 23, 2013 at 2:42 PM, Jouni Malinen <j at 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 at lists.shmoo.com
> http://lists.shmoo.com/mailman/listinfo/hostap