[PATCH v2 0/3] LE OOB pairing support

[PATCH v2 0/3] LE OOB pairing support

[Michael Brudevold]

From: Michael Brudevold <michael.brudevold@veranexsolutions.com>

This patch series implements userspace support for LE OOB pairing. It was
tested against an nRF52 dev kit with Nordic's NFC pairing example. Support
is only for reading a tag; generating and sending back OOB information was
not implemented.

Overall, LE EIR data is not dissimilar to BREDR, but the OOB blob starts
off slightly differently necessitating a different code path before
reaching the EIR parser.

Existing BREDR support was only for P-192 and that remains unchanged,
though enough structure was added for P-256 support for LE. This was the
change from the original patch series, where only the P-192 path existed.

Michael Brudevold (3):
  eir: parse data types for LE OOB pairing
  Accept LE formatted EIR data with neard plugin
  neard: Update D-Bus path and interface

 plugins/neard.c | 94 +++++++++++++++++++++++++++++++++++++++----------
 src/adapter.c   | 19 +++++++---
 src/adapter.h   |  5 +--
 src/eir.c       | 41 +++++++++++++++++----
 src/eir.h       | 10 ++++--
 5 files changed, 135 insertions(+), 34 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] eir: parse data types for LE OOB pairing

[Michael Brudevold]

From: Michael Brudevold <michael.brudevold@veranexsolutions.com>

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

diff --git a/plugins/neard.c b/plugins/neard.c
index 99762482c..11d9e91c4 100644
--- a/plugins/neard.c
+++ b/plugins/neard.c
@@ -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);
 
diff --git a/src/eir.c b/src/eir.c
index 2f9ee036f..79d423074 100644
--- a/src/eir.c
+++ b/src/eir.c
@@ -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;
diff --git a/src/eir.h b/src/eir.h
index 6154e23ec..b2cf00f37 100644
--- a/src/eir.h
+++ b/src/eir.h
@@ -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

[PATCH v2 2/3] Accept LE formatted EIR data with neard plugin

[Michael Brudevold]

From: Michael Brudevold <michael.brudevold@veranexsolutions.com>

LE EIR differs from BREDR EIR in that it does not start with the device
address. Add ability to handle this data and send the correct address type
when adding remote OOB.

BREDR support remains for P-192; P-256 is not added.

This patch does not address requesting LE OOB data.
---
 plugins/neard.c | 86 ++++++++++++++++++++++++++++++++++++++++---------
 src/adapter.c   | 19 ++++++++---
 src/adapter.h   |  5 +--
 3 files changed, 88 insertions(+), 22 deletions(-)

