LE OOB pairing support

From: Michael Brudevold <[email protected]>

LE support added for P-256 and split out from existing BREDR support for P-192

Also attempt to free any existing values before setting new values

---
 plugins/neard.c |  8 ++++----
 src/eir.c       | 41 +++++++++++++++++++++++++++++++++++------
 src/eir.h       | 10 ++++++++--
 3 files changed, 47 insertions(+), 12 deletions(-)

plugins/neard.c +4 -4

index 99762482c..11d9e91c4 100644
@@ -352,11 +352,11 @@ static int process_eir(uint8_t *eir, size_t size, struct oob_params *remote)
        remote->services = eir_data.services;
        eir_data.services = NULL;
 
-       remote->hash = eir_data.hash;
-       eir_data.hash = NULL;
+       remote->hash = eir_data.hash192;
+       eir_data.hash192 = NULL;
 
-       remote->randomizer = eir_data.randomizer;
-       eir_data.randomizer = NULL;
+       remote->randomizer = eir_data.randomizer192;
+       eir_data.randomizer192 = NULL;
 
        eir_data_free(&eir_data);
 

src/eir.c +35 -6

index 2f9ee036f..79d423074 100644
@@ -53,10 +53,14 @@ void eir_data_free(struct eir_data *eir)
        eir->services = NULL;
        g_free(eir->name);
        eir->name = NULL;
-       free(eir->hash);
-       eir->hash = NULL;
-       free(eir->randomizer);
-       eir->randomizer = NULL;
+       free(eir->hash192);
+       eir->hash192 = NULL;
+       free(eir->randomizer192);
+       eir->randomizer192 = NULL;
+       free(eir->hash256);
+       eir->hash256 = NULL;
+       free(eir->randomizer256);
+       eir->randomizer256 = NULL;
        g_slist_free_full(eir->msd_list, g_free);
        eir->msd_list = NULL;
        g_slist_free_full(eir->sd_list, sd_free);

@@ -323,13 +327,15 @@ void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
                case EIR_SSP_HASH:
                        if (data_len < 16)
                                break;
-                       eir->hash = util_memdup(data, 16);
+                       free(eir->hash192);
+                       eir->hash192 = util_memdup(data, 16);
                        break;
 
                case EIR_SSP_RANDOMIZER:
                        if (data_len < 16)
                                break;
-                       eir->randomizer = util_memdup(data, 16);
+                       free(eir->randomizer192);
+                       eir->randomizer192 = util_memdup(data, 16);
                        break;
 
                case EIR_DEVICE_ID:

@@ -342,6 +348,15 @@ void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
                        eir->did_version = data[6] | (data[7] << 8);
                        break;
 
+               case EIR_LE_DEVICE_ADDRESS:
+                       if (data_len < sizeof(bdaddr_t) + 1)
+                               break;
+
+                       memcpy(&eir->addr, data, sizeof(bdaddr_t));
+                       eir->addr_type = data[sizeof(bdaddr_t)] & 0x1 ?
+                                       BDADDR_LE_RANDOM : BDADDR_LE_PUBLIC;
+                       break;
+
                case EIR_SVC_DATA16:
                        eir_parse_uuid16_data(eir, data, data_len);
                        break;

@@ -354,6 +369,20 @@ void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
                        eir_parse_uuid128_data(eir, data, data_len);
                        break;
 
+               case EIR_LE_SC_CONF:
+                       if (data_len < 16)
+                               break;
+                       free(eir->hash256);
+                       eir->hash256 = util_memdup(data, 16);
+                       break;
+
+               case EIR_LE_SC_RAND:
+                       if (data_len < 16)
+                               break;
+                       free(eir->randomizer256);
+                       eir->randomizer256 = util_memdup(data, 16);
+                       break;
+
                case EIR_MANUFACTURER_DATA:
                        eir_parse_msd(eir, data, data_len);
                        break;

src/eir.h +8 -3