diff --git a/plugins/neard.c b/plugins/neard.c
index 11d9e91c4..e03f981e0 100644
--- a/plugins/neard.c
+++ b/plugins/neard.c
@@ -56,11 +56,14 @@ enum cps {
 
 struct oob_params {
 	bdaddr_t address;
+	uint8_t address_type;
 	uint32_t class;
 	char *name;
 	GSList *services;
-	uint8_t *hash;
-	uint8_t *randomizer;
+	uint8_t *hash192;
+	uint8_t *randomizer192;
+	uint8_t *hash256;
+	uint8_t *randomizer256;
 	uint8_t *pin;
 	int pin_len;
 	enum cps power_state;
@@ -70,8 +73,10 @@ static void free_oob_params(struct oob_params *params)
 {
 	g_slist_free_full(params->services, free);
 	g_free(params->name);
-	g_free(params->hash);
-	g_free(params->randomizer);
+	g_free(params->hash192);
+	g_free(params->randomizer192);
+	g_free(params->hash256);
+	g_free(params->randomizer256);
 	free(params->pin);
 }
 
@@ -352,10 +357,10 @@ 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.hash192;
+	remote->hash192 = eir_data.hash192;
 	eir_data.hash192 = NULL;
 
-	remote->randomizer = eir_data.randomizer192;
+	remote->randomizer192 = eir_data.randomizer192;
 	eir_data.randomizer192 = NULL;
 
 	eir_data_free(&eir_data);
@@ -363,6 +368,36 @@ static int process_eir(uint8_t *eir, size_t size, struct oob_params *remote)
 	return 0;
 }
 
+static void process_eir_le(uint8_t *eir, size_t size, struct oob_params *remote)
+{
+	struct eir_data eir_data;
+
+	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;
+
+	remote->randomizer256 = eir_data.randomizer256;
+	eir_data.randomizer256 = NULL;
+
+	eir_data_free(&eir_data);
+}
+
 /*
  * This is (barely documented) Nokia extension format, most work was done by
  * reverse engineering.
@@ -543,7 +578,7 @@ static int process_message(DBusMessage *msg, struct oob_params *remote)
 			uint8_t *eir;
 			int size;
 
-			/* nokia.com:bt and EIR should not be passed together */
+			/* nokia.com:bt, EIR, and EIR.le should not be passed together */
 			if (bacmp(&remote->address, BDADDR_ANY) != 0)
 				goto error;
 
@@ -561,7 +596,7 @@ static int process_message(DBusMessage *msg, struct oob_params *remote)
 			uint8_t *data;
 			int size;
 
-			/* nokia.com:bt and EIR should not be passed together */
+			/* nokia.com:bt, EIR, and EIR.le should not be passed together */
 			if (bacmp(&remote->address, BDADDR_ANY) != 0)
 				goto error;
 
@@ -574,6 +609,23 @@ static int process_message(DBusMessage *msg, struct oob_params *remote)
 
 			if (process_nokia_com_bt(data, size, remote))
 				goto error;
+		} else if (strcasecmp(key, "EIR.le") == 0) {
+			DBusMessageIter array;
+			uint8_t *eir;
+			int size;
+
+			/* nokia.com:bt, EIR, and EIR.le should not be passed together */
+			if (bacmp(&remote->address, BDADDR_ANY) != 0)
+				goto error;
+
+			if (dbus_message_iter_get_arg_type(&value) !=
+					DBUS_TYPE_ARRAY)
+				goto error;
+
+			dbus_message_iter_recurse(&value, &array);
+			dbus_message_iter_get_fixed_array(&array, &eir, &size);
+
+			process_eir_le(eir, size, remote);
 		} else if (strcasecmp(key, "State") == 0) {
 			const char *state;
 
@@ -635,10 +687,13 @@ static void store_params(struct btd_adapter *adapter, struct btd_device *device,
 	if (params->services)
 		device_add_eir_uuids(device, params->services);
 
-	if (params->hash) {
+	if (params->hash192 || params->hash256) {
 		btd_adapter_add_remote_oob_data(adapter, &params->address,
-							params->hash,
-							params->randomizer);
+							params->address_type,
+							params->hash192,
+							params->randomizer192,
+							params->hash256,
+							params->randomizer256);
 	} else if (params->pin_len) {
 		/* TODO
 		 * Handle PIN, for now only discovery mode and 'common' PINs
@@ -692,7 +747,7 @@ static DBusMessage *push_oob(DBusConnection *conn, DBusMessage *msg, void *data)
 	}
 
 	device = btd_adapter_get_device(adapter, &remote.address,
-								BDADDR_BREDR);
+								remote.address_type);
 
 	err = check_device(device);
 	if (err < 0) {
@@ -716,7 +771,7 @@ static DBusMessage *push_oob(DBusConnection *conn, DBusMessage *msg, void *data)
 	free_oob_params(&remote);
 
 	err = adapter_create_bonding(adapter, device_get_address(device),
-							BDADDR_BREDR, io_cap);
+							remote.address_type, io_cap);
 	if (err < 0)
 		return error_reply(msg, -err);
 
@@ -764,7 +819,8 @@ static DBusMessage *request_oob(DBusConnection *conn, DBusMessage *msg,
 		goto done;
 	}
 
-	device = btd_adapter_get_device(adapter, &remote.address, BDADDR_BREDR);
+	device = btd_adapter_get_device(adapter, &remote.address,
+							   remote.address_type);
 
 	err = check_device(device);
 	if (err < 0)
@@ -777,7 +833,7 @@ static DBusMessage *request_oob(DBusConnection *conn, DBusMessage *msg,
 
 	store_params(adapter, device, &remote);
 
-	if (remote.hash && btd_adapter_get_powered(adapter))
+	if (remote.hash192 && btd_adapter_get_powered(adapter))
 		goto read_local;
 done:
 	free_oob_params(&remote);
diff --git a/src/adapter.c b/src/adapter.c
index f7faaa263..9cad36da8 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -8770,7 +8770,11 @@ int adapter_set_io_capability(struct btd_adapter *adapter, uint8_t io_cap)
 
 int btd_adapter_add_remote_oob_data(struct btd_adapter *adapter,
 					const bdaddr_t *bdaddr,
-					uint8_t *hash, uint8_t *randomizer)
+					uint8_t bdaddr_type,
+					uint8_t *hash192,
+					uint8_t *randomizer192,
+					uint8_t *hash256,
+					uint8_t *randomizer256)
 {
 	struct mgmt_cp_add_remote_oob_data cp;
 	char addr[18];
@@ -8780,10 +8784,15 @@ int btd_adapter_add_remote_oob_data(struct btd_adapter *adapter,
 
 	memset(&cp, 0, sizeof(cp));
 	bacpy(&cp.addr.bdaddr, bdaddr);
-	memcpy(cp.hash192, hash, 16);
-
-	if (randomizer)
-		memcpy(cp.rand192, randomizer, 16);
+	cp.addr.type = bdaddr_type;
+	if (hash192)
+		memcpy(cp.hash192, hash192, 16);
+	if (hash256)
+		memcpy(cp.hash256, hash256, 16);
+	if (randomizer192)
+		memcpy(cp.rand192, randomizer192, 16);
+	if (randomizer256)
+		memcpy(cp.rand256, randomizer256, 16);
 
 	if (mgmt_send(adapter->mgmt, MGMT_OP_ADD_REMOTE_OOB_DATA,
 				adapter->dev_id, sizeof(cp), &cp,
diff --git a/src/adapter.h b/src/adapter.h
index 688ed51c6..b0a324a78 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -213,8 +213,9 @@ int adapter_set_io_capability(struct btd_adapter *adapter, uint8_t io_cap);
 int btd_adapter_read_local_oob_data(struct btd_adapter *adapter);
 
 int btd_adapter_add_remote_oob_data(struct btd_adapter *adapter,
-					const bdaddr_t *bdaddr,
-					uint8_t *hash, uint8_t *randomizer);
+					const bdaddr_t *bdaddr, uint8_t bdaddr_type,
+					uint8_t *hash192, uint8_t *randomizer192,
+					uint8_t *hash256, uint8_t *randomizer256);
 
 int btd_adapter_remove_remote_oob_data(struct btd_adapter *adapter,
 							const bdaddr_t *bdaddr);
-- 
2.25.1

[PATCH v2 3/3] neard: Update D-Bus path and interface

[Michael Brudevold]

From: Michael Brudevold <michael.brudevold@veranexsolutions.com>

neard has been carrying legacy interfaces since 2013
---
 plugins/neard.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/plugins/neard.c b/plugins/neard.c
index e03f981e0..77a4630da 100644
--- a/plugins/neard.c
+++ b/plugins/neard.c
@@ -33,8 +33,8 @@
 #include "src/shared/util.h"
 
 #define NEARD_NAME "org.neard"
-#define NEARD_PATH "/"
-#define NEARD_MANAGER_INTERFACE "org.neard.Manager"
+#define NEARD_PATH "/org/neard"
+#define NEARD_MANAGER_INTERFACE "org.neard.AgentManager"
 #define AGENT_INTERFACE "org.neard.HandoverAgent"
 #define AGENT_PATH "/org/bluez/neard_handover_agent"
 #define AGENT_CARRIER_TYPE "bluetooth"
-- 
2.25.1

RE: LE OOB pairing support

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3950 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=649437

---Test result---

Test Summary:
CheckPatch                    FAIL      4.40 seconds
GitLint                       PASS      2.99 seconds
Prep - Setup ELL              PASS      43.55 seconds
Build - Prep                  PASS      0.74 seconds
Build - Configure             PASS      8.77 seconds
Build - Make                  PASS      1434.98 seconds
Make Check                    PASS      11.93 seconds
Make Check w/Valgrind         PASS      450.60 seconds
Make Distcheck                PASS      234.96 seconds
Build w/ext ELL - Configure   PASS      8.77 seconds
Build w/ext ELL - Make        PASS      1408.11 seconds
Incremental Build with patchesPASS      4282.77 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
[v2,1/3] eir: parse data types for LE OOB pairing
WARNING:LONG_LINE_COMMENT: line length of 89 exceeds 80 columns
#202: FILE: src/eir.h:40:
+#define EIR_LE_SC_CONF              0x22  /* LE: Secure Connections Confirmation Value */

WARNING:LONG_LINE_COMMENT: line length of 83 exceeds 80 columns
#203: FILE: src/eir.h:41:
+#define EIR_LE_SC_RAND              0x23  /* LE: Secure Connections Random Value */

/github/workspace/src/12878130.patch total: 0 errors, 2 warnings, 111 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/12878130.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

[v2,2/3] Accept LE formatted EIR data with neard plugin
WARNING:LONG_LINE_COMMENT: line length of 89 exceeds 80 columns
#184: FILE: plugins/neard.c:581:
+			/* nokia.com:bt, EIR, and EIR.le should not be passed together */

WARNING:LONG_LINE_COMMENT: line length of 89 exceeds 80 columns
#193: FILE: plugins/neard.c:599:
+			/* nokia.com:bt, EIR, and EIR.le should not be passed together */

WARNING:LONG_LINE_COMMENT: line length of 89 exceeds 80 columns
#206: FILE: plugins/neard.c:617:
+			/* nokia.com:bt, EIR, and EIR.le should not be passed together */

WARNING:LONG_LINE: line length of 85 exceeds 80 columns
#243: FILE: plugins/neard.c:750:
+								remote.address_type);

WARNING:LONG_LINE: line length of 85 exceeds 80 columns
#252: FILE: plugins/neard.c:774:
+							remote.address_type, io_cap);

WARNING:LONG_LINE: line length of 84 exceeds 80 columns
#322: FILE: src/adapter.h:216:
+					const bdaddr_t *bdaddr, uint8_t bdaddr_type,

WARNING:LONG_LINE: line length of 81 exceeds 80 columns
#323: FILE: src/adapter.h:217:
+					uint8_t *hash192, uint8_t *randomizer192,

WARNING:LONG_LINE: line length of 82 exceeds 80 columns
#324: FILE: src/adapter.h:218:
+					uint8_t *hash256, uint8_t *randomizer256);

/github/workspace/src/12878131.patch total: 0 errors, 8 warnings, 206 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/12878131.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.




---
Regards,
Linux Bluetooth

Re: [PATCH v2 1/3] eir: parse data types for LE OOB pairing

[Luiz Augusto von Dentz]

Hi Michael,

On Fri, Jun 10, 2022 at 3:49 PM Michael Brudevold <puffy.taco@gmail.com> wrote:
>
> From: Michael Brudevold <michael.brudevold@veranexsolutions.com>
>
> 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(-)
>
> diff --git a/plugins/neard.c b/plugins/neard.c
> index 99762482c..11d9e91c4 100644
> --- a/plugins/neard.c
> +++ b/plugins/neard.c
> @@ -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);
>
> diff --git a/src/eir.c b/src/eir.c
> index 2f9ee036f..79d423074 100644
> --- a/src/eir.c
> +++ b/src/eir.c
> @@ -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;
> diff --git a/src/eir.h b/src/eir.h
> index 6154e23ec..b2cf00f37 100644
> --- a/src/eir.h
> +++ b/src/eir.h
> @@ -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

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.



-- 
Luiz Augusto von Dentz

Re: [PATCH v2 1/3] eir: parse data types for LE OOB pairing

[Mike Brudevold]

Hi Luiz,