index 6154e23ec..b2cf00f37 100644
@@ -33,9 +33,12 @@
 #define EIR_PUB_TRGT_ADDR           0x17  /* LE: Public Target Address */
 #define EIR_RND_TRGT_ADDR           0x18  /* LE: Random Target Address */
 #define EIR_GAP_APPEARANCE          0x19  /* GAP appearance */
+#define EIR_LE_DEVICE_ADDRESS       0x1B  /* LE: Bluetooth Device Address */
 #define EIR_SOLICIT32               0x1F  /* LE: Solicit UUIDs, 32-bit */
 #define EIR_SVC_DATA32              0x20  /* LE: Service data, 32-bit UUID */
 #define EIR_SVC_DATA128             0x21  /* LE: Service data, 128-bit UUID */
+#define EIR_LE_SC_CONF              0x22  /* LE: Secure Connections Confirmation Value */
+#define EIR_LE_SC_RAND              0x23  /* LE: Secure Connections Random Value */
 #define EIR_TRANSPORT_DISCOVERY     0x26  /* Transport Discovery Service */
 #define EIR_MANUFACTURER_DATA       0xFF  /* Manufacturer Specific Data */
 

@@ -77,9 +80,12 @@ struct eir_data {
        uint16_t appearance;
        bool name_complete;
        int8_t tx_power;
-       uint8_t *hash;
-       uint8_t *randomizer;
+       uint8_t *hash192;
+       uint8_t *randomizer192;
+       uint8_t *hash256;
+       uint8_t *randomizer256;
        bdaddr_t addr;
+       uint8_t addr_type;
        uint16_t did_vendor;
        uint16_t did_product;
        uint16_t did_version;
--

2.25.1

Hi Michael,

Quoted 141 lines · Michael Brudevold

From: Michael Brudevold <[email protected]>

LE support added for P-256 and split out from existing BREDR support for P-192

Also attempt to free any existing values before setting new values

---
 plugins/neard.c |  8 ++++----
 src/eir.c       | 41 +++++++++++++++++++++++++++++++++++------
 src/eir.h       | 10 ++++++++--
 3 files changed, 47 insertions(+), 12 deletions(-)

plugins/neard.c +4 -4

index 99762482c..11d9e91c4 100644
@@ -352,11 +352,11 @@ static int process_eir(uint8_t *eir, size_t size, struct oob_params *remote)
        remote->services = eir_data.services;
        eir_data.services = NULL;
 
-       remote->hash = eir_data.hash;
-       eir_data.hash = NULL;
+       remote->hash = eir_data.hash192;
+       eir_data.hash192 = NULL;
 
-       remote->randomizer = eir_data.randomizer;
-       eir_data.randomizer = NULL;
+       remote->randomizer = eir_data.randomizer192;
+       eir_data.randomizer192 = NULL;
 
        eir_data_free(&eir_data);
 

src/eir.c +35 -6

index 2f9ee036f..79d423074 100644
@@ -53,10 +53,14 @@ void eir_data_free(struct eir_data *eir)
        eir->services = NULL;
        g_free(eir->name);
        eir->name = NULL;
-       free(eir->hash);
-       eir->hash = NULL;
-       free(eir->randomizer);
-       eir->randomizer = NULL;
+       free(eir->hash192);
+       eir->hash192 = NULL;
+       free(eir->randomizer192);
+       eir->randomizer192 = NULL;
+       free(eir->hash256);
+       eir->hash256 = NULL;
+       free(eir->randomizer256);
+       eir->randomizer256 = NULL;
        g_slist_free_full(eir->msd_list, g_free);
        eir->msd_list = NULL;
        g_slist_free_full(eir->sd_list, sd_free);

@@ -323,13 +327,15 @@ void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
                case EIR_SSP_HASH:
                        if (data_len < 16)
                                break;
-                       eir->hash = util_memdup(data, 16);
+                       free(eir->hash192);
+                       eir->hash192 = util_memdup(data, 16);
                        break;
 
                case EIR_SSP_RANDOMIZER:
                        if (data_len < 16)
                                break;
-                       eir->randomizer = util_memdup(data, 16);
+                       free(eir->randomizer192);
+                       eir->randomizer192 = util_memdup(data, 16);
                        break;
 
                case EIR_DEVICE_ID:

@@ -342,6 +348,15 @@ void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
                        eir->did_version = data[6] | (data[7] << 8);
                        break;
 
+               case EIR_LE_DEVICE_ADDRESS:
+                       if (data_len < sizeof(bdaddr_t) + 1)
+                               break;
+
+                       memcpy(&eir->addr, data, sizeof(bdaddr_t));
+                       eir->addr_type = data[sizeof(bdaddr_t)] & 0x1 ?
+                                       BDADDR_LE_RANDOM : BDADDR_LE_PUBLIC;
+                       break;
+
                case EIR_SVC_DATA16:
                        eir_parse_uuid16_data(eir, data, data_len);
                        break;

@@ -354,6 +369,20 @@ void eir_parse(struct eir_data *eir, const uint8_t *eir_data, uint8_t eir_len)
                        eir_parse_uuid128_data(eir, data, data_len);
                        break;
 
+               case EIR_LE_SC_CONF:
+                       if (data_len < 16)
+                               break;
+                       free(eir->hash256);
+                       eir->hash256 = util_memdup(data, 16);
+                       break;
+
+               case EIR_LE_SC_RAND:
+                       if (data_len < 16)
+                               break;
+                       free(eir->randomizer256);
+                       eir->randomizer256 = util_memdup(data, 16);
+                       break;
+
                case EIR_MANUFACTURER_DATA:
                        eir_parse_msd(eir, data, data_len);
                        break;

src/eir.h +8 -2

index 6154e23ec..b2cf00f37 100644
@@ -33,9 +33,12 @@
 #define EIR_PUB_TRGT_ADDR           0x17  /* LE: Public Target Address */
 #define EIR_RND_TRGT_ADDR           0x18  /* LE: Random Target Address */
 #define EIR_GAP_APPEARANCE          0x19  /* GAP appearance */
+#define EIR_LE_DEVICE_ADDRESS       0x1B  /* LE: Bluetooth Device Address */
 #define EIR_SOLICIT32               0x1F  /* LE: Solicit UUIDs, 32-bit */
 #define EIR_SVC_DATA32              0x20  /* LE: Service data, 32-bit UUID */
 #define EIR_SVC_DATA128             0x21  /* LE: Service data, 128-bit UUID */
+#define EIR_LE_SC_CONF              0x22  /* LE: Secure Connections Confirmation Value */
+#define EIR_LE_SC_RAND              0x23  /* LE: Secure Connections Random Value */
 #define EIR_TRANSPORT_DISCOVERY     0x26  /* Transport Discovery Service */
 #define EIR_MANUFACTURER_DATA       0xFF  /* Manufacturer Specific Data */
 

@@ -77,9 +80,12 @@ struct eir_data {
        uint16_t appearance;
        bool name_complete;
        int8_t tx_power;
-       uint8_t *hash;
-       uint8_t *randomizer;
+       uint8_t *hash192;
+       uint8_t *randomizer192;
+       uint8_t *hash256;
+       uint8_t *randomizer256;
        bdaddr_t addr;
+       uint8_t addr_type;
        uint16_t did_vendor;

Quoted 4 lines

src/eir.h +0 -1

        uint16_t did_product;
        uint16_t did_version;
--

2.25.1

It might be better to handle this via bt_ad instance instead of eir_data, in fact the plan was always to switch to bt_ad but it seems we forgot about it at some point.

Hi Luiz,

Quoted 3 lines

It might be better to handle this via bt_ad instance instead of eir_data, in fact the plan was always to switch to bt_ad but it seems we forgot about it at some point.

Are you thinking something like below (doesn't fully compile, name2utf8 is static in eir so I did nothing about that yet)? Basically where the ad code parses the EIR data, but the neard plugin still manages what to do with the data? The alternative would be where device.c became smarter to consume the ad struct itself and the neard plugin becomes a simple conduit for the ad data.

index 77a4630da..3d4064515 100644

plugins/neard.c +36 -0

@@ -31,6 +31,7 @@
 #include "src/agent.h"
 #include "src/btd.h"
 #include "src/shared/util.h"
+#include "src/shared/ad.h"
 
 #define NEARD_NAME "org.neard"
 #define NEARD_PATH "/org/neard"

@@ -336,6 +337,52 @@ static int check_device(struct btd_device *device)
        return 0;
 }
 
+static void process_oob_adv(void *data, void *user_data)
+{
+       struct bt_ad_data *ad_data = data;
+       struct oob_params *remote = user_data;
+       uint8_t name_len;
+
+       switch (ad_data->type) {
+       case EIR_NAME_SHORT:
+       case EIR_NAME_COMPLETE:
+               name_len = ad_data->len;
+
+               /* Some vendors put a NUL byte terminator into
+                       * the name */
+               while (name_len > 0 && ad_data->data[name_len - 1] == '\0')
+                       name_len--;
+
+               g_free(remote->name);
+
+               remote->name = name2utf8(ad_data->data, name_len);
+               break;
+
+       case BT_AD_LE_DEVICE_ADDRESS:
+               if (ad_data->len < sizeof(bdaddr_t) + 1)
+                       break;
+
+               memcpy(&remote->address, ad_data->data, sizeof(bdaddr_t));
+               remote->address_type = ad_data->data[sizeof(bdaddr_t)] & 0x1 ?
+                               BDADDR_LE_RANDOM : BDADDR_LE_PUBLIC;
+               break;
+
+       case EIR_LE_SC_CONF:
+               if (ad_data->len < 16)
+                       break;
+               free(remote->hash256);
+               remote->hash256 = util_memdup(ad_data->data, 16);

plugins/neard.c +6 -21

 static int process_eir(uint8_t *eir, size_t size, struct oob_params *remote)
 {
        struct eir_data eir_data;

@@ -370,32 +417,17 @@ static int process_eir(uint8_t *eir, size_t
 size, struct oob_params *remote)
 
 static void process_eir_le(uint8_t *eir, size_t size, struct
 oob_params *remote)
 {
-       struct eir_data eir_data;
+       struct bt_ad *ad;
 
        DBG("size %zu", size);
 
-       memset(&eir_data, 0, sizeof(eir_data));
-
-       eir_parse(&eir_data, eir, size);
-
-       bacpy(&remote->address, &eir_data.addr);
-       remote->address_type = eir_data.addr_type;
-
-       remote->class = eir_data.class;
-
-       remote->name = eir_data.name;
-       eir_data.name = NULL;
-
-       remote->services = eir_data.services;
-       eir_data.services = NULL;
-
-       remote->hash256 = eir_data.hash256;
-       eir_data.hash256 = NULL;
+       ad = bt_ad_new_with_data(size, eir);
 
-       remote->randomizer256 = eir_data.randomizer256;
-       eir_data.randomizer256 = NULL;
+       if (ad) {
+               bt_ad_foreach_data(ad, process_oob_adv, remote);
 
-       eir_data_free(&eir_data);
+               bt_ad_unref(ad);
+       }
 }

Hi Mike,

Quoted 7 lines · Mike Brudevold

Hi Luiz,

Quoted 3 lines

It might be better to handle this via bt_ad instance instead of eir_data, in fact the plan was always to switch to bt_ad but it seems we forgot about it at some point.

Quoted 6 lines

Are you thinking something like below (doesn't fully compile, name2utf8 is static in eir so I did nothing about that yet)? Basically where the ad code parses the EIR data, but the neard plugin still manages what to do with the data? The alternative would be where device.c became smarter to consume the ad struct itself and the neard plugin becomes a simple conduit for the ad data.

The later is probably better alternative, like I said I wrote bt_ad to replace eir handling completely so we could also write proper unit testing for it, Im fine if you want to take the time to change the daemon core itself but at least introduce support for the types you will be using in the plugin and then make use of them.

Quoted 57 lines

index 77a4630da..3d4064515 100644

plugins/neard.c +43 -0

@@ -31,6 +31,7 @@
 #include "src/agent.h"
 #include "src/btd.h"
 #include "src/shared/util.h"
+#include "src/shared/ad.h"
 
 #define NEARD_NAME "org.neard"
 #define NEARD_PATH "/org/neard"

@@ -336,6 +337,52 @@ static int check_device(struct btd_device *device)
        return 0;
 }
 
+static void process_oob_adv(void *data, void *user_data)
+{
+       struct bt_ad_data *ad_data = data;
+       struct oob_params *remote = user_data;
+       uint8_t name_len;
+
+       switch (ad_data->type) {
+       case EIR_NAME_SHORT:
+       case EIR_NAME_COMPLETE:
+               name_len = ad_data->len;
+
+               /* Some vendors put a NUL byte terminator into
+                       * the name */
+               while (name_len > 0 && ad_data->data[name_len - 1] == '\0')
+                       name_len--;
+
+               g_free(remote->name);
+
+               remote->name = name2utf8(ad_data->data, name_len);
+               break;
+
+       case BT_AD_LE_DEVICE_ADDRESS:
+               if (ad_data->len < sizeof(bdaddr_t) + 1)
+                       break;
+
+               memcpy(&remote->address, ad_data->data, sizeof(bdaddr_t));
+               remote->address_type = ad_data->data[sizeof(bdaddr_t)] & 0x1 ?
+                               BDADDR_LE_RANDOM : BDADDR_LE_PUBLIC;
+               break;
+
+       case EIR_LE_SC_CONF:
+               if (ad_data->len < 16)
+                       break;
+               free(remote->hash256);
+               remote->hash256 = util_memdup(ad_data->data, 16);
+               break;
+
+       case EIR_LE_SC_RAND:
+               if (ad_data->len < 16)
+                       break;
+               free(remote->randomizer256);
+               remote->randomizer256 = util_memdup(ad_data->data, 16);

plugins/neard.c static int check_device(struct btd_device *device) · line 384

Quoted 3 lines

plugins/neard.c +3 -0

+               break;
+       }
+}

Do we need to duplicate this information? I'd consider just using the bt_ad instance to parse and store them, well perhaps we want to introduce something like bt_ad_foreach_type so you can locate the data by type?

Quoted 33 lines

plugins/neard.c +2 -18

 static int process_eir(uint8_t *eir, size_t size, struct oob_params *remote)
 {
        struct eir_data eir_data;

@@ -370,32 +417,17 @@ static int process_eir(uint8_t *eir, size_t
 size, struct oob_params *remote)
 
 static void process_eir_le(uint8_t *eir, size_t size, struct
 oob_params *remote)
 {
-       struct eir_data eir_data;
+       struct bt_ad *ad;
 
        DBG("size %zu", size);
 
-       memset(&eir_data, 0, sizeof(eir_data));
-
-       eir_parse(&eir_data, eir, size);
-
-       bacpy(&remote->address, &eir_data.addr);
-       remote->address_type = eir_data.addr_type;
-
-       remote->class = eir_data.class;
-
-       remote->name = eir_data.name;
-       eir_data.name = NULL;
-
-       remote->services = eir_data.services;
-       eir_data.services = NULL;
-
-       remote->hash256 = eir_data.hash256;
-       eir_data.hash256 = NULL;
+       ad = bt_ad_new_with_data(size, eir);

plugins/neard.c static int process_eir(uint8_t *eir, size_t · line 433

Quoted 10 lines

plugins/neard.c +4 -3

 
-       remote->randomizer256 = eir_data.randomizer256;
-       eir_data.randomizer256 = NULL;
+       if (ad) {
+               bt_ad_foreach_data(ad, process_oob_adv, remote);
 
-       eir_data_free(&eir_data);
+               bt_ad_unref(ad);
+       }
 }

--
Luiz Augusto von Dentz

Hi Luiz,

Quoted 81 lines · Luiz Augusto von Dentz

Hi Mike,

Quoted 7 lines · Mike Brudevold

Hi Luiz,

Quoted 3 lines

It might be better to handle this via bt_ad instance instead of eir_data, in fact the plan was always to switch to bt_ad but it seems we forgot about it at some point.

Quoted 6 lines

Are you thinking something like below (doesn't fully compile, name2utf8 is static in eir so I did nothing about that yet)? Basically where the ad code parses the EIR data, but the neard plugin still manages what to do with the data? The alternative would be where device.c became smarter to consume the ad struct itself and the neard plugin becomes a simple conduit for the ad data.

The later is probably better alternative, like I said I wrote bt_ad to replace eir handling completely so we could also write proper unit testing for it, Im fine if you want to take the time to change the daemon core itself but at least introduce support for the types you will be using in the plugin and then make use of them.

Quoted 57 lines

index 77a4630da..3d4064515 100644

plugins/neard.c +43 -0

@@ -31,6 +31,7 @@
 #include "src/agent.h"
 #include "src/btd.h"
 #include "src/shared/util.h"
+#include "src/shared/ad.h"
 
 #define NEARD_NAME "org.neard"
 #define NEARD_PATH "/org/neard"

@@ -336,6 +337,52 @@ static int check_device(struct btd_device *device)
        return 0;
 }
 
+static void process_oob_adv(void *data, void *user_data)
+{
+       struct bt_ad_data *ad_data = data;
+       struct oob_params *remote = user_data;
+       uint8_t name_len;
+
+       switch (ad_data->type) {
+       case EIR_NAME_SHORT:
+       case EIR_NAME_COMPLETE:
+               name_len = ad_data->len;
+
+               /* Some vendors put a NUL byte terminator into
+                       * the name */
+               while (name_len > 0 && ad_data->data[name_len - 1] == '\0')
+                       name_len--;
+
+               g_free(remote->name);
+
+               remote->name = name2utf8(ad_data->data, name_len);
+               break;
+
+       case BT_AD_LE_DEVICE_ADDRESS:
+               if (ad_data->len < sizeof(bdaddr_t) + 1)
+                       break;
+
+               memcpy(&remote->address, ad_data->data, sizeof(bdaddr_t));
+               remote->address_type = ad_data->data[sizeof(bdaddr_t)] & 0x1 ?
+                               BDADDR_LE_RANDOM : BDADDR_LE_PUBLIC;
+               break;
+
+       case EIR_LE_SC_CONF:
+               if (ad_data->len < 16)
+                       break;
+               free(remote->hash256);
+               remote->hash256 = util_memdup(ad_data->data, 16);
+               break;
+
+       case EIR_LE_SC_RAND:
+               if (ad_data->len < 16)
+                       break;
+               free(remote->randomizer256);
+               remote->randomizer256 = util_memdup(ad_data->data, 16);

Quoted 8 lines

plugins/neard.c static int check_device(struct btd_device *device) · line 384

Quoted 3 lines

plugins/neard.c +3 -0

+               break;
+       }
+}

Do we need to duplicate this information? I'd consider just using the bt_ad instance to parse and store them, well perhaps we want to introduce something like bt_ad_foreach_type so you can locate the data by type?

That's partly what I was checking on. The ad code doesn't have much functionality right now to be able to parse the meaning of the data, beyond storing them in TLV format (bt_ad_data). Which is the opposite to how the data is given to ad if you're creating an advertisement (e.g., service UUIDs are stored in bt_uuid_t format inside the service queue when using bt_ad_add_service_uuid, not in the data queue). This means the ad code likely has to get smarter, but I wanted to make sure I wasn't missing something that should have been obvious. If the ad code can present the data back in a usable format, then it wouldn't really have to be duplicated. Otherwise this code would have been an easy way to not use the eir code while the ad code gets developed. With some concern still that there's a type_reject_list related to the data ad queue, but that can be completely bypassed when using bt_ad_new_with_data - so this method is doing something that seems unintended.

Quoted 48 lines

Quoted 33 lines

plugins/neard.c +2 -18

 static int process_eir(uint8_t *eir, size_t size, struct oob_params *remote)
 {
        struct eir_data eir_data;

@@ -370,32 +417,17 @@ static int process_eir(uint8_t *eir, size_t
 size, struct oob_params *remote)
 
 static void process_eir_le(uint8_t *eir, size_t size, struct
 oob_params *remote)
 {
-       struct eir_data eir_data;
+       struct bt_ad *ad;
 
        DBG("size %zu", size);
 
-       memset(&eir_data, 0, sizeof(eir_data));
-
-       eir_parse(&eir_data, eir, size);
-
-       bacpy(&remote->address, &eir_data.addr);
-       remote->address_type = eir_data.addr_type;
-
-       remote->class = eir_data.class;
-
-       remote->name = eir_data.name;
-       eir_data.name = NULL;
-
-       remote->services = eir_data.services;
-       eir_data.services = NULL;
-
-       remote->hash256 = eir_data.hash256;
-       eir_data.hash256 = NULL;
+       ad = bt_ad_new_with_data(size, eir);

plugins/neard.c static int process_eir(uint8_t *eir, size_t · line 433

Quoted 10 lines

plugins/neard.c +4 -3

 
-       remote->randomizer256 = eir_data.randomizer256;
-       eir_data.randomizer256 = NULL;
+       if (ad) {
+               bt_ad_foreach_data(ad, process_oob_adv, remote);
 
-       eir_data_free(&eir_data);
+               bt_ad_unref(ad);
+       }
 }

--
Luiz Augusto von Dentz

Hi Mike,

Quoted 95 lines · Mike Brudevold

Hi Luiz,

Quoted 81 lines · Luiz Augusto von Dentz

Hi Mike,

Quoted 7 lines · Mike Brudevold

Hi Luiz,

Quoted 3 lines

It might be better to handle this via bt_ad instance instead of eir_data, in fact the plan was always to switch to bt_ad but it seems we forgot about it at some point.

Quoted 6 lines

Are you thinking something like below (doesn't fully compile, name2utf8 is static in eir so I did nothing about that yet)? Basically where the ad code parses the EIR data, but the neard plugin still manages what to do with the data? The alternative would be where device.c became smarter to consume the ad struct itself and the neard plugin becomes a simple conduit for the ad data.

The later is probably better alternative, like I said I wrote bt_ad to replace eir handling completely so we could also write proper unit testing for it, Im fine if you want to take the time to change the daemon core itself but at least introduce support for the types you will be using in the plugin and then make use of them.

Quoted 57 lines

index 77a4630da..3d4064515 100644

plugins/neard.c +43 -0

@@ -31,6 +31,7 @@
 #include "src/agent.h"
 #include "src/btd.h"
 #include "src/shared/util.h"
+#include "src/shared/ad.h"
 
 #define NEARD_NAME "org.neard"
 #define NEARD_PATH "/org/neard"

@@ -336,6 +337,52 @@ static int check_device(struct btd_device *device)
        return 0;
 }
 
+static void process_oob_adv(void *data, void *user_data)
+{
+       struct bt_ad_data *ad_data = data;
+       struct oob_params *remote = user_data;
+       uint8_t name_len;
+
+       switch (ad_data->type) {
+       case EIR_NAME_SHORT:
+       case EIR_NAME_COMPLETE:
+               name_len = ad_data->len;
+
+               /* Some vendors put a NUL byte terminator into
+                       * the name */
+               while (name_len > 0 && ad_data->data[name_len - 1] == '\0')
+                       name_len--;
+
+               g_free(remote->name);
+
+               remote->name = name2utf8(ad_data->data, name_len);
+               break;
+
+       case BT_AD_LE_DEVICE_ADDRESS:
+               if (ad_data->len < sizeof(bdaddr_t) + 1)
+                       break;
+
+               memcpy(&remote->address, ad_data->data, sizeof(bdaddr_t));
+               remote->address_type = ad_data->data[sizeof(bdaddr_t)] & 0x1 ?
+                               BDADDR_LE_RANDOM : BDADDR_LE_PUBLIC;
+               break;
+
+       case EIR_LE_SC_CONF:
+               if (ad_data->len < 16)
+                       break;
+               free(remote->hash256);
+               remote->hash256 = util_memdup(ad_data->data, 16);
+               break;
+
+       case EIR_LE_SC_RAND:
+               if (ad_data->len < 16)
+                       break;
+               free(remote->randomizer256);
+               remote->randomizer256 = util_memdup(ad_data->data, 16);

Quoted 8 lines

plugins/neard.c static int check_device(struct btd_device *device) · line 384

Quoted 3 lines

plugins/neard.c +3 -0

+               break;
+       }
+}

Do we need to duplicate this information? I'd consider just using the bt_ad instance to parse and store them, well perhaps we want to introduce something like bt_ad_foreach_type so you can locate the data by type?

Quoted 15 lines

That's partly what I was checking on. The ad code doesn't have much functionality right now to be able to parse the meaning of the data, beyond storing them in TLV format (bt_ad_data). Which is the opposite to how the data is given to ad if you're creating an advertisement (e.g., service UUIDs are stored in bt_uuid_t format inside the service queue when using bt_ad_add_service_uuid, not in the data queue). This means the ad code likely has to get smarter, but I wanted to make sure I wasn't missing something that should have been obvious. If the ad code can present the data back in a usable format, then it wouldn't really have to be duplicated. Otherwise this code would have been an easy way to not use the eir code while the ad code gets developed. With some concern still that there's a type_reject_list related to the data ad queue, but that can be completely bypassed when using bt_ad_new_with_data - so this method is doing something that seems unintended.

Are you missing some feedback on these changes?

Quoted 40 lines

Quoted 40 lines

Quoted 40 lines

plugins/neard.c +4 -20

 static int process_eir(uint8_t *eir, size_t size, struct oob_params *remote)
 {
        struct eir_data eir_data;

@@ -370,32 +417,17 @@ static int process_eir(uint8_t *eir, size_t
 size, struct oob_params *remote)
 
 static void process_eir_le(uint8_t *eir, size_t size, struct
 oob_params *remote)
 {
-       struct eir_data eir_data;
+       struct bt_ad *ad;
 
        DBG("size %zu", size);
 
-       memset(&eir_data, 0, sizeof(eir_data));
-
-       eir_parse(&eir_data, eir, size);
-
-       bacpy(&remote->address, &eir_data.addr);
-       remote->address_type = eir_data.addr_type;
-
-       remote->class = eir_data.class;
-
-       remote->name = eir_data.name;
-       eir_data.name = NULL;
-
-       remote->services = eir_data.services;
-       eir_data.services = NULL;
-
-       remote->hash256 = eir_data.hash256;
-       eir_data.hash256 = NULL;
+       ad = bt_ad_new_with_data(size, eir);
 
-       remote->randomizer256 = eir_data.randomizer256;
-       eir_data.randomizer256 = NULL;
+       if (ad) {
+               bt_ad_foreach_data(ad, process_oob_adv, remote);
 

Quoted 9 lines

Quoted 9 lines

plugins/neard.c static int process_eir(uint8_t *eir, size_t · line 433

Quoted 4 lines

plugins/neard.c +2 -1

-       eir_data_free(&eir_data);
+               bt_ad_unref(ad);
+       }
 }

--
Luiz Augusto von Dentz

--
Luiz Augusto von Dentz