On Sat, Jun 11, 2022 at 3:27 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Michael,
>
> On Fri, Jun 10, 2022 at 3:49 PM Michael Brudevold <puffy.taco@gmail.com> wrote:
> >
> > From: Michael Brudevold <michael.brudevold@veranexsolutions.com>
> >
> > 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(-)
> >
> > diff --git a/plugins/neard.c b/plugins/neard.c
> > index 99762482c..11d9e91c4 100644
> > --- a/plugins/neard.c
> > +++ b/plugins/neard.c
> > @@ -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);
> >
> > diff --git a/src/eir.c b/src/eir.c
> > index 2f9ee036f..79d423074 100644
> > --- a/src/eir.c
> > +++ b/src/eir.c
> > @@ -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;
> > diff --git a/src/eir.h b/src/eir.h
> > index 6154e23ec..b2cf00f37 100644
> > --- a/src/eir.h
> > +++ b/src/eir.h
> > @@ -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
>
> 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
--- a/plugins/neard.c
+++ b/plugins/neard.c
@@ -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);
+               break;
+       }
+}
+
 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);
+       }
 }

Re: [PATCH v2 1/3] eir: parse data types for LE OOB pairing

[Luiz Augusto von Dentz]

Hi Mike,

On Mon, Jun 13, 2022 at 2:42 PM Mike Brudevold <puffy.taco@gmail.com> wrote:
>
> Hi Luiz,
>
> > 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.

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.

> index 77a4630da..3d4064515 100644
> --- a/plugins/neard.c
> +++ b/plugins/neard.c
> @@ -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);
> +               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?

>  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);
> +       }
>  }



-- 
Luiz Augusto von Dentz

Re: [PATCH v2 1/3] eir: parse data types for LE OOB pairing

[Mike Brudevold]

Hi Luiz,

On Mon, Jun 13, 2022 at 4:55 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Mike,
>
> On Mon, Jun 13, 2022 at 2:42 PM Mike Brudevold <puffy.taco@gmail.com> wrote:
> >
> > Hi Luiz,
> >
> > > 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.
>
> 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.
>
> > index 77a4630da..3d4064515 100644
> > --- a/plugins/neard.c
> > +++ b/plugins/neard.c
> > @@ -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);
> > +               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.

>
> >  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);
> > +       }
> >  }
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v2 1/3] eir: parse data types for LE OOB pairing

[Luiz Augusto von Dentz]

Hi Mike,

On Mon, Jun 13, 2022 at 3:28 PM Mike Brudevold <puffy.taco@gmail.com> wrote:
>
> Hi Luiz,
>
> On Mon, Jun 13, 2022 at 4:55 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Mike,
> >
> > On Mon, Jun 13, 2022 at 2:42 PM Mike Brudevold <puffy.taco@gmail.com> wrote:
> > >
> > > Hi Luiz,
> > >
> > > > 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.
> >
> > 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.
> >
> > > index 77a4630da..3d4064515 100644
> > > --- a/plugins/neard.c
> > > +++ b/plugins/neard.c
> > > @@ -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);
> > > +               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.

Are you missing some feedback on these changes?

> >
> > >  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);
> > > +       }
> > >  }
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH v2 1/3] eir: parse data types for LE OOB pairing

[Mike Brudevold]

Hi Luiz,

On Tue, Jun 21, 2022 at 1:57 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Mike,
>
> On Mon, Jun 13, 2022 at 3:28 PM Mike Brudevold <puffy.taco@gmail.com> wrote:
> >
> > Hi Luiz,
> >
> > On Mon, Jun 13, 2022 at 4:55 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Mike,
> > >
> > > On Mon, Jun 13, 2022 at 2:42 PM Mike Brudevold <puffy.taco@gmail.com> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > > 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.
> > >
> > > 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.
> > >
> > > > index 77a4630da..3d4064515 100644
> > > > --- a/plugins/neard.c
> > > > +++ b/plugins/neard.c
> > > > @@ -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);
> > > > +               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.
>
> Are you missing some feedback on these changes?

If you have any, I welcome them.  I have an idea of what I would do to
augment struct bt_ad by making bt_ad_new_with_data smarter to parse
out to other queues/data values, but it hasn't been the highest
priority so I haven't put any time into it.

>
> > >
> > > >  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);
> > > > +       }
> > > >  }
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz