[1/2] adapter: Fix addr_type for smp_irk/ltk_info/link_key

Commit Message

Yao Xiao Dec. 11, 2023, 4:27 p.m. UTC

From: Xiao Yao <[e-mail]>

According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
converted between each other, so the addr type can be either
BREDR or LE, so fix it.

Signed-off-by: Xiao Yao <[e-mail]>
---
 src/adapter.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

[e-mail] Dec. 11, 2023, 6:17 p.m. UTC | #1

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=808867

---Test result---

Test Summary:
CheckPatch                    PASS      0.98 seconds
GitLint                       PASS      0.64 seconds
BuildEll                      PASS      24.31 seconds
BluezMake                     PASS      717.35 seconds
MakeCheck                     PASS      11.35 seconds
MakeDistcheck                 PASS      157.03 seconds
CheckValgrind                 PASS      220.60 seconds
CheckSmatch                   PASS      329.27 seconds
bluezmakeextell               PASS      103.85 seconds
IncrementalBuild              PASS      1351.13 seconds
ScanBuild                     PASS      930.85 seconds



---
Regards,
Linux Bluetooth

[e-mail] Dec. 11, 2023, 6:40 p.m. UTC | #2

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Tue, 12 Dec 2023 00:27:28 +0800 you wrote:
> From: Xiao Yao <[e-mail]>
> 
> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
> converted between each other, so the addr type can be either
> BREDR or LE, so fix it.
> 
> [...]

Here is the summary with links:
  - [1/2] adapter: Fix addr_type for smp_irk/ltk_info/link_key
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=d5536e0cd431
  - [2/2] device: Add notifications when the bdaddr_type changes
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=ba9fda12d26b

You are awesome, thank you!

Robin Candau Dec. 16, 2023, 12:59 a.m. UTC | #3

On 12/11/23 17:27, Xiao Yao wrote:
> From: Xiao Yao<[e-mail]>
>
> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
> converted between each other, so the addr type can be either
> BREDR or LE, so fix it.
>
> Signed-off-by: Xiao Yao<[e-mail]>
> ---
>   src/adapter.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 86fff72bc..ee70b00d2 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
>   
>   struct link_key_info {
>   	bdaddr_t bdaddr;
> +	uint8_t bdaddr_type;
>   	unsigned char key[16];
>   	uint8_t type;
>   	uint8_t pin_len;
> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
>   	return false;
>   }
>   
> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
> +					uint8_t bdaddr_type)
> +
>   {
>   	struct link_key_info *info = NULL;
>   	char *str;
> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>   	info = g_new0(struct link_key_info, 1);
>   
>   	str2ba(peer, &info->bdaddr);
> +	info->bdaddr_type = bdaddr_type;
>   
>   	if (!strncmp(str, "0x", 2))
>   		str2buf(&str[2], info->key, sizeof(info->key));
> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
>   		struct link_key_info *info = l->data;
>   
>   		bacpy(&key->addr.bdaddr, &info->bdaddr);
> -		key->addr.type = BDADDR_BREDR;
> +		key->addr.type = info->bdaddr_type;
>   		key->type = info->type;
>   		memcpy(key->val, info->key, 16);
>   		key->pin_len = info->pin_len;
> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
>   		btd_error(adapter->dev_id, "Load connection parameters failed");
>   }
>   
> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
> +static uint8_t get_addr_type(GKeyFile *keyfile)
>   {
>   	uint8_t addr_type;
>   	char *type;
>   
> +	/* The AddressType is written to file only When dev->le is
> +	 * set to true, as referenced in the update_technologies().
> +	 * Therefore, When type is NULL, it default to BDADDR_BREDR.
> +	 */
>   	type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
>   	if (!type)
> -		return BDADDR_LE_PUBLIC;
> +		return BDADDR_BREDR;
>   
>   	if (g_str_equal(type, "public"))
>   		addr_type = BDADDR_LE_PUBLIC;
> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
>   			g_clear_error(&gerr);
>   		}
>   
> -		key_info = get_key_info(key_file, entry->d_name);
> +		bdaddr_type = get_addr_type(key_file);
>   
> -		bdaddr_type = get_le_addr_type(key_file);
> +		key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>   
>   		ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>   

Hello,

It seems like the above commit introduced a regression where the initial 
auto-connect for Bluetooth devices doesn't work anymore.

Indeed, at system startup, starting a Bluetooth device will cause it to 
go in a "connected/disconnected" state loop, requiring to initialize a 
manual connection first (with sometimes multiple attempts needed) before 
getting it connected correctly and working as intended.

`systemctl status bluetooth` reports the following error:

[...]
déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys 
for hci0: Invalid Parameters (0x0d)
[...]

**I bisected the bug with `git bisect` and it pointed out this commit 
[1] as the "faulty" one.
I can confirm that reverting it solves the issue.

I reported this bug including details in the GitHub repo [2].

I remain available if any additional information are needed.

[1] 
https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
[2] https://github.com/bluez/bluez/issues/686|
|

Luiz Augusto von Dentz Aug. 26, 2024, 9:12 p.m. UTC | #4

Hi Xiao,

On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
>
> On 12/11/23 17:27, Xiao Yao wrote:
>
> From: Xiao Yao <[e-mail]>
>
> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
> converted between each other, so the addr type can be either
> BREDR or LE, so fix it.
>
> Signed-off-by: Xiao Yao <[e-mail]>
> ---
>  src/adapter.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 86fff72bc..ee70b00d2 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
>
>  struct link_key_info {
>   bdaddr_t bdaddr;
> + uint8_t bdaddr_type;
>   unsigned char key[16];
>   uint8_t type;
>   uint8_t pin_len;
> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
>   return false;
>  }
>
> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
> + uint8_t bdaddr_type)
> +
>  {
>   struct link_key_info *info = NULL;
>   char *str;
> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>   info = g_new0(struct link_key_info, 1);
>
>   str2ba(peer, &info->bdaddr);
> + info->bdaddr_type = bdaddr_type;
>
>   if (!strncmp(str, "0x", 2))
>   str2buf(&str[2], info->key, sizeof(info->key));
> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
>   struct link_key_info *info = l->data;
>
>   bacpy(&key->addr.bdaddr, &info->bdaddr);
> - key->addr.type = BDADDR_BREDR;
> + key->addr.type = info->bdaddr_type;
>   key->type = info->type;
>   memcpy(key->val, info->key, 16);
>   key->pin_len = info->pin_len;
> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
>   btd_error(adapter->dev_id, "Load connection parameters failed");
>  }
>
> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
> +static uint8_t get_addr_type(GKeyFile *keyfile)
>  {
>   uint8_t addr_type;
>   char *type;
>
> + /* The AddressType is written to file only When dev->le is
> + * set to true, as referenced in the update_technologies().
> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
> + */
>   type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
>   if (!type)
> - return BDADDR_LE_PUBLIC;
> + return BDADDR_BREDR;
>
>   if (g_str_equal(type, "public"))
>   addr_type = BDADDR_LE_PUBLIC;
> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
>   g_clear_error(&gerr);
>   }
>
> - key_info = get_key_info(key_file, entry->d_name);
> + bdaddr_type = get_addr_type(key_file);
>
> - bdaddr_type = get_le_addr_type(key_file);
> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>
>   ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>
>
> Hello,
>
> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
>
> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
>
> `systemctl status bluetooth` reports the following error:
>
> [...]
> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
> [...]
>
> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
> I can confirm that reverting it solves the issue.
>
> I reported this bug including details in the GitHub repo [2].
>
> I remain available if any additional information are needed.
>
> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
> [2] https://github.com/bluez/bluez/issues/686
>
> --
> Regards,
> Robin Candau / Antiz

Perhaps related to:
https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?

Yao Xiao Aug. 27, 2024, 7:39 a.m. UTC | #5

Hi luiz,
The kernel should always report the correct address type for all kinds 
of keys. Older kernels ignore the derivation of keys, which prevents the 
user layer from knowing the correct address type. This can cause a lot 
of confusion for users. For example, when a user connects to a 
traditional Bluetooth audio device, due to the default activation of key 
derivation, bluetoothd reports both BREDR and BLE pairing and binding, 
and the address is updated to LE public. When the user actively calls 
the Connect method of org.bluez.Device1 to reconnect, sometimes the 
address type is set to LE public, leading to a failed connection, and 
vice versa.

We should enable bluetoothd to know the reason for the generation of 
keys as much as possible. Older kernels always assume the address type 
of linkey is BREDR, and ltk/irk/csrk are LE public. Therefore, we can 
use this as a basis for judgment, allowing bluetoothd to handle various 
issues, such as reloading keys and handling unpaired devices. Below is a 
pseudo-code that is not running in practice but just a thought. Please 
help review it.
-------------------------------------------
diff --git a/src/adapter.c b/src/adapter.c
index 85ddfc165..babe7c9b2 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4958,6 +4958,7 @@ static void load_devices(struct btd_adapter *adapter)
          struct irk_info *irk_info;
          struct conn_param *param;
          uint8_t bdaddr_type;
+        bool derived_key;

          if (entry->d_type == DT_UNKNOWN)
              entry->d_type = util_get_dt(dirname, entry->d_name);
@@ -4976,10 +4977,21 @@ static void load_devices(struct btd_adapter 
*adapter)
              g_clear_error(&gerr);
          }

+        derived_key = g_key_file_get_boolean(key_file, "General",
+                                "DerivedKey", NULL);
+
+        /* For link key */
          bdaddr_type = get_addr_type(key_file);
+        if (!derived_key)
+            bdaddr_type = BDADDR_BREDR;

          key_info = get_key_info(key_file, entry->d_name, bdaddr_type);

+        /* For ltk/irk/csrk */
+        bdaddr_type = get_addr_type(key_file);
+        if (!derived_key)
+            bdaddr_type = BDADDR_LE_PUBLIC;
+
          ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);

          peripheral_ltk_info = get_peripheral_ltk_info(key_file,
@@ -8673,6 +8685,12 @@ static void new_link_key_callback(uint16_t index, 
uint16_t length,
          return;
      }

+    /*
+     * For older kernels, the address type is always BREDR.
+     */
+    if (addr->type == BDADDR_LE_PUBLIC)
+        device->derived_key = true;
+
      if (ev->store_hint) {
          const struct mgmt_link_key_info *key = &ev->key;

@@ -8791,6 +8809,12 @@ static void new_long_term_key_callback(uint16_t 
index, uint16_t length,
          return;
      }

+    /*
+     * For older kernels, the address type is always BDADDR_LE_PUBLIC.
+     */
+    if (addr->type == BDADDR_BREDR)
+        device->derived_key = true;
+
      /*
       * Some older kernel versions set store_hint for long term keys
       * from resolvable and unresolvable random addresses, but there
@@ -8855,6 +8879,9 @@ static void new_csrk_callback(uint16_t index, 
uint16_t length,
          return;
      }

+    if (addr->type == BDADDR_BREDR)
+        device->derived_key = true;
+
      device_set_csrk(device, key->val, 0, key->type, ev->store_hint);
  }

@@ -8941,6 +8968,9 @@ static void new_irk_callback(uint16_t index, 
uint16_t length,
          return;
      }

+    if (addr->type == BDADDR_BREDR)
+        device->derived_key = true;
+
      device_update_addr(device, &addr->bdaddr, addr->type);

      if (duplicate)
diff --git a/src/device.c b/src/device.c
index a1dc0750c..062b9c49d 100644
--- a/src/device.c
+++ b/src/device.c
@@ -272,6 +272,7 @@ struct btd_device {
      struct csrk_info *remote_csrk;
      struct ltk_info *ltk;
      struct queue    *sirks;
+    bool        derived_key;            /* key is derived. */

      sdp_list_t    *tmp_records;

@@ -482,6 +483,9 @@ static gboolean store_device_info_cb(gpointer user_data)
      g_key_file_set_boolean(key_file, "General", "Blocked",
                              device->blocked);

+    g_key_file_set_boolean(key_file, "General", "DerivedKey",
+                            device->derived_key);
+
      if (device->wake_override != WAKE_FLAG_DEFAULT) {
          g_key_file_set_boolean(key_file, "General", "WakeAllowed",
                         device->wake_override ==
@@ -1829,7 +1833,11 @@ static void bonding_request_cancel(struct 
bonding_req *bonding)
      struct btd_device *device = bonding->device;
      struct btd_adapter *adapter = device->adapter;

-    adapter_cancel_bonding(adapter, &device->bdaddr, device->bdaddr_type);
+    if (device->derived_key) {
+        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_BREDR);
+        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_LE_PUBLIC);
+    } else
+        adapter_cancel_bonding(adapter, &device->bdaddr, 
device->bdaddr_type);
  }

  static void dev_disconn_service(gpointer a, gpointer b)
@@ -3196,12 +3204,19 @@ static DBusMessage 
*cancel_pairing(DBusConnection *conn, DBusMessage *msg,
  {
      struct btd_device *device = data;
      struct bonding_req *req = device->bonding;
+    uint8_t bdaddr_type;

      DBG("");

      if (!req) {
-        btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
-                        device->bdaddr_type);
+        if (device->derived_key) {
+            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
+                            BDADDR_BREDR);
+            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
+                            BDADDR_LE_PUBLIC);
+        } else
+            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
+                            device->bdaddr_type);
          return btd_error_does_not_exist(msg);
      }

@@ -3833,6 +3848,9 @@ next:
          gerr = NULL;
      }

+    device->derived_key = g_key_file_get_boolean(key_file, "General",
+                            "DerivedKey", NULL);
+
      if (store_needed)
          store_device_info(device);
  }

On 2024/8/27 5:12, Luiz Augusto von Dentz wrote:
> Hi Xiao,
> 
> On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
>>
>> On 12/11/23 17:27, Xiao Yao wrote:
>>
>> From: Xiao Yao <[e-mail]>
>>
>> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
>> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
>> converted between each other, so the addr type can be either
>> BREDR or LE, so fix it.
>>
>> Signed-off-by: Xiao Yao <[e-mail]>
>> ---
>>   src/adapter.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 86fff72bc..ee70b00d2 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
>>
>>   struct link_key_info {
>>    bdaddr_t bdaddr;
>> + uint8_t bdaddr_type;
>>    unsigned char key[16];
>>    uint8_t type;
>>    uint8_t pin_len;
>> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
>>    return false;
>>   }
>>
>> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
>> + uint8_t bdaddr_type)
>> +
>>   {
>>    struct link_key_info *info = NULL;
>>    char *str;
>> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>>    info = g_new0(struct link_key_info, 1);
>>
>>    str2ba(peer, &info->bdaddr);
>> + info->bdaddr_type = bdaddr_type;
>>
>>    if (!strncmp(str, "0x", 2))
>>    str2buf(&str[2], info->key, sizeof(info->key));
>> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
>>    struct link_key_info *info = l->data;
>>
>>    bacpy(&key->addr.bdaddr, &info->bdaddr);
>> - key->addr.type = BDADDR_BREDR;
>> + key->addr.type = info->bdaddr_type;
>>    key->type = info->type;
>>    memcpy(key->val, info->key, 16);
>>    key->pin_len = info->pin_len;
>> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
>>    btd_error(adapter->dev_id, "Load connection parameters failed");
>>   }
>>
>> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
>> +static uint8_t get_addr_type(GKeyFile *keyfile)
>>   {
>>    uint8_t addr_type;
>>    char *type;
>>
>> + /* The AddressType is written to file only When dev->le is
>> + * set to true, as referenced in the update_technologies().
>> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
>> + */
>>    type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
>>    if (!type)
>> - return BDADDR_LE_PUBLIC;
>> + return BDADDR_BREDR;
>>
>>    if (g_str_equal(type, "public"))
>>    addr_type = BDADDR_LE_PUBLIC;
>> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
>>    g_clear_error(&gerr);
>>    }
>>
>> - key_info = get_key_info(key_file, entry->d_name);
>> + bdaddr_type = get_addr_type(key_file);
>>
>> - bdaddr_type = get_le_addr_type(key_file);
>> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>>
>>    ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>>
>>
>> Hello,
>>
>> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
>>
>> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
>>
>> `systemctl status bluetooth` reports the following error:
>>
>> [...]
>> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
>> [...]
>>
>> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
>> I can confirm that reverting it solves the issue.
>>
>> I reported this bug including details in the GitHub repo [2].
>>
>> I remain available if any additional information are needed.
>>
>> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
>> [2] https://github.com/bluez/bluez/issues/686
>>
>> --
>> Regards,
>> Robin Candau / Antiz
> 
> Perhaps related to:
> https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?

Luiz Augusto von Dentz Aug. 27, 2024, 12:52 p.m. UTC | #6

Hi,

On Tue, Aug 27, 2024 at 3:41 AM Yao Xiao <[e-mail]> wrote:
>
> Hi luiz,
> The kernel should always report the correct address type for all kinds
> of keys. Older kernels ignore the derivation of keys, which prevents the
> user layer from knowing the correct address type. This can cause a lot
> of confusion for users. For example, when a user connects to a
> traditional Bluetooth audio device, due to the default activation of key
> derivation, bluetoothd reports both BREDR and BLE pairing and binding,
> and the address is updated to LE public. When the user actively calls
> the Connect method of org.bluez.Device1 to reconnect, sometimes the
> address type is set to LE public, leading to a failed connection, and
> vice versa.

The correct address type can only be the ones listed on mgmt-api
otherwise we are breaking APIs, also except if there are some new case
I don't know and Load Keys and Load LTK are bearer specific.

> We should enable bluetoothd to know the reason for the generation of
> keys as much as possible. Older kernels always assume the address type
> of linkey is BREDR, and ltk/irk/csrk are LE public. Therefore, we can
> use this as a basis for judgment, allowing bluetoothd to handle various
> issues, such as reloading keys and handling unpaired devices. Below is a
> pseudo-code that is not running in practice but just a thought. Please
> help review it.

Yeah, and what exactly was wrong with that? What you mention about
having keys for both BR/EDR and LE public is not an issue, that
basically the very definition of a dual-mode device and bluetoothd
shall be able to tell when to connect to BR/EDR and LE public, if it
doesn't we shall fix that and not start breaking APIs.

> -------------------------------------------
> diff --git a/src/adapter.c b/src/adapter.c
> index 85ddfc165..babe7c9b2 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -4958,6 +4958,7 @@ static void load_devices(struct btd_adapter *adapter)
>           struct irk_info *irk_info;
>           struct conn_param *param;
>           uint8_t bdaddr_type;
> +        bool derived_key;
>
>           if (entry->d_type == DT_UNKNOWN)
>               entry->d_type = util_get_dt(dirname, entry->d_name);
> @@ -4976,10 +4977,21 @@ static void load_devices(struct btd_adapter
> *adapter)
>               g_clear_error(&gerr);
>           }
>
> +        derived_key = g_key_file_get_boolean(key_file, "General",
> +                                "DerivedKey", NULL);
> +
> +        /* For link key */
>           bdaddr_type = get_addr_type(key_file);
> +        if (!derived_key)
> +            bdaddr_type = BDADDR_BREDR;
>
>           key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>
> +        /* For ltk/irk/csrk */
> +        bdaddr_type = get_addr_type(key_file);
> +        if (!derived_key)
> +            bdaddr_type = BDADDR_LE_PUBLIC;
> +
>           ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>
>           peripheral_ltk_info = get_peripheral_ltk_info(key_file,
> @@ -8673,6 +8685,12 @@ static void new_link_key_callback(uint16_t index,
> uint16_t length,
>           return;
>       }
>
> +    /*
> +     * For older kernels, the address type is always BREDR.
> +     */
> +    if (addr->type == BDADDR_LE_PUBLIC)
> +        device->derived_key = true;
> +
>       if (ev->store_hint) {
>           const struct mgmt_link_key_info *key = &ev->key;
>
> @@ -8791,6 +8809,12 @@ static void new_long_term_key_callback(uint16_t
> index, uint16_t length,
>           return;
>       }
>
> +    /*
> +     * For older kernels, the address type is always BDADDR_LE_PUBLIC.
> +     */
> +    if (addr->type == BDADDR_BREDR)
> +        device->derived_key = true;
> +
>       /*
>        * Some older kernel versions set store_hint for long term keys
>        * from resolvable and unresolvable random addresses, but there
> @@ -8855,6 +8879,9 @@ static void new_csrk_callback(uint16_t index,
> uint16_t length,
>           return;
>       }
>
> +    if (addr->type == BDADDR_BREDR)
> +        device->derived_key = true;
> +
>       device_set_csrk(device, key->val, 0, key->type, ev->store_hint);
>   }
>
> @@ -8941,6 +8968,9 @@ static void new_irk_callback(uint16_t index,
> uint16_t length,
>           return;
>       }
>
> +    if (addr->type == BDADDR_BREDR)
> +        device->derived_key = true;
> +
>       device_update_addr(device, &addr->bdaddr, addr->type);
>
>       if (duplicate)
> diff --git a/src/device.c b/src/device.c
> index a1dc0750c..062b9c49d 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -272,6 +272,7 @@ struct btd_device {
>       struct csrk_info *remote_csrk;
>       struct ltk_info *ltk;
>       struct queue    *sirks;
> +    bool        derived_key;            /* key is derived. */
>
>       sdp_list_t    *tmp_records;
>
> @@ -482,6 +483,9 @@ static gboolean store_device_info_cb(gpointer user_data)
>       g_key_file_set_boolean(key_file, "General", "Blocked",
>                               device->blocked);
>
> +    g_key_file_set_boolean(key_file, "General", "DerivedKey",
> +                            device->derived_key);
> +
>       if (device->wake_override != WAKE_FLAG_DEFAULT) {
>           g_key_file_set_boolean(key_file, "General", "WakeAllowed",
>                          device->wake_override ==
> @@ -1829,7 +1833,11 @@ static void bonding_request_cancel(struct
> bonding_req *bonding)
>       struct btd_device *device = bonding->device;
>       struct btd_adapter *adapter = device->adapter;
>
> -    adapter_cancel_bonding(adapter, &device->bdaddr, device->bdaddr_type);
> +    if (device->derived_key) {
> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_BREDR);
> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_LE_PUBLIC);
> +    } else
> +        adapter_cancel_bonding(adapter, &device->bdaddr,
> device->bdaddr_type);
>   }
>
>   static void dev_disconn_service(gpointer a, gpointer b)
> @@ -3196,12 +3204,19 @@ static DBusMessage
> *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
>   {
>       struct btd_device *device = data;
>       struct bonding_req *req = device->bonding;
> +    uint8_t bdaddr_type;
>
>       DBG("");
>
>       if (!req) {
> -        btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> -                        device->bdaddr_type);
> +        if (device->derived_key) {
> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> +                            BDADDR_BREDR);
> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> +                            BDADDR_LE_PUBLIC);
> +        } else
> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> +                            device->bdaddr_type);
>           return btd_error_does_not_exist(msg);
>       }
>
> @@ -3833,6 +3848,9 @@ next:
>           gerr = NULL;
>       }
>
> +    device->derived_key = g_key_file_get_boolean(key_file, "General",
> +                            "DerivedKey", NULL);
> +
>       if (store_needed)
>           store_device_info(device);
>   }
>
> On 2024/8/27 5:12, Luiz Augusto von Dentz wrote:
> > Hi Xiao,
> >
> > On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
> >>
> >> On 12/11/23 17:27, Xiao Yao wrote:
> >>
> >> From: Xiao Yao <[e-mail]>
> >>
> >> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
> >> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
> >> converted between each other, so the addr type can be either
> >> BREDR or LE, so fix it.
> >>
> >> Signed-off-by: Xiao Yao <[e-mail]>
> >> ---
> >>   src/adapter.c | 20 ++++++++++++++------
> >>   1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/adapter.c b/src/adapter.c
> >> index 86fff72bc..ee70b00d2 100644
> >> --- a/src/adapter.c
> >> +++ b/src/adapter.c
> >> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
> >>
> >>   struct link_key_info {
> >>    bdaddr_t bdaddr;
> >> + uint8_t bdaddr_type;
> >>    unsigned char key[16];
> >>    uint8_t type;
> >>    uint8_t pin_len;
> >> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> >>    return false;
> >>   }
> >>
> >> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
> >> + uint8_t bdaddr_type)
> >> +
> >>   {
> >>    struct link_key_info *info = NULL;
> >>    char *str;
> >> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >>    info = g_new0(struct link_key_info, 1);
> >>
> >>    str2ba(peer, &info->bdaddr);
> >> + info->bdaddr_type = bdaddr_type;
> >>
> >>    if (!strncmp(str, "0x", 2))
> >>    str2buf(&str[2], info->key, sizeof(info->key));
> >> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
> >>    struct link_key_info *info = l->data;
> >>
> >>    bacpy(&key->addr.bdaddr, &info->bdaddr);
> >> - key->addr.type = BDADDR_BREDR;
> >> + key->addr.type = info->bdaddr_type;
> >>    key->type = info->type;
> >>    memcpy(key->val, info->key, 16);
> >>    key->pin_len = info->pin_len;
> >> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
> >>    btd_error(adapter->dev_id, "Load connection parameters failed");
> >>   }
> >>
> >> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
> >> +static uint8_t get_addr_type(GKeyFile *keyfile)
> >>   {
> >>    uint8_t addr_type;
> >>    char *type;
> >>
> >> + /* The AddressType is written to file only When dev->le is
> >> + * set to true, as referenced in the update_technologies().
> >> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
> >> + */
> >>    type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
> >>    if (!type)
> >> - return BDADDR_LE_PUBLIC;
> >> + return BDADDR_BREDR;
> >>
> >>    if (g_str_equal(type, "public"))
> >>    addr_type = BDADDR_LE_PUBLIC;
> >> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
> >>    g_clear_error(&gerr);
> >>    }
> >>
> >> - key_info = get_key_info(key_file, entry->d_name);
> >> + bdaddr_type = get_addr_type(key_file);
> >>
> >> - bdaddr_type = get_le_addr_type(key_file);
> >> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
> >>
> >>    ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> >>
> >>
> >> Hello,
> >>
> >> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
> >>
> >> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
> >>
> >> `systemctl status bluetooth` reports the following error:
> >>
> >> [...]
> >> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
> >> [...]
> >>
> >> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
> >> I can confirm that reverting it solves the issue.
> >>
> >> I reported this bug including details in the GitHub repo [2].
> >>
> >> I remain available if any additional information are needed.
> >>
> >> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
> >> [2] https://github.com/bluez/bluez/issues/686
> >>
> >> --
> >> Regards,
> >> Robin Candau / Antiz
> >
> > Perhaps related to:
> > https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?
>

Yao Xiao Aug. 27, 2024, 2:33 p.m. UTC | #7

Hi Luiz,

On 2024/8/27 20:52, Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Tue, Aug 27, 2024 at 3:41 AM Yao Xiao <[e-mail]> wrote:
>>
>> Hi luiz,
>> The kernel should always report the correct address type for all kinds
>> of keys. Older kernels ignore the derivation of keys, which prevents the
>> user layer from knowing the correct address type. This can cause a lot
>> of confusion for users. For example, when a user connects to a
>> traditional Bluetooth audio device, due to the default activation of key
>> derivation, bluetoothd reports both BREDR and BLE pairing and binding,
>> and the address is updated to LE public. When the user actively calls
>> the Connect method of org.bluez.Device1 to reconnect, sometimes the
>> address type is set to LE public, leading to a failed connection, and
>> vice versa.
> 
> The correct address type can only be the ones listed on mgmt-api
> otherwise we are breaking APIs, also except if there are some new case
> I don't know and Load Keys and Load LTK are bearer specific.
> 
>> We should enable bluetoothd to know the reason for the generation of
>> keys as much as possible. Older kernels always assume the address type
>> of linkey is BREDR, and ltk/irk/csrk are LE public. Therefore, we can
>> use this as a basis for judgment, allowing bluetoothd to handle various
>> issues, such as reloading keys and handling unpaired devices. Below is a
>> pseudo-code that is not running in practice but just a thought. Please
>> help review it.
> 
> Yeah, and what exactly was wrong with that? What you mention about
> having keys for both BR/EDR and LE public is not an issue, that
> basically the very definition of a dual-mode device and bluetoothd
> shall be able to tell when to connect to BR/EDR and LE public, if it
> doesn't we shall fix that and not start breaking APIs.
Returning to my original issue: When BlueZ acts as a sink device and a 
phone connects to it, pairs successfully, and derives related keys, 
BlueZ reports to the user layer that bonded (both bredr_state.bonded and 
dev->le_state.bonded) are true, and addr_type is LE_PUBLIC. If the 
device restarts and the org.bluez.Device1:Connect method is called to 
reconnect to the phone, according to the rules in select_conn_bearer, 
addr_type will be set to LE_PUBLIC, leading to a failed reconnection 
when device_connect_le is called.

In this situation, how should bluetoothd choose the correct address type 
when it doesn't know the physical bearer layer? Should there be a 
configuration option to turn key derivation on or off? Additionally, 
there's another use case: When a phone connects to a BlueZ device, it 
continuously transmits custom data via BLE and music via traditional 
Bluetooth (BREDR). If the user only wants to unpair the BREDR, it may 
cause the BLE connection to be disconnected as well, leading to data 
transmission interruption. Should we also consider turning off key 
derivation in this scenario?

> 
>> -------------------------------------------
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 85ddfc165..babe7c9b2 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -4958,6 +4958,7 @@ static void load_devices(struct btd_adapter *adapter)
>>            struct irk_info *irk_info;
>>            struct conn_param *param;
>>            uint8_t bdaddr_type;
>> +        bool derived_key;
>>
>>            if (entry->d_type == DT_UNKNOWN)
>>                entry->d_type = util_get_dt(dirname, entry->d_name);
>> @@ -4976,10 +4977,21 @@ static void load_devices(struct btd_adapter
>> *adapter)
>>                g_clear_error(&gerr);
>>            }
>>
>> +        derived_key = g_key_file_get_boolean(key_file, "General",
>> +                                "DerivedKey", NULL);
>> +
>> +        /* For link key */
>>            bdaddr_type = get_addr_type(key_file);
>> +        if (!derived_key)
>> +            bdaddr_type = BDADDR_BREDR;
>>
>>            key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>>
>> +        /* For ltk/irk/csrk */
>> +        bdaddr_type = get_addr_type(key_file);
>> +        if (!derived_key)
>> +            bdaddr_type = BDADDR_LE_PUBLIC;
>> +
>>            ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>>
>>            peripheral_ltk_info = get_peripheral_ltk_info(key_file,
>> @@ -8673,6 +8685,12 @@ static void new_link_key_callback(uint16_t index,
>> uint16_t length,
>>            return;
>>        }
>>
>> +    /*
>> +     * For older kernels, the address type is always BREDR.
>> +     */
>> +    if (addr->type == BDADDR_LE_PUBLIC)
>> +        device->derived_key = true;
>> +
>>        if (ev->store_hint) {
>>            const struct mgmt_link_key_info *key = &ev->key;
>>
>> @@ -8791,6 +8809,12 @@ static void new_long_term_key_callback(uint16_t
>> index, uint16_t length,
>>            return;
>>        }
>>
>> +    /*
>> +     * For older kernels, the address type is always BDADDR_LE_PUBLIC.
>> +     */
>> +    if (addr->type == BDADDR_BREDR)
>> +        device->derived_key = true;
>> +
>>        /*
>>         * Some older kernel versions set store_hint for long term keys
>>         * from resolvable and unresolvable random addresses, but there
>> @@ -8855,6 +8879,9 @@ static void new_csrk_callback(uint16_t index,
>> uint16_t length,
>>            return;
>>        }
>>
>> +    if (addr->type == BDADDR_BREDR)
>> +        device->derived_key = true;
>> +
>>        device_set_csrk(device, key->val, 0, key->type, ev->store_hint);
>>    }
>>
>> @@ -8941,6 +8968,9 @@ static void new_irk_callback(uint16_t index,
>> uint16_t length,
>>            return;
>>        }
>>
>> +    if (addr->type == BDADDR_BREDR)
>> +        device->derived_key = true;
>> +
>>        device_update_addr(device, &addr->bdaddr, addr->type);
>>
>>        if (duplicate)
>> diff --git a/src/device.c b/src/device.c
>> index a1dc0750c..062b9c49d 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -272,6 +272,7 @@ struct btd_device {
>>        struct csrk_info *remote_csrk;
>>        struct ltk_info *ltk;
>>        struct queue    *sirks;
>> +    bool        derived_key;            /* key is derived. */
>>
>>        sdp_list_t    *tmp_records;
>>
>> @@ -482,6 +483,9 @@ static gboolean store_device_info_cb(gpointer user_data)
>>        g_key_file_set_boolean(key_file, "General", "Blocked",
>>                                device->blocked);
>>
>> +    g_key_file_set_boolean(key_file, "General", "DerivedKey",
>> +                            device->derived_key);
>> +
>>        if (device->wake_override != WAKE_FLAG_DEFAULT) {
>>            g_key_file_set_boolean(key_file, "General", "WakeAllowed",
>>                           device->wake_override ==
>> @@ -1829,7 +1833,11 @@ static void bonding_request_cancel(struct
>> bonding_req *bonding)
>>        struct btd_device *device = bonding->device;
>>        struct btd_adapter *adapter = device->adapter;
>>
>> -    adapter_cancel_bonding(adapter, &device->bdaddr, device->bdaddr_type);
>> +    if (device->derived_key) {
>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_BREDR);
>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_LE_PUBLIC);
>> +    } else
>> +        adapter_cancel_bonding(adapter, &device->bdaddr,
>> device->bdaddr_type);
>>    }
>>
>>    static void dev_disconn_service(gpointer a, gpointer b)
>> @@ -3196,12 +3204,19 @@ static DBusMessage
>> *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
>>    {
>>        struct btd_device *device = data;
>>        struct bonding_req *req = device->bonding;
>> +    uint8_t bdaddr_type;
>>
>>        DBG("");
>>
>>        if (!req) {
>> -        btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>> -                        device->bdaddr_type);
>> +        if (device->derived_key) {
>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>> +                            BDADDR_BREDR);
>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>> +                            BDADDR_LE_PUBLIC);
>> +        } else
>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>> +                            device->bdaddr_type);
>>            return btd_error_does_not_exist(msg);
>>        }
>>
>> @@ -3833,6 +3848,9 @@ next:
>>            gerr = NULL;
>>        }
>>
>> +    device->derived_key = g_key_file_get_boolean(key_file, "General",
>> +                            "DerivedKey", NULL);
>> +
>>        if (store_needed)
>>            store_device_info(device);
>>    }
>>
>> On 2024/8/27 5:12, Luiz Augusto von Dentz wrote:
>>> Hi Xiao,
>>>
>>> On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
>>>>
>>>> On 12/11/23 17:27, Xiao Yao wrote:
>>>>
>>>> From: Xiao Yao <[e-mail]>
>>>>
>>>> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
>>>> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
>>>> converted between each other, so the addr type can be either
>>>> BREDR or LE, so fix it.
>>>>
>>>> Signed-off-by: Xiao Yao <[e-mail]>
>>>> ---
>>>>    src/adapter.c | 20 ++++++++++++++------
>>>>    1 file changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>> index 86fff72bc..ee70b00d2 100644
>>>> --- a/src/adapter.c
>>>> +++ b/src/adapter.c
>>>> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
>>>>
>>>>    struct link_key_info {
>>>>     bdaddr_t bdaddr;
>>>> + uint8_t bdaddr_type;
>>>>     unsigned char key[16];
>>>>     uint8_t type;
>>>>     uint8_t pin_len;
>>>> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
>>>>     return false;
>>>>    }
>>>>
>>>> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>>>> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
>>>> + uint8_t bdaddr_type)
>>>> +
>>>>    {
>>>>     struct link_key_info *info = NULL;
>>>>     char *str;
>>>> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>>>>     info = g_new0(struct link_key_info, 1);
>>>>
>>>>     str2ba(peer, &info->bdaddr);
>>>> + info->bdaddr_type = bdaddr_type;
>>>>
>>>>     if (!strncmp(str, "0x", 2))
>>>>     str2buf(&str[2], info->key, sizeof(info->key));
>>>> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
>>>>     struct link_key_info *info = l->data;
>>>>
>>>>     bacpy(&key->addr.bdaddr, &info->bdaddr);
>>>> - key->addr.type = BDADDR_BREDR;
>>>> + key->addr.type = info->bdaddr_type;
>>>>     key->type = info->type;
>>>>     memcpy(key->val, info->key, 16);
>>>>     key->pin_len = info->pin_len;
>>>> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
>>>>     btd_error(adapter->dev_id, "Load connection parameters failed");
>>>>    }
>>>>
>>>> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
>>>> +static uint8_t get_addr_type(GKeyFile *keyfile)
>>>>    {
>>>>     uint8_t addr_type;
>>>>     char *type;
>>>>
>>>> + /* The AddressType is written to file only When dev->le is
>>>> + * set to true, as referenced in the update_technologies().
>>>> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
>>>> + */
>>>>     type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
>>>>     if (!type)
>>>> - return BDADDR_LE_PUBLIC;
>>>> + return BDADDR_BREDR;
>>>>
>>>>     if (g_str_equal(type, "public"))
>>>>     addr_type = BDADDR_LE_PUBLIC;
>>>> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
>>>>     g_clear_error(&gerr);
>>>>     }
>>>>
>>>> - key_info = get_key_info(key_file, entry->d_name);
>>>> + bdaddr_type = get_addr_type(key_file);
>>>>
>>>> - bdaddr_type = get_le_addr_type(key_file);
>>>> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>>>>
>>>>     ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>>>>
>>>>
>>>> Hello,
>>>>
>>>> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
>>>>
>>>> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
>>>>
>>>> `systemctl status bluetooth` reports the following error:
>>>>
>>>> [...]
>>>> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
>>>> [...]
>>>>
>>>> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
>>>> I can confirm that reverting it solves the issue.
>>>>
>>>> I reported this bug including details in the GitHub repo [2].
>>>>
>>>> I remain available if any additional information are needed.
>>>>
>>>> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
>>>> [2] https://github.com/bluez/bluez/issues/686
>>>>
>>>> --
>>>> Regards,
>>>> Robin Candau / Antiz
>>>
>>> Perhaps related to:
>>> https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?
>>
> 
>

Luiz Augusto von Dentz Aug. 27, 2024, 3:47 p.m. UTC | #8

Hi,

On Tue, Aug 27, 2024 at 10:33 AM Yao Xiao <[e-mail]> wrote:
>
> Hi Luiz,
>
> On 2024/8/27 20:52, Luiz Augusto von Dentz wrote:
> > Hi,
> >
> > On Tue, Aug 27, 2024 at 3:41 AM Yao Xiao <[e-mail]> wrote:
> >>
> >> Hi luiz,
> >> The kernel should always report the correct address type for all kinds
> >> of keys. Older kernels ignore the derivation of keys, which prevents the
> >> user layer from knowing the correct address type. This can cause a lot
> >> of confusion for users. For example, when a user connects to a
> >> traditional Bluetooth audio device, due to the default activation of key
> >> derivation, bluetoothd reports both BREDR and BLE pairing and binding,
> >> and the address is updated to LE public. When the user actively calls
> >> the Connect method of org.bluez.Device1 to reconnect, sometimes the
> >> address type is set to LE public, leading to a failed connection, and
> >> vice versa.
> >
> > The correct address type can only be the ones listed on mgmt-api
> > otherwise we are breaking APIs, also except if there are some new case
> > I don't know and Load Keys and Load LTK are bearer specific.
> >
> >> We should enable bluetoothd to know the reason for the generation of
> >> keys as much as possible. Older kernels always assume the address type
> >> of linkey is BREDR, and ltk/irk/csrk are LE public. Therefore, we can
> >> use this as a basis for judgment, allowing bluetoothd to handle various
> >> issues, such as reloading keys and handling unpaired devices. Below is a
> >> pseudo-code that is not running in practice but just a thought. Please
> >> help review it.
> >
> > Yeah, and what exactly was wrong with that? What you mention about
> > having keys for both BR/EDR and LE public is not an issue, that
> > basically the very definition of a dual-mode device and bluetoothd
> > shall be able to tell when to connect to BR/EDR and LE public, if it
> > doesn't we shall fix that and not start breaking APIs.
> Returning to my original issue: When BlueZ acts as a sink device and a
> phone connects to it, pairs successfully, and derives related keys,
> BlueZ reports to the user layer that bonded (both bredr_state.bonded and
> dev->le_state.bonded) are true, and addr_type is LE_PUBLIC. If the
> device restarts and the org.bluez.Device1:Connect method is called to
> reconnect to the phone, according to the rules in select_conn_bearer,
> addr_type will be set to LE_PUBLIC, leading to a failed reconnection
> when device_connect_le is called.

If I recall this correctly we do reconnect based on the last seen
bearer, also the addr_type is only really relevant for LE bearer,
BR/EDR shall be detected via bearer state alone. Anyway there was a
discussion regarding having bearer specific API's as alternative to
org.bluez.Device1.Connect to pickup a bearer, but in some cases, like
when acting as a peripheral, perhaps we need a different policy, in
the specific case of being a A2DP sink though, we could detect that LE
keys are for a peripheral and in that case advertisement shall be used
for connecting the LE bearer thus the BR/EDR shall be chosen which
shall fix this behavior.

> In this situation, how should bluetoothd choose the correct address type
> when it doesn't know the physical bearer layer? Should there be a
> configuration option to turn key derivation on or off? Additionally,
> there's another use case: When a phone connects to a BlueZ device, it
> continuously transmits custom data via BLE and music via traditional
> Bluetooth (BREDR). If the user only wants to unpair the BREDR, it may
> cause the BLE connection to be disconnected as well, leading to data
> transmission interruption. Should we also consider turning off key
> derivation in this scenario?

Device API is not bearer specific though, so one cannot really unpair
BR/EDR alone, in fact I would be surprised if any stack implements
this that way because depending on what bearer is connected it may
trigger a missing link key which could possibly lead to repairing
procedure which will most likely having to replace the other bearer
keys as well, if the idea is you only really need BR/EDR then you
might as well disable LE completely.

>
> >
> >> -------------------------------------------
> >> diff --git a/src/adapter.c b/src/adapter.c
> >> index 85ddfc165..babe7c9b2 100644
> >> --- a/src/adapter.c
> >> +++ b/src/adapter.c
> >> @@ -4958,6 +4958,7 @@ static void load_devices(struct btd_adapter *adapter)
> >>            struct irk_info *irk_info;
> >>            struct conn_param *param;
> >>            uint8_t bdaddr_type;
> >> +        bool derived_key;
> >>
> >>            if (entry->d_type == DT_UNKNOWN)
> >>                entry->d_type = util_get_dt(dirname, entry->d_name);
> >> @@ -4976,10 +4977,21 @@ static void load_devices(struct btd_adapter
> >> *adapter)
> >>                g_clear_error(&gerr);
> >>            }
> >>
> >> +        derived_key = g_key_file_get_boolean(key_file, "General",
> >> +                                "DerivedKey", NULL);
> >> +
> >> +        /* For link key */
> >>            bdaddr_type = get_addr_type(key_file);
> >> +        if (!derived_key)
> >> +            bdaddr_type = BDADDR_BREDR;
> >>
> >>            key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
> >>
> >> +        /* For ltk/irk/csrk */
> >> +        bdaddr_type = get_addr_type(key_file);
> >> +        if (!derived_key)
> >> +            bdaddr_type = BDADDR_LE_PUBLIC;
> >> +
> >>            ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> >>
> >>            peripheral_ltk_info = get_peripheral_ltk_info(key_file,
> >> @@ -8673,6 +8685,12 @@ static void new_link_key_callback(uint16_t index,
> >> uint16_t length,
> >>            return;
> >>        }
> >>
> >> +    /*
> >> +     * For older kernels, the address type is always BREDR.
> >> +     */
> >> +    if (addr->type == BDADDR_LE_PUBLIC)
> >> +        device->derived_key = true;
> >> +
> >>        if (ev->store_hint) {
> >>            const struct mgmt_link_key_info *key = &ev->key;
> >>
> >> @@ -8791,6 +8809,12 @@ static void new_long_term_key_callback(uint16_t
> >> index, uint16_t length,
> >>            return;
> >>        }
> >>
> >> +    /*
> >> +     * For older kernels, the address type is always BDADDR_LE_PUBLIC.
> >> +     */
> >> +    if (addr->type == BDADDR_BREDR)
> >> +        device->derived_key = true;
> >> +
> >>        /*
> >>         * Some older kernel versions set store_hint for long term keys
> >>         * from resolvable and unresolvable random addresses, but there
> >> @@ -8855,6 +8879,9 @@ static void new_csrk_callback(uint16_t index,
> >> uint16_t length,
> >>            return;
> >>        }
> >>
> >> +    if (addr->type == BDADDR_BREDR)
> >> +        device->derived_key = true;
> >> +
> >>        device_set_csrk(device, key->val, 0, key->type, ev->store_hint);
> >>    }
> >>
> >> @@ -8941,6 +8968,9 @@ static void new_irk_callback(uint16_t index,
> >> uint16_t length,
> >>            return;
> >>        }
> >>
> >> +    if (addr->type == BDADDR_BREDR)
> >> +        device->derived_key = true;
> >> +
> >>        device_update_addr(device, &addr->bdaddr, addr->type);
> >>
> >>        if (duplicate)
> >> diff --git a/src/device.c b/src/device.c
> >> index a1dc0750c..062b9c49d 100644
> >> --- a/src/device.c
> >> +++ b/src/device.c
> >> @@ -272,6 +272,7 @@ struct btd_device {
> >>        struct csrk_info *remote_csrk;
> >>        struct ltk_info *ltk;
> >>        struct queue    *sirks;
> >> +    bool        derived_key;            /* key is derived. */
> >>
> >>        sdp_list_t    *tmp_records;
> >>
> >> @@ -482,6 +483,9 @@ static gboolean store_device_info_cb(gpointer user_data)
> >>        g_key_file_set_boolean(key_file, "General", "Blocked",
> >>                                device->blocked);
> >>
> >> +    g_key_file_set_boolean(key_file, "General", "DerivedKey",
> >> +                            device->derived_key);
> >> +
> >>        if (device->wake_override != WAKE_FLAG_DEFAULT) {
> >>            g_key_file_set_boolean(key_file, "General", "WakeAllowed",
> >>                           device->wake_override ==
> >> @@ -1829,7 +1833,11 @@ static void bonding_request_cancel(struct
> >> bonding_req *bonding)
> >>        struct btd_device *device = bonding->device;
> >>        struct btd_adapter *adapter = device->adapter;
> >>
> >> -    adapter_cancel_bonding(adapter, &device->bdaddr, device->bdaddr_type);
> >> +    if (device->derived_key) {
> >> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_BREDR);
> >> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_LE_PUBLIC);
> >> +    } else
> >> +        adapter_cancel_bonding(adapter, &device->bdaddr,
> >> device->bdaddr_type);
> >>    }
> >>
> >>    static void dev_disconn_service(gpointer a, gpointer b)
> >> @@ -3196,12 +3204,19 @@ static DBusMessage
> >> *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
> >>    {
> >>        struct btd_device *device = data;
> >>        struct bonding_req *req = device->bonding;
> >> +    uint8_t bdaddr_type;
> >>
> >>        DBG("");
> >>
> >>        if (!req) {
> >> -        btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >> -                        device->bdaddr_type);
> >> +        if (device->derived_key) {
> >> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >> +                            BDADDR_BREDR);
> >> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >> +                            BDADDR_LE_PUBLIC);
> >> +        } else
> >> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >> +                            device->bdaddr_type);
> >>            return btd_error_does_not_exist(msg);
> >>        }
> >>
> >> @@ -3833,6 +3848,9 @@ next:
> >>            gerr = NULL;
> >>        }
> >>
> >> +    device->derived_key = g_key_file_get_boolean(key_file, "General",
> >> +                            "DerivedKey", NULL);
> >> +
> >>        if (store_needed)
> >>            store_device_info(device);
> >>    }
> >>
> >> On 2024/8/27 5:12, Luiz Augusto von Dentz wrote:
> >>> Hi Xiao,
> >>>
> >>> On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
> >>>>
> >>>> On 12/11/23 17:27, Xiao Yao wrote:
> >>>>
> >>>> From: Xiao Yao <[e-mail]>
> >>>>
> >>>> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
> >>>> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
> >>>> converted between each other, so the addr type can be either
> >>>> BREDR or LE, so fix it.
> >>>>
> >>>> Signed-off-by: Xiao Yao <[e-mail]>
> >>>> ---
> >>>>    src/adapter.c | 20 ++++++++++++++------
> >>>>    1 file changed, 14 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/src/adapter.c b/src/adapter.c
> >>>> index 86fff72bc..ee70b00d2 100644
> >>>> --- a/src/adapter.c
> >>>> +++ b/src/adapter.c
> >>>> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
> >>>>
> >>>>    struct link_key_info {
> >>>>     bdaddr_t bdaddr;
> >>>> + uint8_t bdaddr_type;
> >>>>     unsigned char key[16];
> >>>>     uint8_t type;
> >>>>     uint8_t pin_len;
> >>>> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> >>>>     return false;
> >>>>    }
> >>>>
> >>>> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >>>> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
> >>>> + uint8_t bdaddr_type)
> >>>> +
> >>>>    {
> >>>>     struct link_key_info *info = NULL;
> >>>>     char *str;
> >>>> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >>>>     info = g_new0(struct link_key_info, 1);
> >>>>
> >>>>     str2ba(peer, &info->bdaddr);
> >>>> + info->bdaddr_type = bdaddr_type;
> >>>>
> >>>>     if (!strncmp(str, "0x", 2))
> >>>>     str2buf(&str[2], info->key, sizeof(info->key));
> >>>> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
> >>>>     struct link_key_info *info = l->data;
> >>>>
> >>>>     bacpy(&key->addr.bdaddr, &info->bdaddr);
> >>>> - key->addr.type = BDADDR_BREDR;
> >>>> + key->addr.type = info->bdaddr_type;
> >>>>     key->type = info->type;
> >>>>     memcpy(key->val, info->key, 16);
> >>>>     key->pin_len = info->pin_len;
> >>>> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
> >>>>     btd_error(adapter->dev_id, "Load connection parameters failed");
> >>>>    }
> >>>>
> >>>> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
> >>>> +static uint8_t get_addr_type(GKeyFile *keyfile)
> >>>>    {
> >>>>     uint8_t addr_type;
> >>>>     char *type;
> >>>>
> >>>> + /* The AddressType is written to file only When dev->le is
> >>>> + * set to true, as referenced in the update_technologies().
> >>>> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
> >>>> + */
> >>>>     type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
> >>>>     if (!type)
> >>>> - return BDADDR_LE_PUBLIC;
> >>>> + return BDADDR_BREDR;
> >>>>
> >>>>     if (g_str_equal(type, "public"))
> >>>>     addr_type = BDADDR_LE_PUBLIC;
> >>>> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
> >>>>     g_clear_error(&gerr);
> >>>>     }
> >>>>
> >>>> - key_info = get_key_info(key_file, entry->d_name);
> >>>> + bdaddr_type = get_addr_type(key_file);
> >>>>
> >>>> - bdaddr_type = get_le_addr_type(key_file);
> >>>> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
> >>>>
> >>>>     ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> >>>>
> >>>>
> >>>> Hello,
> >>>>
> >>>> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
> >>>>
> >>>> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
> >>>>
> >>>> `systemctl status bluetooth` reports the following error:
> >>>>
> >>>> [...]
> >>>> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
> >>>> [...]
> >>>>
> >>>> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
> >>>> I can confirm that reverting it solves the issue.
> >>>>
> >>>> I reported this bug including details in the GitHub repo [2].
> >>>>
> >>>> I remain available if any additional information are needed.
> >>>>
> >>>> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
> >>>> [2] https://github.com/bluez/bluez/issues/686
> >>>>
> >>>> --
> >>>> Regards,
> >>>> Robin Candau / Antiz
> >>>
> >>> Perhaps related to:
> >>> https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?
> >>
> >
> >
>

Yao Xiao Aug. 27, 2024, 6:01 p.m. UTC | #9

Hi,

On 2024/8/27 23:47, Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Tue, Aug 27, 2024 at 10:33 AM Yao Xiao <[e-mail]> wrote:
>>
>> Hi Luiz,
>>
>> On 2024/8/27 20:52, Luiz Augusto von Dentz wrote:
>>> Hi,
>>>
>>> On Tue, Aug 27, 2024 at 3:41 AM Yao Xiao <[e-mail]> wrote:
>>>>
>>>> Hi luiz,
>>>> The kernel should always report the correct address type for all kinds
>>>> of keys. Older kernels ignore the derivation of keys, which prevents the
>>>> user layer from knowing the correct address type. This can cause a lot
>>>> of confusion for users. For example, when a user connects to a
>>>> traditional Bluetooth audio device, due to the default activation of key
>>>> derivation, bluetoothd reports both BREDR and BLE pairing and binding,
>>>> and the address is updated to LE public. When the user actively calls
>>>> the Connect method of org.bluez.Device1 to reconnect, sometimes the
>>>> address type is set to LE public, leading to a failed connection, and
>>>> vice versa.
>>>
>>> The correct address type can only be the ones listed on mgmt-api
>>> otherwise we are breaking APIs, also except if there are some new case
>>> I don't know and Load Keys and Load LTK are bearer specific.
>>>
>>>> We should enable bluetoothd to know the reason for the generation of
>>>> keys as much as possible. Older kernels always assume the address type
>>>> of linkey is BREDR, and ltk/irk/csrk are LE public. Therefore, we can
>>>> use this as a basis for judgment, allowing bluetoothd to handle various
>>>> issues, such as reloading keys and handling unpaired devices. Below is a
>>>> pseudo-code that is not running in practice but just a thought. Please
>>>> help review it.
>>>
>>> Yeah, and what exactly was wrong with that? What you mention about
>>> having keys for both BR/EDR and LE public is not an issue, that
>>> basically the very definition of a dual-mode device and bluetoothd
>>> shall be able to tell when to connect to BR/EDR and LE public, if it
>>> doesn't we shall fix that and not start breaking APIs.
>> Returning to my original issue: When BlueZ acts as a sink device and a
>> phone connects to it, pairs successfully, and derives related keys,
>> BlueZ reports to the user layer that bonded (both bredr_state.bonded and
>> dev->le_state.bonded) are true, and addr_type is LE_PUBLIC. If the
>> device restarts and the org.bluez.Device1:Connect method is called to
>> reconnect to the phone, according to the rules in select_conn_bearer,
>> addr_type will be set to LE_PUBLIC, leading to a failed reconnection
>> when device_connect_le is called.
> 
> If I recall this correctly we do reconnect based on the last seen
> bearer, also the addr_type is only really relevant for LE bearer,
> BR/EDR shall be detected via bearer state alone. Anyway there was a
> discussion regarding having bearer specific API's as alternative to
> org.bluez.Device1.Connect to pickup a bearer, but in some cases, like
> when acting as a peripheral, perhaps we need a different policy, in
> the specific case of being a A2DP sink though, we could detect that LE
> keys are for a peripheral and in that case advertisement shall be used
> for connecting the LE bearer thus the BR/EDR shall be chosen which
> shall fix this behavior.
> 
>> In this situation, how should bluetoothd choose the correct address type
>> when it doesn't know the physical bearer layer? Should there be a
>> configuration option to turn key derivation on or off? Additionally,
>> there's another use case: When a phone connects to a BlueZ device, it
>> continuously transmits custom data via BLE and music via traditional
>> Bluetooth (BREDR). If the user only wants to unpair the BREDR, it may
>> cause the BLE connection to be disconnected as well, leading to data
>> transmission interruption. Should we also consider turning off key
>> derivation in this scenario?
> 
> Device API is not bearer specific though, so one cannot really unpair
> BR/EDR alone, in fact I would be surprised if any stack implements
> this that way because depending on what bearer is connected it may
> trigger a missing link key which could possibly lead to repairing
> procedure which will most likely having to replace the other bearer
> keys as well, if the idea is you only really need BR/EDR then you
> might as well disable LE completely.

Could we possibly implement a configuration that allows for the enabling 
or disabling of key derivation? Currently, a significant portion of 
devices only require either BR/EDR or BLE functionality independently 
and do not need key derivation, which can cause some confusion and 
issues for users. Additionally, as I understand, there are some 
proprietary RTOS Bluetooth protocol stacks that have disabled key 
derivation and support separate management of pairing keys for BR/EDR 
and BLE devices. For example, an IoT device that supports both BR/EDR 
and BLE could be detected as two separate devices by an iPhone, sharing 
the same address—one for BLE used for ANCS notifications (Apple 
Notification Center Service (ANCS) Specification) and the other for 
A2DP. The phone could connect to or remove either device without 
interference between the two.

> 
>>
>>>
>>>> -------------------------------------------
>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>> index 85ddfc165..babe7c9b2 100644
>>>> --- a/src/adapter.c
>>>> +++ b/src/adapter.c
>>>> @@ -4958,6 +4958,7 @@ static void load_devices(struct btd_adapter *adapter)
>>>>             struct irk_info *irk_info;
>>>>             struct conn_param *param;
>>>>             uint8_t bdaddr_type;
>>>> +        bool derived_key;
>>>>
>>>>             if (entry->d_type == DT_UNKNOWN)
>>>>                 entry->d_type = util_get_dt(dirname, entry->d_name);
>>>> @@ -4976,10 +4977,21 @@ static void load_devices(struct btd_adapter
>>>> *adapter)
>>>>                 g_clear_error(&gerr);
>>>>             }
>>>>
>>>> +        derived_key = g_key_file_get_boolean(key_file, "General",
>>>> +                                "DerivedKey", NULL);
>>>> +
>>>> +        /* For link key */
>>>>             bdaddr_type = get_addr_type(key_file);
>>>> +        if (!derived_key)
>>>> +            bdaddr_type = BDADDR_BREDR;
>>>>
>>>>             key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>>>>
>>>> +        /* For ltk/irk/csrk */
>>>> +        bdaddr_type = get_addr_type(key_file);
>>>> +        if (!derived_key)
>>>> +            bdaddr_type = BDADDR_LE_PUBLIC;
>>>> +
>>>>             ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>>>>
>>>>             peripheral_ltk_info = get_peripheral_ltk_info(key_file,
>>>> @@ -8673,6 +8685,12 @@ static void new_link_key_callback(uint16_t index,
>>>> uint16_t length,
>>>>             return;
>>>>         }
>>>>
>>>> +    /*
>>>> +     * For older kernels, the address type is always BREDR.
>>>> +     */
>>>> +    if (addr->type == BDADDR_LE_PUBLIC)
>>>> +        device->derived_key = true;
>>>> +
>>>>         if (ev->store_hint) {
>>>>             const struct mgmt_link_key_info *key = &ev->key;
>>>>
>>>> @@ -8791,6 +8809,12 @@ static void new_long_term_key_callback(uint16_t
>>>> index, uint16_t length,
>>>>             return;
>>>>         }
>>>>
>>>> +    /*
>>>> +     * For older kernels, the address type is always BDADDR_LE_PUBLIC.
>>>> +     */
>>>> +    if (addr->type == BDADDR_BREDR)
>>>> +        device->derived_key = true;
>>>> +
>>>>         /*
>>>>          * Some older kernel versions set store_hint for long term keys
>>>>          * from resolvable and unresolvable random addresses, but there
>>>> @@ -8855,6 +8879,9 @@ static void new_csrk_callback(uint16_t index,
>>>> uint16_t length,
>>>>             return;
>>>>         }
>>>>
>>>> +    if (addr->type == BDADDR_BREDR)
>>>> +        device->derived_key = true;
>>>> +
>>>>         device_set_csrk(device, key->val, 0, key->type, ev->store_hint);
>>>>     }
>>>>
>>>> @@ -8941,6 +8968,9 @@ static void new_irk_callback(uint16_t index,
>>>> uint16_t length,
>>>>             return;
>>>>         }
>>>>
>>>> +    if (addr->type == BDADDR_BREDR)
>>>> +        device->derived_key = true;
>>>> +
>>>>         device_update_addr(device, &addr->bdaddr, addr->type);
>>>>
>>>>         if (duplicate)
>>>> diff --git a/src/device.c b/src/device.c
>>>> index a1dc0750c..062b9c49d 100644
>>>> --- a/src/device.c
>>>> +++ b/src/device.c
>>>> @@ -272,6 +272,7 @@ struct btd_device {
>>>>         struct csrk_info *remote_csrk;
>>>>         struct ltk_info *ltk;
>>>>         struct queue    *sirks;
>>>> +    bool        derived_key;            /* key is derived. */
>>>>
>>>>         sdp_list_t    *tmp_records;
>>>>
>>>> @@ -482,6 +483,9 @@ static gboolean store_device_info_cb(gpointer user_data)
>>>>         g_key_file_set_boolean(key_file, "General", "Blocked",
>>>>                                 device->blocked);
>>>>
>>>> +    g_key_file_set_boolean(key_file, "General", "DerivedKey",
>>>> +                            device->derived_key);
>>>> +
>>>>         if (device->wake_override != WAKE_FLAG_DEFAULT) {
>>>>             g_key_file_set_boolean(key_file, "General", "WakeAllowed",
>>>>                            device->wake_override ==
>>>> @@ -1829,7 +1833,11 @@ static void bonding_request_cancel(struct
>>>> bonding_req *bonding)
>>>>         struct btd_device *device = bonding->device;
>>>>         struct btd_adapter *adapter = device->adapter;
>>>>
>>>> -    adapter_cancel_bonding(adapter, &device->bdaddr, device->bdaddr_type);
>>>> +    if (device->derived_key) {
>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_BREDR);
>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_LE_PUBLIC);
>>>> +    } else
>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr,
>>>> device->bdaddr_type);
>>>>     }
>>>>
>>>>     static void dev_disconn_service(gpointer a, gpointer b)
>>>> @@ -3196,12 +3204,19 @@ static DBusMessage
>>>> *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
>>>>     {
>>>>         struct btd_device *device = data;
>>>>         struct bonding_req *req = device->bonding;
>>>> +    uint8_t bdaddr_type;
>>>>
>>>>         DBG("");
>>>>
>>>>         if (!req) {
>>>> -        btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>> -                        device->bdaddr_type);
>>>> +        if (device->derived_key) {
>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>> +                            BDADDR_BREDR);
>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>> +                            BDADDR_LE_PUBLIC);
>>>> +        } else
>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>> +                            device->bdaddr_type);
>>>>             return btd_error_does_not_exist(msg);
>>>>         }
>>>>
>>>> @@ -3833,6 +3848,9 @@ next:
>>>>             gerr = NULL;
>>>>         }
>>>>
>>>> +    device->derived_key = g_key_file_get_boolean(key_file, "General",
>>>> +                            "DerivedKey", NULL);
>>>> +
>>>>         if (store_needed)
>>>>             store_device_info(device);
>>>>     }
>>>>
>>>> On 2024/8/27 5:12, Luiz Augusto von Dentz wrote:
>>>>> Hi Xiao,
>>>>>
>>>>> On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
>>>>>>
>>>>>> On 12/11/23 17:27, Xiao Yao wrote:
>>>>>>
>>>>>> From: Xiao Yao <[e-mail]>
>>>>>>
>>>>>> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
>>>>>> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
>>>>>> converted between each other, so the addr type can be either
>>>>>> BREDR or LE, so fix it.
>>>>>>
>>>>>> Signed-off-by: Xiao Yao <[e-mail]>
>>>>>> ---
>>>>>>     src/adapter.c | 20 ++++++++++++++------
>>>>>>     1 file changed, 14 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>>>> index 86fff72bc..ee70b00d2 100644
>>>>>> --- a/src/adapter.c
>>>>>> +++ b/src/adapter.c
>>>>>> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
>>>>>>
>>>>>>     struct link_key_info {
>>>>>>      bdaddr_t bdaddr;
>>>>>> + uint8_t bdaddr_type;
>>>>>>      unsigned char key[16];
>>>>>>      uint8_t type;
>>>>>>      uint8_t pin_len;
>>>>>> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
>>>>>>      return false;
>>>>>>     }
>>>>>>
>>>>>> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>>>>>> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
>>>>>> + uint8_t bdaddr_type)
>>>>>> +
>>>>>>     {
>>>>>>      struct link_key_info *info = NULL;
>>>>>>      char *str;
>>>>>> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>>>>>>      info = g_new0(struct link_key_info, 1);
>>>>>>
>>>>>>      str2ba(peer, &info->bdaddr);
>>>>>> + info->bdaddr_type = bdaddr_type;
>>>>>>
>>>>>>      if (!strncmp(str, "0x", 2))
>>>>>>      str2buf(&str[2], info->key, sizeof(info->key));
>>>>>> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
>>>>>>      struct link_key_info *info = l->data;
>>>>>>
>>>>>>      bacpy(&key->addr.bdaddr, &info->bdaddr);
>>>>>> - key->addr.type = BDADDR_BREDR;
>>>>>> + key->addr.type = info->bdaddr_type;
>>>>>>      key->type = info->type;
>>>>>>      memcpy(key->val, info->key, 16);
>>>>>>      key->pin_len = info->pin_len;
>>>>>> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
>>>>>>      btd_error(adapter->dev_id, "Load connection parameters failed");
>>>>>>     }
>>>>>>
>>>>>> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
>>>>>> +static uint8_t get_addr_type(GKeyFile *keyfile)
>>>>>>     {
>>>>>>      uint8_t addr_type;
>>>>>>      char *type;
>>>>>>
>>>>>> + /* The AddressType is written to file only When dev->le is
>>>>>> + * set to true, as referenced in the update_technologies().
>>>>>> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
>>>>>> + */
>>>>>>      type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
>>>>>>      if (!type)
>>>>>> - return BDADDR_LE_PUBLIC;
>>>>>> + return BDADDR_BREDR;
>>>>>>
>>>>>>      if (g_str_equal(type, "public"))
>>>>>>      addr_type = BDADDR_LE_PUBLIC;
>>>>>> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
>>>>>>      g_clear_error(&gerr);
>>>>>>      }
>>>>>>
>>>>>> - key_info = get_key_info(key_file, entry->d_name);
>>>>>> + bdaddr_type = get_addr_type(key_file);
>>>>>>
>>>>>> - bdaddr_type = get_le_addr_type(key_file);
>>>>>> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>>>>>>
>>>>>>      ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>>>>>>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
>>>>>>
>>>>>> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
>>>>>>
>>>>>> `systemctl status bluetooth` reports the following error:
>>>>>>
>>>>>> [...]
>>>>>> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
>>>>>> [...]
>>>>>>
>>>>>> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
>>>>>> I can confirm that reverting it solves the issue.
>>>>>>
>>>>>> I reported this bug including details in the GitHub repo [2].
>>>>>>
>>>>>> I remain available if any additional information are needed.
>>>>>>
>>>>>> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
>>>>>> [2] https://github.com/bluez/bluez/issues/686
>>>>>>
>>>>>> --
>>>>>> Regards,
>>>>>> Robin Candau / Antiz
>>>>>
>>>>> Perhaps related to:
>>>>> https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?
>>>>
>>>
>>>
>>
> 
>

Luiz Augusto von Dentz Aug. 27, 2024, 6:30 p.m. UTC | #10

Hi Yao,

On Tue, Aug 27, 2024 at 2:03 PM Yao Xiao <[e-mail]> wrote:
>
> Hi,
>
> On 2024/8/27 23:47, Luiz Augusto von Dentz wrote:
> > Hi,
> >
> > On Tue, Aug 27, 2024 at 10:33 AM Yao Xiao <[e-mail]> wrote:
> >>
> >> Hi Luiz,
> >>
> >> On 2024/8/27 20:52, Luiz Augusto von Dentz wrote:
> >>> Hi,
> >>>
> >>> On Tue, Aug 27, 2024 at 3:41 AM Yao Xiao <[e-mail]> wrote:
> >>>>
> >>>> Hi luiz,
> >>>> The kernel should always report the correct address type for all kinds
> >>>> of keys. Older kernels ignore the derivation of keys, which prevents the
> >>>> user layer from knowing the correct address type. This can cause a lot
> >>>> of confusion for users. For example, when a user connects to a
> >>>> traditional Bluetooth audio device, due to the default activation of key
> >>>> derivation, bluetoothd reports both BREDR and BLE pairing and binding,
> >>>> and the address is updated to LE public. When the user actively calls
> >>>> the Connect method of org.bluez.Device1 to reconnect, sometimes the
> >>>> address type is set to LE public, leading to a failed connection, and
> >>>> vice versa.
> >>>
> >>> The correct address type can only be the ones listed on mgmt-api
> >>> otherwise we are breaking APIs, also except if there are some new case
> >>> I don't know and Load Keys and Load LTK are bearer specific.
> >>>
> >>>> We should enable bluetoothd to know the reason for the generation of
> >>>> keys as much as possible. Older kernels always assume the address type
> >>>> of linkey is BREDR, and ltk/irk/csrk are LE public. Therefore, we can
> >>>> use this as a basis for judgment, allowing bluetoothd to handle various
> >>>> issues, such as reloading keys and handling unpaired devices. Below is a
> >>>> pseudo-code that is not running in practice but just a thought. Please
> >>>> help review it.
> >>>
> >>> Yeah, and what exactly was wrong with that? What you mention about
> >>> having keys for both BR/EDR and LE public is not an issue, that
> >>> basically the very definition of a dual-mode device and bluetoothd
> >>> shall be able to tell when to connect to BR/EDR and LE public, if it
> >>> doesn't we shall fix that and not start breaking APIs.
> >> Returning to my original issue: When BlueZ acts as a sink device and a
> >> phone connects to it, pairs successfully, and derives related keys,
> >> BlueZ reports to the user layer that bonded (both bredr_state.bonded and
> >> dev->le_state.bonded) are true, and addr_type is LE_PUBLIC. If the
> >> device restarts and the org.bluez.Device1:Connect method is called to
> >> reconnect to the phone, according to the rules in select_conn_bearer,
> >> addr_type will be set to LE_PUBLIC, leading to a failed reconnection
> >> when device_connect_le is called.
> >
> > If I recall this correctly we do reconnect based on the last seen
> > bearer, also the addr_type is only really relevant for LE bearer,
> > BR/EDR shall be detected via bearer state alone. Anyway there was a
> > discussion regarding having bearer specific API's as alternative to
> > org.bluez.Device1.Connect to pickup a bearer, but in some cases, like
> > when acting as a peripheral, perhaps we need a different policy, in
> > the specific case of being a A2DP sink though, we could detect that LE
> > keys are for a peripheral and in that case advertisement shall be used
> > for connecting the LE bearer thus the BR/EDR shall be chosen which
> > shall fix this behavior.
> >
> >> In this situation, how should bluetoothd choose the correct address type
> >> when it doesn't know the physical bearer layer? Should there be a
> >> configuration option to turn key derivation on or off? Additionally,
> >> there's another use case: When a phone connects to a BlueZ device, it
> >> continuously transmits custom data via BLE and music via traditional
> >> Bluetooth (BREDR). If the user only wants to unpair the BREDR, it may
> >> cause the BLE connection to be disconnected as well, leading to data
> >> transmission interruption. Should we also consider turning off key
> >> derivation in this scenario?
> >
> > Device API is not bearer specific though, so one cannot really unpair
> > BR/EDR alone, in fact I would be surprised if any stack implements
> > this that way because depending on what bearer is connected it may
> > trigger a missing link key which could possibly lead to repairing
> > procedure which will most likely having to replace the other bearer
> > keys as well, if the idea is you only really need BR/EDR then you
> > might as well disable LE completely.
>
> Could we possibly implement a configuration that allows for the enabling
> or disabling of key derivation? Currently, a significant portion of
> devices only require either BR/EDR or BLE functionality independently
> and do not need key derivation, which can cause some confusion and
> issues for users. Additionally, as I understand, there are some
> proprietary RTOS Bluetooth protocol stacks that have disabled key
> derivation and support separate management of pairing keys for BR/EDR
> and BLE devices. For example, an IoT device that supports both BR/EDR
> and BLE could be detected as two separate devices by an iPhone, sharing
> the same address—one for BLE used for ANCS notifications (Apple
> Notification Center Service (ANCS) Specification) and the other for
> A2DP. The phone could connect to or remove either device without
> interference between the two.

That will likely require a lot of work to get working since we would
endup with the same device object, if this really exists in real life
Im afraid it breaks a lot of things in the process and it will never
be backwards compatible, so I'm really not convinced this is the right
thing to do, in fact Id claim that if a device wants to have separate
pairing/bonding procedures then it must use different addresses which
means the LE bearer shall probably use a random static address rather
than attempt to reuse the same identity address of BR/EDR, afterall it
want to have distinct devices so it really makes no sense to keep
using the same address.

>
> >
> >>
> >>>
> >>>> -------------------------------------------
> >>>> diff --git a/src/adapter.c b/src/adapter.c
> >>>> index 85ddfc165..babe7c9b2 100644
> >>>> --- a/src/adapter.c
> >>>> +++ b/src/adapter.c
> >>>> @@ -4958,6 +4958,7 @@ static void load_devices(struct btd_adapter *adapter)
> >>>>             struct irk_info *irk_info;
> >>>>             struct conn_param *param;
> >>>>             uint8_t bdaddr_type;
> >>>> +        bool derived_key;
> >>>>
> >>>>             if (entry->d_type == DT_UNKNOWN)
> >>>>                 entry->d_type = util_get_dt(dirname, entry->d_name);
> >>>> @@ -4976,10 +4977,21 @@ static void load_devices(struct btd_adapter
> >>>> *adapter)
> >>>>                 g_clear_error(&gerr);
> >>>>             }
> >>>>
> >>>> +        derived_key = g_key_file_get_boolean(key_file, "General",
> >>>> +                                "DerivedKey", NULL);
> >>>> +
> >>>> +        /* For link key */
> >>>>             bdaddr_type = get_addr_type(key_file);
> >>>> +        if (!derived_key)
> >>>> +            bdaddr_type = BDADDR_BREDR;
> >>>>
> >>>>             key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
> >>>>
> >>>> +        /* For ltk/irk/csrk */
> >>>> +        bdaddr_type = get_addr_type(key_file);
> >>>> +        if (!derived_key)
> >>>> +            bdaddr_type = BDADDR_LE_PUBLIC;
> >>>> +
> >>>>             ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> >>>>
> >>>>             peripheral_ltk_info = get_peripheral_ltk_info(key_file,
> >>>> @@ -8673,6 +8685,12 @@ static void new_link_key_callback(uint16_t index,
> >>>> uint16_t length,
> >>>>             return;
> >>>>         }
> >>>>
> >>>> +    /*
> >>>> +     * For older kernels, the address type is always BREDR.
> >>>> +     */
> >>>> +    if (addr->type == BDADDR_LE_PUBLIC)
> >>>> +        device->derived_key = true;
> >>>> +
> >>>>         if (ev->store_hint) {
> >>>>             const struct mgmt_link_key_info *key = &ev->key;
> >>>>
> >>>> @@ -8791,6 +8809,12 @@ static void new_long_term_key_callback(uint16_t
> >>>> index, uint16_t length,
> >>>>             return;
> >>>>         }
> >>>>
> >>>> +    /*
> >>>> +     * For older kernels, the address type is always BDADDR_LE_PUBLIC.
> >>>> +     */
> >>>> +    if (addr->type == BDADDR_BREDR)
> >>>> +        device->derived_key = true;
> >>>> +
> >>>>         /*
> >>>>          * Some older kernel versions set store_hint for long term keys
> >>>>          * from resolvable and unresolvable random addresses, but there
> >>>> @@ -8855,6 +8879,9 @@ static void new_csrk_callback(uint16_t index,
> >>>> uint16_t length,
> >>>>             return;
> >>>>         }
> >>>>
> >>>> +    if (addr->type == BDADDR_BREDR)
> >>>> +        device->derived_key = true;
> >>>> +
> >>>>         device_set_csrk(device, key->val, 0, key->type, ev->store_hint);
> >>>>     }
> >>>>
> >>>> @@ -8941,6 +8968,9 @@ static void new_irk_callback(uint16_t index,
> >>>> uint16_t length,
> >>>>             return;
> >>>>         }
> >>>>
> >>>> +    if (addr->type == BDADDR_BREDR)
> >>>> +        device->derived_key = true;
> >>>> +
> >>>>         device_update_addr(device, &addr->bdaddr, addr->type);
> >>>>
> >>>>         if (duplicate)
> >>>> diff --git a/src/device.c b/src/device.c
> >>>> index a1dc0750c..062b9c49d 100644
> >>>> --- a/src/device.c
> >>>> +++ b/src/device.c
> >>>> @@ -272,6 +272,7 @@ struct btd_device {
> >>>>         struct csrk_info *remote_csrk;
> >>>>         struct ltk_info *ltk;
> >>>>         struct queue    *sirks;
> >>>> +    bool        derived_key;            /* key is derived. */
> >>>>
> >>>>         sdp_list_t    *tmp_records;
> >>>>
> >>>> @@ -482,6 +483,9 @@ static gboolean store_device_info_cb(gpointer user_data)
> >>>>         g_key_file_set_boolean(key_file, "General", "Blocked",
> >>>>                                 device->blocked);
> >>>>
> >>>> +    g_key_file_set_boolean(key_file, "General", "DerivedKey",
> >>>> +                            device->derived_key);
> >>>> +
> >>>>         if (device->wake_override != WAKE_FLAG_DEFAULT) {
> >>>>             g_key_file_set_boolean(key_file, "General", "WakeAllowed",
> >>>>                            device->wake_override ==
> >>>> @@ -1829,7 +1833,11 @@ static void bonding_request_cancel(struct
> >>>> bonding_req *bonding)
> >>>>         struct btd_device *device = bonding->device;
> >>>>         struct btd_adapter *adapter = device->adapter;
> >>>>
> >>>> -    adapter_cancel_bonding(adapter, &device->bdaddr, device->bdaddr_type);
> >>>> +    if (device->derived_key) {
> >>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_BREDR);
> >>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_LE_PUBLIC);
> >>>> +    } else
> >>>> +        adapter_cancel_bonding(adapter, &device->bdaddr,
> >>>> device->bdaddr_type);
> >>>>     }
> >>>>
> >>>>     static void dev_disconn_service(gpointer a, gpointer b)
> >>>> @@ -3196,12 +3204,19 @@ static DBusMessage
> >>>> *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
> >>>>     {
> >>>>         struct btd_device *device = data;
> >>>>         struct bonding_req *req = device->bonding;
> >>>> +    uint8_t bdaddr_type;
> >>>>
> >>>>         DBG("");
> >>>>
> >>>>         if (!req) {
> >>>> -        btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >>>> -                        device->bdaddr_type);
> >>>> +        if (device->derived_key) {
> >>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >>>> +                            BDADDR_BREDR);
> >>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >>>> +                            BDADDR_LE_PUBLIC);
> >>>> +        } else
> >>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >>>> +                            device->bdaddr_type);
> >>>>             return btd_error_does_not_exist(msg);
> >>>>         }
> >>>>
> >>>> @@ -3833,6 +3848,9 @@ next:
> >>>>             gerr = NULL;
> >>>>         }
> >>>>
> >>>> +    device->derived_key = g_key_file_get_boolean(key_file, "General",
> >>>> +                            "DerivedKey", NULL);
> >>>> +
> >>>>         if (store_needed)
> >>>>             store_device_info(device);
> >>>>     }
> >>>>
> >>>> On 2024/8/27 5:12, Luiz Augusto von Dentz wrote:
> >>>>> Hi Xiao,
> >>>>>
> >>>>> On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
> >>>>>>
> >>>>>> On 12/11/23 17:27, Xiao Yao wrote:
> >>>>>>
> >>>>>> From: Xiao Yao <[e-mail]>
> >>>>>>
> >>>>>> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
> >>>>>> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
> >>>>>> converted between each other, so the addr type can be either
> >>>>>> BREDR or LE, so fix it.
> >>>>>>
> >>>>>> Signed-off-by: Xiao Yao <[e-mail]>
> >>>>>> ---
> >>>>>>     src/adapter.c | 20 ++++++++++++++------
> >>>>>>     1 file changed, 14 insertions(+), 6 deletions(-)
> >>>>>>
> >>>>>> diff --git a/src/adapter.c b/src/adapter.c
> >>>>>> index 86fff72bc..ee70b00d2 100644
> >>>>>> --- a/src/adapter.c
> >>>>>> +++ b/src/adapter.c
> >>>>>> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
> >>>>>>
> >>>>>>     struct link_key_info {
> >>>>>>      bdaddr_t bdaddr;
> >>>>>> + uint8_t bdaddr_type;
> >>>>>>      unsigned char key[16];
> >>>>>>      uint8_t type;
> >>>>>>      uint8_t pin_len;
> >>>>>> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> >>>>>>      return false;
> >>>>>>     }
> >>>>>>
> >>>>>> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >>>>>> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
> >>>>>> + uint8_t bdaddr_type)
> >>>>>> +
> >>>>>>     {
> >>>>>>      struct link_key_info *info = NULL;
> >>>>>>      char *str;
> >>>>>> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >>>>>>      info = g_new0(struct link_key_info, 1);
> >>>>>>
> >>>>>>      str2ba(peer, &info->bdaddr);
> >>>>>> + info->bdaddr_type = bdaddr_type;
> >>>>>>
> >>>>>>      if (!strncmp(str, "0x", 2))
> >>>>>>      str2buf(&str[2], info->key, sizeof(info->key));
> >>>>>> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
> >>>>>>      struct link_key_info *info = l->data;
> >>>>>>
> >>>>>>      bacpy(&key->addr.bdaddr, &info->bdaddr);
> >>>>>> - key->addr.type = BDADDR_BREDR;
> >>>>>> + key->addr.type = info->bdaddr_type;
> >>>>>>      key->type = info->type;
> >>>>>>      memcpy(key->val, info->key, 16);
> >>>>>>      key->pin_len = info->pin_len;
> >>>>>> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
> >>>>>>      btd_error(adapter->dev_id, "Load connection parameters failed");
> >>>>>>     }
> >>>>>>
> >>>>>> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
> >>>>>> +static uint8_t get_addr_type(GKeyFile *keyfile)
> >>>>>>     {
> >>>>>>      uint8_t addr_type;
> >>>>>>      char *type;
> >>>>>>
> >>>>>> + /* The AddressType is written to file only When dev->le is
> >>>>>> + * set to true, as referenced in the update_technologies().
> >>>>>> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
> >>>>>> + */
> >>>>>>      type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
> >>>>>>      if (!type)
> >>>>>> - return BDADDR_LE_PUBLIC;
> >>>>>> + return BDADDR_BREDR;
> >>>>>>
> >>>>>>      if (g_str_equal(type, "public"))
> >>>>>>      addr_type = BDADDR_LE_PUBLIC;
> >>>>>> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
> >>>>>>      g_clear_error(&gerr);
> >>>>>>      }
> >>>>>>
> >>>>>> - key_info = get_key_info(key_file, entry->d_name);
> >>>>>> + bdaddr_type = get_addr_type(key_file);
> >>>>>>
> >>>>>> - bdaddr_type = get_le_addr_type(key_file);
> >>>>>> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
> >>>>>>
> >>>>>>      ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> >>>>>>
> >>>>>>
> >>>>>> Hello,
> >>>>>>
> >>>>>> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
> >>>>>>
> >>>>>> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
> >>>>>>
> >>>>>> `systemctl status bluetooth` reports the following error:
> >>>>>>
> >>>>>> [...]
> >>>>>> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
> >>>>>> [...]
> >>>>>>
> >>>>>> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
> >>>>>> I can confirm that reverting it solves the issue.
> >>>>>>
> >>>>>> I reported this bug including details in the GitHub repo [2].
> >>>>>>
> >>>>>> I remain available if any additional information are needed.
> >>>>>>
> >>>>>> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
> >>>>>> [2] https://github.com/bluez/bluez/issues/686
> >>>>>>
> >>>>>> --
> >>>>>> Regards,
> >>>>>> Robin Candau / Antiz
> >>>>>
> >>>>> Perhaps related to:
> >>>>> https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?
> >>>>
> >>>
> >>>
> >>
> >
> >
>

Yao Xiao Aug. 28, 2024, 10:01 a.m. UTC | #11

Hi Luiz,

On 2024/8/28 2:30, Luiz Augusto von Dentz wrote:
> Hi Yao,
> 
> On Tue, Aug 27, 2024 at 2:03 PM Yao Xiao <[e-mail]> wrote:
>>
>> Hi,
>>
>> On 2024/8/27 23:47, Luiz Augusto von Dentz wrote:
>>> Hi,
>>>
>>> On Tue, Aug 27, 2024 at 10:33 AM Yao Xiao <[e-mail]> wrote:
>>>>
>>>> Hi Luiz,
>>>>
>>>> On 2024/8/27 20:52, Luiz Augusto von Dentz wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Aug 27, 2024 at 3:41 AM Yao Xiao <[e-mail]> wrote:
>>>>>>
>>>>>> Hi luiz,
>>>>>> The kernel should always report the correct address type for all kinds
>>>>>> of keys. Older kernels ignore the derivation of keys, which prevents the
>>>>>> user layer from knowing the correct address type. This can cause a lot
>>>>>> of confusion for users. For example, when a user connects to a
>>>>>> traditional Bluetooth audio device, due to the default activation of key
>>>>>> derivation, bluetoothd reports both BREDR and BLE pairing and binding,
>>>>>> and the address is updated to LE public. When the user actively calls
>>>>>> the Connect method of org.bluez.Device1 to reconnect, sometimes the
>>>>>> address type is set to LE public, leading to a failed connection, and
>>>>>> vice versa.
>>>>>
>>>>> The correct address type can only be the ones listed on mgmt-api
>>>>> otherwise we are breaking APIs, also except if there are some new case
>>>>> I don't know and Load Keys and Load LTK are bearer specific.
>>>>>
>>>>>> We should enable bluetoothd to know the reason for the generation of
>>>>>> keys as much as possible. Older kernels always assume the address type
>>>>>> of linkey is BREDR, and ltk/irk/csrk are LE public. Therefore, we can
>>>>>> use this as a basis for judgment, allowing bluetoothd to handle various
>>>>>> issues, such as reloading keys and handling unpaired devices. Below is a
>>>>>> pseudo-code that is not running in practice but just a thought. Please
>>>>>> help review it.
>>>>>
>>>>> Yeah, and what exactly was wrong with that? What you mention about
>>>>> having keys for both BR/EDR and LE public is not an issue, that
>>>>> basically the very definition of a dual-mode device and bluetoothd
>>>>> shall be able to tell when to connect to BR/EDR and LE public, if it
>>>>> doesn't we shall fix that and not start breaking APIs.
>>>> Returning to my original issue: When BlueZ acts as a sink device and a
>>>> phone connects to it, pairs successfully, and derives related keys,
>>>> BlueZ reports to the user layer that bonded (both bredr_state.bonded and
>>>> dev->le_state.bonded) are true, and addr_type is LE_PUBLIC. If the
>>>> device restarts and the org.bluez.Device1:Connect method is called to
>>>> reconnect to the phone, according to the rules in select_conn_bearer,
>>>> addr_type will be set to LE_PUBLIC, leading to a failed reconnection
>>>> when device_connect_le is called.
>>>
>>> If I recall this correctly we do reconnect based on the last seen
>>> bearer, also the addr_type is only really relevant for LE bearer,
>>> BR/EDR shall be detected via bearer state alone. Anyway there was a
>>> discussion regarding having bearer specific API's as alternative to
>>> org.bluez.Device1.Connect to pickup a bearer, but in some cases, like
>>> when acting as a peripheral, perhaps we need a different policy, in
>>> the specific case of being a A2DP sink though, we could detect that LE
>>> keys are for a peripheral and in that case advertisement shall be used
>>> for connecting the LE bearer thus the BR/EDR shall be chosen which
>>> shall fix this behavior.
>>>
>>>> In this situation, how should bluetoothd choose the correct address type
>>>> when it doesn't know the physical bearer layer? Should there be a
>>>> configuration option to turn key derivation on or off? Additionally,
>>>> there's another use case: When a phone connects to a BlueZ device, it
>>>> continuously transmits custom data via BLE and music via traditional
>>>> Bluetooth (BREDR). If the user only wants to unpair the BREDR, it may
>>>> cause the BLE connection to be disconnected as well, leading to data
>>>> transmission interruption. Should we also consider turning off key
>>>> derivation in this scenario?
>>>
>>> Device API is not bearer specific though, so one cannot really unpair
>>> BR/EDR alone, in fact I would be surprised if any stack implements
>>> this that way because depending on what bearer is connected it may
>>> trigger a missing link key which could possibly lead to repairing
>>> procedure which will most likely having to replace the other bearer
>>> keys as well, if the idea is you only really need BR/EDR then you
>>> might as well disable LE completely.
>>
>> Could we possibly implement a configuration that allows for the enabling
>> or disabling of key derivation? Currently, a significant portion of
>> devices only require either BR/EDR or BLE functionality independently
>> and do not need key derivation, which can cause some confusion and
>> issues for users. Additionally, as I understand, there are some
>> proprietary RTOS Bluetooth protocol stacks that have disabled key
>> derivation and support separate management of pairing keys for BR/EDR
>> and BLE devices. For example, an IoT device that supports both BR/EDR
>> and BLE could be detected as two separate devices by an iPhone, sharing
>> the same address—one for BLE used for ANCS notifications (Apple
>> Notification Center Service (ANCS) Specification) and the other for
>> A2DP. The phone could connect to or remove either device without
>> interference between the two.
> 
> That will likely require a lot of work to get working since we would
> endup with the same device object, if this really exists in real life
> Im afraid it breaks a lot of things in the process and it will never
> be backwards compatible, so I'm really not convinced this is the right
> thing to do, in fact Id claim that if a device wants to have separate
> pairing/bonding procedures then it must use different addresses which
> means the LE bearer shall probably use a random static address rather
> than attempt to reuse the same identity address of BR/EDR, afterall it
> want to have distinct devices so it really makes no sense to keep
> using the same address.

BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 6, Part B
1.3 DEVICE ADDRESS
Devices are identified using a device address and an address type;
Whenever two device addresses are compared, the comparison shall include
the device address type (i.e. if the two addresses have different types, 
they are different even if the two 48-bit addresses are the same).

According to the specification, even if the addresses are the same, as 
long as the address types are different, they are considered two 
different devices; the iPhone(iOS) can scan for two devices: they have 
the same address but different address types, and can be paired and 
unpaired independently.

> 
>>
>>>
>>>>
>>>>>
>>>>>> -------------------------------------------
>>>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>>>> index 85ddfc165..babe7c9b2 100644
>>>>>> --- a/src/adapter.c
>>>>>> +++ b/src/adapter.c
>>>>>> @@ -4958,6 +4958,7 @@ static void load_devices(struct btd_adapter *adapter)
>>>>>>              struct irk_info *irk_info;
>>>>>>              struct conn_param *param;
>>>>>>              uint8_t bdaddr_type;
>>>>>> +        bool derived_key;
>>>>>>
>>>>>>              if (entry->d_type == DT_UNKNOWN)
>>>>>>                  entry->d_type = util_get_dt(dirname, entry->d_name);
>>>>>> @@ -4976,10 +4977,21 @@ static void load_devices(struct btd_adapter
>>>>>> *adapter)
>>>>>>                  g_clear_error(&gerr);
>>>>>>              }
>>>>>>
>>>>>> +        derived_key = g_key_file_get_boolean(key_file, "General",
>>>>>> +                                "DerivedKey", NULL);
>>>>>> +
>>>>>> +        /* For link key */
>>>>>>              bdaddr_type = get_addr_type(key_file);
>>>>>> +        if (!derived_key)
>>>>>> +            bdaddr_type = BDADDR_BREDR;
>>>>>>
>>>>>>              key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>>>>>>
>>>>>> +        /* For ltk/irk/csrk */
>>>>>> +        bdaddr_type = get_addr_type(key_file);
>>>>>> +        if (!derived_key)
>>>>>> +            bdaddr_type = BDADDR_LE_PUBLIC;
>>>>>> +
>>>>>>              ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>>>>>>
>>>>>>              peripheral_ltk_info = get_peripheral_ltk_info(key_file,
>>>>>> @@ -8673,6 +8685,12 @@ static void new_link_key_callback(uint16_t index,
>>>>>> uint16_t length,
>>>>>>              return;
>>>>>>          }
>>>>>>
>>>>>> +    /*
>>>>>> +     * For older kernels, the address type is always BREDR.
>>>>>> +     */
>>>>>> +    if (addr->type == BDADDR_LE_PUBLIC)
>>>>>> +        device->derived_key = true;
>>>>>> +
>>>>>>          if (ev->store_hint) {
>>>>>>              const struct mgmt_link_key_info *key = &ev->key;
>>>>>>
>>>>>> @@ -8791,6 +8809,12 @@ static void new_long_term_key_callback(uint16_t
>>>>>> index, uint16_t length,
>>>>>>              return;
>>>>>>          }
>>>>>>
>>>>>> +    /*
>>>>>> +     * For older kernels, the address type is always BDADDR_LE_PUBLIC.
>>>>>> +     */
>>>>>> +    if (addr->type == BDADDR_BREDR)
>>>>>> +        device->derived_key = true;
>>>>>> +
>>>>>>          /*
>>>>>>           * Some older kernel versions set store_hint for long term keys
>>>>>>           * from resolvable and unresolvable random addresses, but there
>>>>>> @@ -8855,6 +8879,9 @@ static void new_csrk_callback(uint16_t index,
>>>>>> uint16_t length,
>>>>>>              return;
>>>>>>          }
>>>>>>
>>>>>> +    if (addr->type == BDADDR_BREDR)
>>>>>> +        device->derived_key = true;
>>>>>> +
>>>>>>          device_set_csrk(device, key->val, 0, key->type, ev->store_hint);
>>>>>>      }
>>>>>>
>>>>>> @@ -8941,6 +8968,9 @@ static void new_irk_callback(uint16_t index,
>>>>>> uint16_t length,
>>>>>>              return;
>>>>>>          }
>>>>>>
>>>>>> +    if (addr->type == BDADDR_BREDR)
>>>>>> +        device->derived_key = true;
>>>>>> +
>>>>>>          device_update_addr(device, &addr->bdaddr, addr->type);
>>>>>>
>>>>>>          if (duplicate)
>>>>>> diff --git a/src/device.c b/src/device.c
>>>>>> index a1dc0750c..062b9c49d 100644
>>>>>> --- a/src/device.c
>>>>>> +++ b/src/device.c
>>>>>> @@ -272,6 +272,7 @@ struct btd_device {
>>>>>>          struct csrk_info *remote_csrk;
>>>>>>          struct ltk_info *ltk;
>>>>>>          struct queue    *sirks;
>>>>>> +    bool        derived_key;            /* key is derived. */
>>>>>>
>>>>>>          sdp_list_t    *tmp_records;
>>>>>>
>>>>>> @@ -482,6 +483,9 @@ static gboolean store_device_info_cb(gpointer user_data)
>>>>>>          g_key_file_set_boolean(key_file, "General", "Blocked",
>>>>>>                                  device->blocked);
>>>>>>
>>>>>> +    g_key_file_set_boolean(key_file, "General", "DerivedKey",
>>>>>> +                            device->derived_key);
>>>>>> +
>>>>>>          if (device->wake_override != WAKE_FLAG_DEFAULT) {
>>>>>>              g_key_file_set_boolean(key_file, "General", "WakeAllowed",
>>>>>>                             device->wake_override ==
>>>>>> @@ -1829,7 +1833,11 @@ static void bonding_request_cancel(struct
>>>>>> bonding_req *bonding)
>>>>>>          struct btd_device *device = bonding->device;
>>>>>>          struct btd_adapter *adapter = device->adapter;
>>>>>>
>>>>>> -    adapter_cancel_bonding(adapter, &device->bdaddr, device->bdaddr_type);
>>>>>> +    if (device->derived_key) {
>>>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_BREDR);
>>>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_LE_PUBLIC);
>>>>>> +    } else
>>>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr,
>>>>>> device->bdaddr_type);
>>>>>>      }
>>>>>>
>>>>>>      static void dev_disconn_service(gpointer a, gpointer b)
>>>>>> @@ -3196,12 +3204,19 @@ static DBusMessage
>>>>>> *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
>>>>>>      {
>>>>>>          struct btd_device *device = data;
>>>>>>          struct bonding_req *req = device->bonding;
>>>>>> +    uint8_t bdaddr_type;
>>>>>>
>>>>>>          DBG("");
>>>>>>
>>>>>>          if (!req) {
>>>>>> -        btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>>>> -                        device->bdaddr_type);
>>>>>> +        if (device->derived_key) {
>>>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>>>> +                            BDADDR_BREDR);
>>>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>>>> +                            BDADDR_LE_PUBLIC);
>>>>>> +        } else
>>>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>>>> +                            device->bdaddr_type);
>>>>>>              return btd_error_does_not_exist(msg);
>>>>>>          }
>>>>>>
>>>>>> @@ -3833,6 +3848,9 @@ next:
>>>>>>              gerr = NULL;
>>>>>>          }
>>>>>>
>>>>>> +    device->derived_key = g_key_file_get_boolean(key_file, "General",
>>>>>> +                            "DerivedKey", NULL);
>>>>>> +
>>>>>>          if (store_needed)
>>>>>>              store_device_info(device);
>>>>>>      }
>>>>>>
>>>>>> On 2024/8/27 5:12, Luiz Augusto von Dentz wrote:
>>>>>>> Hi Xiao,
>>>>>>>
>>>>>>> On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On 12/11/23 17:27, Xiao Yao wrote:
>>>>>>>>
>>>>>>>> From: Xiao Yao <[e-mail]>
>>>>>>>>
>>>>>>>> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
>>>>>>>> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
>>>>>>>> converted between each other, so the addr type can be either
>>>>>>>> BREDR or LE, so fix it.
>>>>>>>>
>>>>>>>> Signed-off-by: Xiao Yao <[e-mail]>
>>>>>>>> ---
>>>>>>>>      src/adapter.c | 20 ++++++++++++++------
>>>>>>>>      1 file changed, 14 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>>>>>> index 86fff72bc..ee70b00d2 100644
>>>>>>>> --- a/src/adapter.c
>>>>>>>> +++ b/src/adapter.c
>>>>>>>> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
>>>>>>>>
>>>>>>>>      struct link_key_info {
>>>>>>>>       bdaddr_t bdaddr;
>>>>>>>> + uint8_t bdaddr_type;
>>>>>>>>       unsigned char key[16];
>>>>>>>>       uint8_t type;
>>>>>>>>       uint8_t pin_len;
>>>>>>>> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
>>>>>>>>       return false;
>>>>>>>>      }
>>>>>>>>
>>>>>>>> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>>>>>>>> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
>>>>>>>> + uint8_t bdaddr_type)
>>>>>>>> +
>>>>>>>>      {
>>>>>>>>       struct link_key_info *info = NULL;
>>>>>>>>       char *str;
>>>>>>>> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>>>>>>>>       info = g_new0(struct link_key_info, 1);
>>>>>>>>
>>>>>>>>       str2ba(peer, &info->bdaddr);
>>>>>>>> + info->bdaddr_type = bdaddr_type;
>>>>>>>>
>>>>>>>>       if (!strncmp(str, "0x", 2))
>>>>>>>>       str2buf(&str[2], info->key, sizeof(info->key));
>>>>>>>> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
>>>>>>>>       struct link_key_info *info = l->data;
>>>>>>>>
>>>>>>>>       bacpy(&key->addr.bdaddr, &info->bdaddr);
>>>>>>>> - key->addr.type = BDADDR_BREDR;
>>>>>>>> + key->addr.type = info->bdaddr_type;
>>>>>>>>       key->type = info->type;
>>>>>>>>       memcpy(key->val, info->key, 16);
>>>>>>>>       key->pin_len = info->pin_len;
>>>>>>>> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
>>>>>>>>       btd_error(adapter->dev_id, "Load connection parameters failed");
>>>>>>>>      }
>>>>>>>>
>>>>>>>> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
>>>>>>>> +static uint8_t get_addr_type(GKeyFile *keyfile)
>>>>>>>>      {
>>>>>>>>       uint8_t addr_type;
>>>>>>>>       char *type;
>>>>>>>>
>>>>>>>> + /* The AddressType is written to file only When dev->le is
>>>>>>>> + * set to true, as referenced in the update_technologies().
>>>>>>>> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
>>>>>>>> + */
>>>>>>>>       type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
>>>>>>>>       if (!type)
>>>>>>>> - return BDADDR_LE_PUBLIC;
>>>>>>>> + return BDADDR_BREDR;
>>>>>>>>
>>>>>>>>       if (g_str_equal(type, "public"))
>>>>>>>>       addr_type = BDADDR_LE_PUBLIC;
>>>>>>>> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
>>>>>>>>       g_clear_error(&gerr);
>>>>>>>>       }
>>>>>>>>
>>>>>>>> - key_info = get_key_info(key_file, entry->d_name);
>>>>>>>> + bdaddr_type = get_addr_type(key_file);
>>>>>>>>
>>>>>>>> - bdaddr_type = get_le_addr_type(key_file);
>>>>>>>> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>>>>>>>>
>>>>>>>>       ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>>>>>>>>
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
>>>>>>>>
>>>>>>>> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
>>>>>>>>
>>>>>>>> `systemctl status bluetooth` reports the following error:
>>>>>>>>
>>>>>>>> [...]
>>>>>>>> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
>>>>>>>> [...]
>>>>>>>>
>>>>>>>> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
>>>>>>>> I can confirm that reverting it solves the issue.
>>>>>>>>
>>>>>>>> I reported this bug including details in the GitHub repo [2].
>>>>>>>>
>>>>>>>> I remain available if any additional information are needed.
>>>>>>>>
>>>>>>>> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
>>>>>>>> [2] https://github.com/bluez/bluez/issues/686
>>>>>>>>
>>>>>>>> --
>>>>>>>> Regards,
>>>>>>>> Robin Candau / Antiz
>>>>>>>
>>>>>>> Perhaps related to:
>>>>>>> https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
>

Luiz Augusto von Dentz Aug. 28, 2024, 2:37 p.m. UTC | #12

Hi,

On Wed, Aug 28, 2024 at 6:03 AM Yao Xiao <[e-mail]> wrote:
>
> Hi Luiz,
>
> On 2024/8/28 2:30, Luiz Augusto von Dentz wrote:
> > Hi Yao,
> >
> > On Tue, Aug 27, 2024 at 2:03 PM Yao Xiao <[e-mail]> wrote:
> >>
> >> Hi,
> >>
> >> On 2024/8/27 23:47, Luiz Augusto von Dentz wrote:
> >>> Hi,
> >>>
> >>> On Tue, Aug 27, 2024 at 10:33 AM Yao Xiao <[e-mail]> wrote:
> >>>>
> >>>> Hi Luiz,
> >>>>
> >>>> On 2024/8/27 20:52, Luiz Augusto von Dentz wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Tue, Aug 27, 2024 at 3:41 AM Yao Xiao <[e-mail]> wrote:
> >>>>>>
> >>>>>> Hi luiz,
> >>>>>> The kernel should always report the correct address type for all kinds
> >>>>>> of keys. Older kernels ignore the derivation of keys, which prevents the
> >>>>>> user layer from knowing the correct address type. This can cause a lot
> >>>>>> of confusion for users. For example, when a user connects to a
> >>>>>> traditional Bluetooth audio device, due to the default activation of key
> >>>>>> derivation, bluetoothd reports both BREDR and BLE pairing and binding,
> >>>>>> and the address is updated to LE public. When the user actively calls
> >>>>>> the Connect method of org.bluez.Device1 to reconnect, sometimes the
> >>>>>> address type is set to LE public, leading to a failed connection, and
> >>>>>> vice versa.
> >>>>>
> >>>>> The correct address type can only be the ones listed on mgmt-api
> >>>>> otherwise we are breaking APIs, also except if there are some new case
> >>>>> I don't know and Load Keys and Load LTK are bearer specific.
> >>>>>
> >>>>>> We should enable bluetoothd to know the reason for the generation of
> >>>>>> keys as much as possible. Older kernels always assume the address type
> >>>>>> of linkey is BREDR, and ltk/irk/csrk are LE public. Therefore, we can
> >>>>>> use this as a basis for judgment, allowing bluetoothd to handle various
> >>>>>> issues, such as reloading keys and handling unpaired devices. Below is a
> >>>>>> pseudo-code that is not running in practice but just a thought. Please
> >>>>>> help review it.
> >>>>>
> >>>>> Yeah, and what exactly was wrong with that? What you mention about
> >>>>> having keys for both BR/EDR and LE public is not an issue, that
> >>>>> basically the very definition of a dual-mode device and bluetoothd
> >>>>> shall be able to tell when to connect to BR/EDR and LE public, if it
> >>>>> doesn't we shall fix that and not start breaking APIs.
> >>>> Returning to my original issue: When BlueZ acts as a sink device and a
> >>>> phone connects to it, pairs successfully, and derives related keys,
> >>>> BlueZ reports to the user layer that bonded (both bredr_state.bonded and
> >>>> dev->le_state.bonded) are true, and addr_type is LE_PUBLIC. If the
> >>>> device restarts and the org.bluez.Device1:Connect method is called to
> >>>> reconnect to the phone, according to the rules in select_conn_bearer,
> >>>> addr_type will be set to LE_PUBLIC, leading to a failed reconnection
> >>>> when device_connect_le is called.
> >>>
> >>> If I recall this correctly we do reconnect based on the last seen
> >>> bearer, also the addr_type is only really relevant for LE bearer,
> >>> BR/EDR shall be detected via bearer state alone. Anyway there was a
> >>> discussion regarding having bearer specific API's as alternative to
> >>> org.bluez.Device1.Connect to pickup a bearer, but in some cases, like
> >>> when acting as a peripheral, perhaps we need a different policy, in
> >>> the specific case of being a A2DP sink though, we could detect that LE
> >>> keys are for a peripheral and in that case advertisement shall be used
> >>> for connecting the LE bearer thus the BR/EDR shall be chosen which
> >>> shall fix this behavior.
> >>>
> >>>> In this situation, how should bluetoothd choose the correct address type
> >>>> when it doesn't know the physical bearer layer? Should there be a
> >>>> configuration option to turn key derivation on or off? Additionally,
> >>>> there's another use case: When a phone connects to a BlueZ device, it
> >>>> continuously transmits custom data via BLE and music via traditional
> >>>> Bluetooth (BREDR). If the user only wants to unpair the BREDR, it may
> >>>> cause the BLE connection to be disconnected as well, leading to data
> >>>> transmission interruption. Should we also consider turning off key
> >>>> derivation in this scenario?
> >>>
> >>> Device API is not bearer specific though, so one cannot really unpair
> >>> BR/EDR alone, in fact I would be surprised if any stack implements
> >>> this that way because depending on what bearer is connected it may
> >>> trigger a missing link key which could possibly lead to repairing
> >>> procedure which will most likely having to replace the other bearer
> >>> keys as well, if the idea is you only really need BR/EDR then you
> >>> might as well disable LE completely.
> >>
> >> Could we possibly implement a configuration that allows for the enabling
> >> or disabling of key derivation? Currently, a significant portion of
> >> devices only require either BR/EDR or BLE functionality independently
> >> and do not need key derivation, which can cause some confusion and
> >> issues for users. Additionally, as I understand, there are some
> >> proprietary RTOS Bluetooth protocol stacks that have disabled key
> >> derivation and support separate management of pairing keys for BR/EDR
> >> and BLE devices. For example, an IoT device that supports both BR/EDR
> >> and BLE could be detected as two separate devices by an iPhone, sharing
> >> the same address—one for BLE used for ANCS notifications (Apple
> >> Notification Center Service (ANCS) Specification) and the other for
> >> A2DP. The phone could connect to or remove either device without
> >> interference between the two.
> >
> > That will likely require a lot of work to get working since we would
> > endup with the same device object, if this really exists in real life
> > Im afraid it breaks a lot of things in the process and it will never
> > be backwards compatible, so I'm really not convinced this is the right
> > thing to do, in fact Id claim that if a device wants to have separate
> > pairing/bonding procedures then it must use different addresses which
> > means the LE bearer shall probably use a random static address rather
> > than attempt to reuse the same identity address of BR/EDR, afterall it
> > want to have distinct devices so it really makes no sense to keep
> > using the same address.
>
> BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 6, Part B
> 1.3 DEVICE ADDRESS
> Devices are identified using a device address and an address type;
> Whenever two device addresses are compared, the comparison shall include
> the device address type (i.e. if the two addresses have different types,
> they are different even if the two 48-bit addresses are the same).

Yeah, but it doesn't make any difference if both BREDR and LE address
type is a public identity address, which must be the case since BREDR
can only be a public address, or are you talking about giving a random
static the same address to a public? While possible I would argue that
is a self inflicted pain because the device should attempt to generate
a unique address exactly for the purpose of not allocating addresses
which may belong to another company (OUI):

https://www.wireshark.org/tools/oui-lookup.html

And no, as far as MAC address is concerned there is no address type,
this is something that Bluetooth invented and that I very much dislike
since it completely disregards things like OUI exists.

> According to the specification, even if the addresses are the same, as
> long as the address types are different, they are considered two
> different devices; the iPhone(iOS) can scan for two devices: they have
> the same address but different address types, and can be paired and
> unpaired independently.

That is fair enough when the devices are from different manufacturers,
what I'm questioning is why someone would deliberately have the same
address with different address types? Btw, are these devices on the
market already?

>
> >
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> -------------------------------------------
> >>>>>> diff --git a/src/adapter.c b/src/adapter.c
> >>>>>> index 85ddfc165..babe7c9b2 100644
> >>>>>> --- a/src/adapter.c
> >>>>>> +++ b/src/adapter.c
> >>>>>> @@ -4958,6 +4958,7 @@ static void load_devices(struct btd_adapter *adapter)
> >>>>>>              struct irk_info *irk_info;
> >>>>>>              struct conn_param *param;
> >>>>>>              uint8_t bdaddr_type;
> >>>>>> +        bool derived_key;
> >>>>>>
> >>>>>>              if (entry->d_type == DT_UNKNOWN)
> >>>>>>                  entry->d_type = util_get_dt(dirname, entry->d_name);
> >>>>>> @@ -4976,10 +4977,21 @@ static void load_devices(struct btd_adapter
> >>>>>> *adapter)
> >>>>>>                  g_clear_error(&gerr);
> >>>>>>              }
> >>>>>>
> >>>>>> +        derived_key = g_key_file_get_boolean(key_file, "General",
> >>>>>> +                                "DerivedKey", NULL);
> >>>>>> +
> >>>>>> +        /* For link key */
> >>>>>>              bdaddr_type = get_addr_type(key_file);
> >>>>>> +        if (!derived_key)
> >>>>>> +            bdaddr_type = BDADDR_BREDR;
> >>>>>>
> >>>>>>              key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
> >>>>>>
> >>>>>> +        /* For ltk/irk/csrk */
> >>>>>> +        bdaddr_type = get_addr_type(key_file);
> >>>>>> +        if (!derived_key)
> >>>>>> +            bdaddr_type = BDADDR_LE_PUBLIC;
> >>>>>> +
> >>>>>>              ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> >>>>>>
> >>>>>>              peripheral_ltk_info = get_peripheral_ltk_info(key_file,
> >>>>>> @@ -8673,6 +8685,12 @@ static void new_link_key_callback(uint16_t index,
> >>>>>> uint16_t length,
> >>>>>>              return;
> >>>>>>          }
> >>>>>>
> >>>>>> +    /*
> >>>>>> +     * For older kernels, the address type is always BREDR.
> >>>>>> +     */
> >>>>>> +    if (addr->type == BDADDR_LE_PUBLIC)
> >>>>>> +        device->derived_key = true;
> >>>>>> +
> >>>>>>          if (ev->store_hint) {
> >>>>>>              const struct mgmt_link_key_info *key = &ev->key;
> >>>>>>
> >>>>>> @@ -8791,6 +8809,12 @@ static void new_long_term_key_callback(uint16_t
> >>>>>> index, uint16_t length,
> >>>>>>              return;
> >>>>>>          }
> >>>>>>
> >>>>>> +    /*
> >>>>>> +     * For older kernels, the address type is always BDADDR_LE_PUBLIC.
> >>>>>> +     */
> >>>>>> +    if (addr->type == BDADDR_BREDR)
> >>>>>> +        device->derived_key = true;
> >>>>>> +
> >>>>>>          /*
> >>>>>>           * Some older kernel versions set store_hint for long term keys
> >>>>>>           * from resolvable and unresolvable random addresses, but there
> >>>>>> @@ -8855,6 +8879,9 @@ static void new_csrk_callback(uint16_t index,
> >>>>>> uint16_t length,
> >>>>>>              return;
> >>>>>>          }
> >>>>>>
> >>>>>> +    if (addr->type == BDADDR_BREDR)
> >>>>>> +        device->derived_key = true;
> >>>>>> +
> >>>>>>          device_set_csrk(device, key->val, 0, key->type, ev->store_hint);
> >>>>>>      }
> >>>>>>
> >>>>>> @@ -8941,6 +8968,9 @@ static void new_irk_callback(uint16_t index,
> >>>>>> uint16_t length,
> >>>>>>              return;
> >>>>>>          }
> >>>>>>
> >>>>>> +    if (addr->type == BDADDR_BREDR)
> >>>>>> +        device->derived_key = true;
> >>>>>> +
> >>>>>>          device_update_addr(device, &addr->bdaddr, addr->type);
> >>>>>>
> >>>>>>          if (duplicate)
> >>>>>> diff --git a/src/device.c b/src/device.c
> >>>>>> index a1dc0750c..062b9c49d 100644
> >>>>>> --- a/src/device.c
> >>>>>> +++ b/src/device.c
> >>>>>> @@ -272,6 +272,7 @@ struct btd_device {
> >>>>>>          struct csrk_info *remote_csrk;
> >>>>>>          struct ltk_info *ltk;
> >>>>>>          struct queue    *sirks;
> >>>>>> +    bool        derived_key;            /* key is derived. */
> >>>>>>
> >>>>>>          sdp_list_t    *tmp_records;
> >>>>>>
> >>>>>> @@ -482,6 +483,9 @@ static gboolean store_device_info_cb(gpointer user_data)
> >>>>>>          g_key_file_set_boolean(key_file, "General", "Blocked",
> >>>>>>                                  device->blocked);
> >>>>>>
> >>>>>> +    g_key_file_set_boolean(key_file, "General", "DerivedKey",
> >>>>>> +                            device->derived_key);
> >>>>>> +
> >>>>>>          if (device->wake_override != WAKE_FLAG_DEFAULT) {
> >>>>>>              g_key_file_set_boolean(key_file, "General", "WakeAllowed",
> >>>>>>                             device->wake_override ==
> >>>>>> @@ -1829,7 +1833,11 @@ static void bonding_request_cancel(struct
> >>>>>> bonding_req *bonding)
> >>>>>>          struct btd_device *device = bonding->device;
> >>>>>>          struct btd_adapter *adapter = device->adapter;
> >>>>>>
> >>>>>> -    adapter_cancel_bonding(adapter, &device->bdaddr, device->bdaddr_type);
> >>>>>> +    if (device->derived_key) {
> >>>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_BREDR);
> >>>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_LE_PUBLIC);
> >>>>>> +    } else
> >>>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr,
> >>>>>> device->bdaddr_type);
> >>>>>>      }
> >>>>>>
> >>>>>>      static void dev_disconn_service(gpointer a, gpointer b)
> >>>>>> @@ -3196,12 +3204,19 @@ static DBusMessage
> >>>>>> *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
> >>>>>>      {
> >>>>>>          struct btd_device *device = data;
> >>>>>>          struct bonding_req *req = device->bonding;
> >>>>>> +    uint8_t bdaddr_type;
> >>>>>>
> >>>>>>          DBG("");
> >>>>>>
> >>>>>>          if (!req) {
> >>>>>> -        btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >>>>>> -                        device->bdaddr_type);
> >>>>>> +        if (device->derived_key) {
> >>>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >>>>>> +                            BDADDR_BREDR);
> >>>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >>>>>> +                            BDADDR_LE_PUBLIC);
> >>>>>> +        } else
> >>>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> >>>>>> +                            device->bdaddr_type);
> >>>>>>              return btd_error_does_not_exist(msg);
> >>>>>>          }
> >>>>>>
> >>>>>> @@ -3833,6 +3848,9 @@ next:
> >>>>>>              gerr = NULL;
> >>>>>>          }
> >>>>>>
> >>>>>> +    device->derived_key = g_key_file_get_boolean(key_file, "General",
> >>>>>> +                            "DerivedKey", NULL);
> >>>>>> +
> >>>>>>          if (store_needed)
> >>>>>>              store_device_info(device);
> >>>>>>      }
> >>>>>>
> >>>>>> On 2024/8/27 5:12, Luiz Augusto von Dentz wrote:
> >>>>>>> Hi Xiao,
> >>>>>>>
> >>>>>>> On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
> >>>>>>>>
> >>>>>>>> On 12/11/23 17:27, Xiao Yao wrote:
> >>>>>>>>
> >>>>>>>> From: Xiao Yao <[e-mail]>
> >>>>>>>>
> >>>>>>>> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
> >>>>>>>> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
> >>>>>>>> converted between each other, so the addr type can be either
> >>>>>>>> BREDR or LE, so fix it.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Xiao Yao <[e-mail]>
> >>>>>>>> ---
> >>>>>>>>      src/adapter.c | 20 ++++++++++++++------
> >>>>>>>>      1 file changed, 14 insertions(+), 6 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/src/adapter.c b/src/adapter.c
> >>>>>>>> index 86fff72bc..ee70b00d2 100644
> >>>>>>>> --- a/src/adapter.c
> >>>>>>>> +++ b/src/adapter.c
> >>>>>>>> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
> >>>>>>>>
> >>>>>>>>      struct link_key_info {
> >>>>>>>>       bdaddr_t bdaddr;
> >>>>>>>> + uint8_t bdaddr_type;
> >>>>>>>>       unsigned char key[16];
> >>>>>>>>       uint8_t type;
> >>>>>>>>       uint8_t pin_len;
> >>>>>>>> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> >>>>>>>>       return false;
> >>>>>>>>      }
> >>>>>>>>
> >>>>>>>> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >>>>>>>> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
> >>>>>>>> + uint8_t bdaddr_type)
> >>>>>>>> +
> >>>>>>>>      {
> >>>>>>>>       struct link_key_info *info = NULL;
> >>>>>>>>       char *str;
> >>>>>>>> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >>>>>>>>       info = g_new0(struct link_key_info, 1);
> >>>>>>>>
> >>>>>>>>       str2ba(peer, &info->bdaddr);
> >>>>>>>> + info->bdaddr_type = bdaddr_type;
> >>>>>>>>
> >>>>>>>>       if (!strncmp(str, "0x", 2))
> >>>>>>>>       str2buf(&str[2], info->key, sizeof(info->key));
> >>>>>>>> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
> >>>>>>>>       struct link_key_info *info = l->data;
> >>>>>>>>
> >>>>>>>>       bacpy(&key->addr.bdaddr, &info->bdaddr);
> >>>>>>>> - key->addr.type = BDADDR_BREDR;
> >>>>>>>> + key->addr.type = info->bdaddr_type;
> >>>>>>>>       key->type = info->type;
> >>>>>>>>       memcpy(key->val, info->key, 16);
> >>>>>>>>       key->pin_len = info->pin_len;
> >>>>>>>> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
> >>>>>>>>       btd_error(adapter->dev_id, "Load connection parameters failed");
> >>>>>>>>      }
> >>>>>>>>
> >>>>>>>> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
> >>>>>>>> +static uint8_t get_addr_type(GKeyFile *keyfile)
> >>>>>>>>      {
> >>>>>>>>       uint8_t addr_type;
> >>>>>>>>       char *type;
> >>>>>>>>
> >>>>>>>> + /* The AddressType is written to file only When dev->le is
> >>>>>>>> + * set to true, as referenced in the update_technologies().
> >>>>>>>> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
> >>>>>>>> + */
> >>>>>>>>       type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
> >>>>>>>>       if (!type)
> >>>>>>>> - return BDADDR_LE_PUBLIC;
> >>>>>>>> + return BDADDR_BREDR;
> >>>>>>>>
> >>>>>>>>       if (g_str_equal(type, "public"))
> >>>>>>>>       addr_type = BDADDR_LE_PUBLIC;
> >>>>>>>> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
> >>>>>>>>       g_clear_error(&gerr);
> >>>>>>>>       }
> >>>>>>>>
> >>>>>>>> - key_info = get_key_info(key_file, entry->d_name);
> >>>>>>>> + bdaddr_type = get_addr_type(key_file);
> >>>>>>>>
> >>>>>>>> - bdaddr_type = get_le_addr_type(key_file);
> >>>>>>>> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
> >>>>>>>>
> >>>>>>>>       ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Hello,
> >>>>>>>>
> >>>>>>>> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
> >>>>>>>>
> >>>>>>>> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
> >>>>>>>>
> >>>>>>>> `systemctl status bluetooth` reports the following error:
> >>>>>>>>
> >>>>>>>> [...]
> >>>>>>>> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
> >>>>>>>> [...]
> >>>>>>>>
> >>>>>>>> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
> >>>>>>>> I can confirm that reverting it solves the issue.
> >>>>>>>>
> >>>>>>>> I reported this bug including details in the GitHub repo [2].
> >>>>>>>>
> >>>>>>>> I remain available if any additional information are needed.
> >>>>>>>>
> >>>>>>>> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
> >>>>>>>> [2] https://github.com/bluez/bluez/issues/686
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> Regards,
> >>>>>>>> Robin Candau / Antiz
> >>>>>>>
> >>>>>>> Perhaps related to:
> >>>>>>> https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>
> >
> >
>

Yao Xiao Aug. 29, 2024, 1:27 p.m. UTC | #13

Hi,

On 2024/8/28 22:37, Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Wed, Aug 28, 2024 at 6:03 AM Yao Xiao <[e-mail]> wrote:
>>
>> Hi Luiz,
>>
>> On 2024/8/28 2:30, Luiz Augusto von Dentz wrote:
>>> Hi Yao,
>>>
>>> On Tue, Aug 27, 2024 at 2:03 PM Yao Xiao <[e-mail]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 2024/8/27 23:47, Luiz Augusto von Dentz wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Aug 27, 2024 at 10:33 AM Yao Xiao <[e-mail]> wrote:
>>>>>>
>>>>>> Hi Luiz,
>>>>>>
>>>>>> On 2024/8/27 20:52, Luiz Augusto von Dentz wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, Aug 27, 2024 at 3:41 AM Yao Xiao <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> Hi luiz,
>>>>>>>> The kernel should always report the correct address type for all kinds
>>>>>>>> of keys. Older kernels ignore the derivation of keys, which prevents the
>>>>>>>> user layer from knowing the correct address type. This can cause a lot
>>>>>>>> of confusion for users. For example, when a user connects to a
>>>>>>>> traditional Bluetooth audio device, due to the default activation of key
>>>>>>>> derivation, bluetoothd reports both BREDR and BLE pairing and binding,
>>>>>>>> and the address is updated to LE public. When the user actively calls
>>>>>>>> the Connect method of org.bluez.Device1 to reconnect, sometimes the
>>>>>>>> address type is set to LE public, leading to a failed connection, and
>>>>>>>> vice versa.
>>>>>>>
>>>>>>> The correct address type can only be the ones listed on mgmt-api
>>>>>>> otherwise we are breaking APIs, also except if there are some new case
>>>>>>> I don't know and Load Keys and Load LTK are bearer specific.
>>>>>>>
>>>>>>>> We should enable bluetoothd to know the reason for the generation of
>>>>>>>> keys as much as possible. Older kernels always assume the address type
>>>>>>>> of linkey is BREDR, and ltk/irk/csrk are LE public. Therefore, we can
>>>>>>>> use this as a basis for judgment, allowing bluetoothd to handle various
>>>>>>>> issues, such as reloading keys and handling unpaired devices. Below is a
>>>>>>>> pseudo-code that is not running in practice but just a thought. Please
>>>>>>>> help review it.
>>>>>>>
>>>>>>> Yeah, and what exactly was wrong with that? What you mention about
>>>>>>> having keys for both BR/EDR and LE public is not an issue, that
>>>>>>> basically the very definition of a dual-mode device and bluetoothd
>>>>>>> shall be able to tell when to connect to BR/EDR and LE public, if it
>>>>>>> doesn't we shall fix that and not start breaking APIs.
>>>>>> Returning to my original issue: When BlueZ acts as a sink device and a
>>>>>> phone connects to it, pairs successfully, and derives related keys,
>>>>>> BlueZ reports to the user layer that bonded (both bredr_state.bonded and
>>>>>> dev->le_state.bonded) are true, and addr_type is LE_PUBLIC. If the
>>>>>> device restarts and the org.bluez.Device1:Connect method is called to
>>>>>> reconnect to the phone, according to the rules in select_conn_bearer,
>>>>>> addr_type will be set to LE_PUBLIC, leading to a failed reconnection
>>>>>> when device_connect_le is called.
>>>>>
>>>>> If I recall this correctly we do reconnect based on the last seen
>>>>> bearer, also the addr_type is only really relevant for LE bearer,
>>>>> BR/EDR shall be detected via bearer state alone. Anyway there was a
>>>>> discussion regarding having bearer specific API's as alternative to
>>>>> org.bluez.Device1.Connect to pickup a bearer, but in some cases, like
>>>>> when acting as a peripheral, perhaps we need a different policy, in
>>>>> the specific case of being a A2DP sink though, we could detect that LE
>>>>> keys are for a peripheral and in that case advertisement shall be used
>>>>> for connecting the LE bearer thus the BR/EDR shall be chosen which
>>>>> shall fix this behavior.
>>>>>
>>>>>> In this situation, how should bluetoothd choose the correct address type
>>>>>> when it doesn't know the physical bearer layer? Should there be a
>>>>>> configuration option to turn key derivation on or off? Additionally,
>>>>>> there's another use case: When a phone connects to a BlueZ device, it
>>>>>> continuously transmits custom data via BLE and music via traditional
>>>>>> Bluetooth (BREDR). If the user only wants to unpair the BREDR, it may
>>>>>> cause the BLE connection to be disconnected as well, leading to data
>>>>>> transmission interruption. Should we also consider turning off key
>>>>>> derivation in this scenario?
>>>>>
>>>>> Device API is not bearer specific though, so one cannot really unpair
>>>>> BR/EDR alone, in fact I would be surprised if any stack implements
>>>>> this that way because depending on what bearer is connected it may
>>>>> trigger a missing link key which could possibly lead to repairing
>>>>> procedure which will most likely having to replace the other bearer
>>>>> keys as well, if the idea is you only really need BR/EDR then you
>>>>> might as well disable LE completely.
>>>>
>>>> Could we possibly implement a configuration that allows for the enabling
>>>> or disabling of key derivation? Currently, a significant portion of
>>>> devices only require either BR/EDR or BLE functionality independently
>>>> and do not need key derivation, which can cause some confusion and
>>>> issues for users. Additionally, as I understand, there are some
>>>> proprietary RTOS Bluetooth protocol stacks that have disabled key
>>>> derivation and support separate management of pairing keys for BR/EDR
>>>> and BLE devices. For example, an IoT device that supports both BR/EDR
>>>> and BLE could be detected as two separate devices by an iPhone, sharing
>>>> the same address—one for BLE used for ANCS notifications (Apple
>>>> Notification Center Service (ANCS) Specification) and the other for
>>>> A2DP. The phone could connect to or remove either device without
>>>> interference between the two.
>>>
>>> That will likely require a lot of work to get working since we would
>>> endup with the same device object, if this really exists in real life
>>> Im afraid it breaks a lot of things in the process and it will never
>>> be backwards compatible, so I'm really not convinced this is the right
>>> thing to do, in fact Id claim that if a device wants to have separate
>>> pairing/bonding procedures then it must use different addresses which
>>> means the LE bearer shall probably use a random static address rather
>>> than attempt to reuse the same identity address of BR/EDR, afterall it
>>> want to have distinct devices so it really makes no sense to keep
>>> using the same address.
>>
>> BLUETOOTH CORE SPECIFICATION Version 5.2 | Vol 6, Part B
>> 1.3 DEVICE ADDRESS
>> Devices are identified using a device address and an address type;
>> Whenever two device addresses are compared, the comparison shall include
>> the device address type (i.e. if the two addresses have different types,
>> they are different even if the two 48-bit addresses are the same).
> 
> Yeah, but it doesn't make any difference if both BREDR and LE address
> type is a public identity address, which must be the case since BREDR
> can only be a public address, or are you talking about giving a random
> static the same address to a public? While possible I would argue that
> is a self inflicted pain because the device should attempt to generate
> a unique address exactly for the purpose of not allocating addresses
> which may belong to another company (OUI):
> 
> https://www.wireshark.org/tools/oui-lookup.html
> 
> And no, as far as MAC address is concerned there is no address type,
> this is something that Bluetooth invented and that I very much dislike
> since it completely disregards things like OUI exists.

I fully agree.

> 
>> According to the specification, even if the addresses are the same, as
>> long as the address types are different, they are considered two
>> different devices; the iPhone(iOS) can scan for two devices: they have
>> the same address but different address types, and can be paired and
>> unpaired independently.
> 
> That is fair enough when the devices are from different manufacturers,
> what I'm questioning is why someone would deliberately have the same
> address with different address types? Btw, are these devices on the
> market already?

This type of product has indeed been launched on the market. Based on my 
tests, the own addr type bit in the broadcast of the peripheral device 
is set to 0x01, which corresponds to a Random Device address, and the 
address remains consistent with the BREDR address.
When I modify the own addr type in the broadcast of the peripheral 
device to 0, the iPhone (iOS) will then treat them as one device.

Own_Address_Type
Value Parameter Description
0x00 Public Device Address (default)
0x01 Random Device Address
0x02 Controller generates Resolvable ...
0x03 Controller generates Resolvable ...

> 
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> -------------------------------------------
>>>>>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>>>>>> index 85ddfc165..babe7c9b2 100644
>>>>>>>> --- a/src/adapter.c
>>>>>>>> +++ b/src/adapter.c
>>>>>>>> @@ -4958,6 +4958,7 @@ static void load_devices(struct btd_adapter *adapter)
>>>>>>>>               struct irk_info *irk_info;
>>>>>>>>               struct conn_param *param;
>>>>>>>>               uint8_t bdaddr_type;
>>>>>>>> +        bool derived_key;
>>>>>>>>
>>>>>>>>               if (entry->d_type == DT_UNKNOWN)
>>>>>>>>                   entry->d_type = util_get_dt(dirname, entry->d_name);
>>>>>>>> @@ -4976,10 +4977,21 @@ static void load_devices(struct btd_adapter
>>>>>>>> *adapter)
>>>>>>>>                   g_clear_error(&gerr);
>>>>>>>>               }
>>>>>>>>
>>>>>>>> +        derived_key = g_key_file_get_boolean(key_file, "General",
>>>>>>>> +                                "DerivedKey", NULL);
>>>>>>>> +
>>>>>>>> +        /* For link key */
>>>>>>>>               bdaddr_type = get_addr_type(key_file);
>>>>>>>> +        if (!derived_key)
>>>>>>>> +            bdaddr_type = BDADDR_BREDR;
>>>>>>>>
>>>>>>>>               key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>>>>>>>>
>>>>>>>> +        /* For ltk/irk/csrk */
>>>>>>>> +        bdaddr_type = get_addr_type(key_file);
>>>>>>>> +        if (!derived_key)
>>>>>>>> +            bdaddr_type = BDADDR_LE_PUBLIC;
>>>>>>>> +
>>>>>>>>               ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>>>>>>>>
>>>>>>>>               peripheral_ltk_info = get_peripheral_ltk_info(key_file,
>>>>>>>> @@ -8673,6 +8685,12 @@ static void new_link_key_callback(uint16_t index,
>>>>>>>> uint16_t length,
>>>>>>>>               return;
>>>>>>>>           }
>>>>>>>>
>>>>>>>> +    /*
>>>>>>>> +     * For older kernels, the address type is always BREDR.
>>>>>>>> +     */
>>>>>>>> +    if (addr->type == BDADDR_LE_PUBLIC)
>>>>>>>> +        device->derived_key = true;
>>>>>>>> +
>>>>>>>>           if (ev->store_hint) {
>>>>>>>>               const struct mgmt_link_key_info *key = &ev->key;
>>>>>>>>
>>>>>>>> @@ -8791,6 +8809,12 @@ static void new_long_term_key_callback(uint16_t
>>>>>>>> index, uint16_t length,
>>>>>>>>               return;
>>>>>>>>           }
>>>>>>>>
>>>>>>>> +    /*
>>>>>>>> +     * For older kernels, the address type is always BDADDR_LE_PUBLIC.
>>>>>>>> +     */
>>>>>>>> +    if (addr->type == BDADDR_BREDR)
>>>>>>>> +        device->derived_key = true;
>>>>>>>> +
>>>>>>>>           /*
>>>>>>>>            * Some older kernel versions set store_hint for long term keys
>>>>>>>>            * from resolvable and unresolvable random addresses, but there
>>>>>>>> @@ -8855,6 +8879,9 @@ static void new_csrk_callback(uint16_t index,
>>>>>>>> uint16_t length,
>>>>>>>>               return;
>>>>>>>>           }
>>>>>>>>
>>>>>>>> +    if (addr->type == BDADDR_BREDR)
>>>>>>>> +        device->derived_key = true;
>>>>>>>> +
>>>>>>>>           device_set_csrk(device, key->val, 0, key->type, ev->store_hint);
>>>>>>>>       }
>>>>>>>>
>>>>>>>> @@ -8941,6 +8968,9 @@ static void new_irk_callback(uint16_t index,
>>>>>>>> uint16_t length,
>>>>>>>>               return;
>>>>>>>>           }
>>>>>>>>
>>>>>>>> +    if (addr->type == BDADDR_BREDR)
>>>>>>>> +        device->derived_key = true;
>>>>>>>> +
>>>>>>>>           device_update_addr(device, &addr->bdaddr, addr->type);
>>>>>>>>
>>>>>>>>           if (duplicate)
>>>>>>>> diff --git a/src/device.c b/src/device.c
>>>>>>>> index a1dc0750c..062b9c49d 100644
>>>>>>>> --- a/src/device.c
>>>>>>>> +++ b/src/device.c
>>>>>>>> @@ -272,6 +272,7 @@ struct btd_device {
>>>>>>>>           struct csrk_info *remote_csrk;
>>>>>>>>           struct ltk_info *ltk;
>>>>>>>>           struct queue    *sirks;
>>>>>>>> +    bool        derived_key;            /* key is derived. */
>>>>>>>>
>>>>>>>>           sdp_list_t    *tmp_records;
>>>>>>>>
>>>>>>>> @@ -482,6 +483,9 @@ static gboolean store_device_info_cb(gpointer user_data)
>>>>>>>>           g_key_file_set_boolean(key_file, "General", "Blocked",
>>>>>>>>                                   device->blocked);
>>>>>>>>
>>>>>>>> +    g_key_file_set_boolean(key_file, "General", "DerivedKey",
>>>>>>>> +                            device->derived_key);
>>>>>>>> +
>>>>>>>>           if (device->wake_override != WAKE_FLAG_DEFAULT) {
>>>>>>>>               g_key_file_set_boolean(key_file, "General", "WakeAllowed",
>>>>>>>>                              device->wake_override ==
>>>>>>>> @@ -1829,7 +1833,11 @@ static void bonding_request_cancel(struct
>>>>>>>> bonding_req *bonding)
>>>>>>>>           struct btd_device *device = bonding->device;
>>>>>>>>           struct btd_adapter *adapter = device->adapter;
>>>>>>>>
>>>>>>>> -    adapter_cancel_bonding(adapter, &device->bdaddr, device->bdaddr_type);
>>>>>>>> +    if (device->derived_key) {
>>>>>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_BREDR);
>>>>>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr, BDADDR_LE_PUBLIC);
>>>>>>>> +    } else
>>>>>>>> +        adapter_cancel_bonding(adapter, &device->bdaddr,
>>>>>>>> device->bdaddr_type);
>>>>>>>>       }
>>>>>>>>
>>>>>>>>       static void dev_disconn_service(gpointer a, gpointer b)
>>>>>>>> @@ -3196,12 +3204,19 @@ static DBusMessage
>>>>>>>> *cancel_pairing(DBusConnection *conn, DBusMessage *msg,
>>>>>>>>       {
>>>>>>>>           struct btd_device *device = data;
>>>>>>>>           struct bonding_req *req = device->bonding;
>>>>>>>> +    uint8_t bdaddr_type;
>>>>>>>>
>>>>>>>>           DBG("");
>>>>>>>>
>>>>>>>>           if (!req) {
>>>>>>>> -        btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>>>>>> -                        device->bdaddr_type);
>>>>>>>> +        if (device->derived_key) {
>>>>>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>>>>>> +                            BDADDR_BREDR);
>>>>>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>>>>>> +                            BDADDR_LE_PUBLIC);
>>>>>>>> +        } else
>>>>>>>> +            btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
>>>>>>>> +                            device->bdaddr_type);
>>>>>>>>               return btd_error_does_not_exist(msg);
>>>>>>>>           }
>>>>>>>>
>>>>>>>> @@ -3833,6 +3848,9 @@ next:
>>>>>>>>               gerr = NULL;
>>>>>>>>           }
>>>>>>>>
>>>>>>>> +    device->derived_key = g_key_file_get_boolean(key_file, "General",
>>>>>>>> +                            "DerivedKey", NULL);
>>>>>>>> +
>>>>>>>>           if (store_needed)
>>>>>>>>               store_device_info(device);
>>>>>>>>       }
>>>>>>>>
>>>>>>>> On 2024/8/27 5:12, Luiz Augusto von Dentz wrote:
>>>>>>>>> Hi Xiao,
>>>>>>>>>
>>>>>>>>> On Fri, Dec 15, 2023 at 8:06 PM Robin Candau <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> On 12/11/23 17:27, Xiao Yao wrote:
>>>>>>>>>>
>>>>>>>>>> From: Xiao Yao <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> According to BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 3,
>>>>>>>>>> Part H, 2.4.24/2.4.25, The LE LTK and BR/EDR link keys can be
>>>>>>>>>> converted between each other, so the addr type can be either
>>>>>>>>>> BREDR or LE, so fix it.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Xiao Yao <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>       src/adapter.c | 20 ++++++++++++++------
>>>>>>>>>>       1 file changed, 14 insertions(+), 6 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>>>>>>>> index 86fff72bc..ee70b00d2 100644
>>>>>>>>>> --- a/src/adapter.c
>>>>>>>>>> +++ b/src/adapter.c
>>>>>>>>>> @@ -170,6 +170,7 @@ static GSList *conn_fail_list = NULL;
>>>>>>>>>>
>>>>>>>>>>       struct link_key_info {
>>>>>>>>>>        bdaddr_t bdaddr;
>>>>>>>>>> + uint8_t bdaddr_type;
>>>>>>>>>>        unsigned char key[16];
>>>>>>>>>>        uint8_t type;
>>>>>>>>>>        uint8_t pin_len;
>>>>>>>>>> @@ -3964,7 +3965,9 @@ static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
>>>>>>>>>>        return false;
>>>>>>>>>>       }
>>>>>>>>>>
>>>>>>>>>> -static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>>>>>>>>>> +static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
>>>>>>>>>> + uint8_t bdaddr_type)
>>>>>>>>>> +
>>>>>>>>>>       {
>>>>>>>>>>        struct link_key_info *info = NULL;
>>>>>>>>>>        char *str;
>>>>>>>>>> @@ -3976,6 +3979,7 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>>>>>>>>>>        info = g_new0(struct link_key_info, 1);
>>>>>>>>>>
>>>>>>>>>>        str2ba(peer, &info->bdaddr);
>>>>>>>>>> + info->bdaddr_type = bdaddr_type;
>>>>>>>>>>
>>>>>>>>>>        if (!strncmp(str, "0x", 2))
>>>>>>>>>>        str2buf(&str[2], info->key, sizeof(info->key));
>>>>>>>>>> @@ -4343,7 +4347,7 @@ static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
>>>>>>>>>>        struct link_key_info *info = l->data;
>>>>>>>>>>
>>>>>>>>>>        bacpy(&key->addr.bdaddr, &info->bdaddr);
>>>>>>>>>> - key->addr.type = BDADDR_BREDR;
>>>>>>>>>> + key->addr.type = info->bdaddr_type;
>>>>>>>>>>        key->type = info->type;
>>>>>>>>>>        memcpy(key->val, info->key, 16);
>>>>>>>>>>        key->pin_len = info->pin_len;
>>>>>>>>>> @@ -4598,14 +4602,18 @@ static void load_conn_params(struct btd_adapter *adapter, GSList *params)
>>>>>>>>>>        btd_error(adapter->dev_id, "Load connection parameters failed");
>>>>>>>>>>       }
>>>>>>>>>>
>>>>>>>>>> -static uint8_t get_le_addr_type(GKeyFile *keyfile)
>>>>>>>>>> +static uint8_t get_addr_type(GKeyFile *keyfile)
>>>>>>>>>>       {
>>>>>>>>>>        uint8_t addr_type;
>>>>>>>>>>        char *type;
>>>>>>>>>>
>>>>>>>>>> + /* The AddressType is written to file only When dev->le is
>>>>>>>>>> + * set to true, as referenced in the update_technologies().
>>>>>>>>>> + * Therefore, When type is NULL, it default to BDADDR_BREDR.
>>>>>>>>>> + */
>>>>>>>>>>        type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
>>>>>>>>>>        if (!type)
>>>>>>>>>> - return BDADDR_LE_PUBLIC;
>>>>>>>>>> + return BDADDR_BREDR;
>>>>>>>>>>
>>>>>>>>>>        if (g_str_equal(type, "public"))
>>>>>>>>>>        addr_type = BDADDR_LE_PUBLIC;
>>>>>>>>>> @@ -4914,9 +4922,9 @@ static void load_devices(struct btd_adapter *adapter)
>>>>>>>>>>        g_clear_error(&gerr);
>>>>>>>>>>        }
>>>>>>>>>>
>>>>>>>>>> - key_info = get_key_info(key_file, entry->d_name);
>>>>>>>>>> + bdaddr_type = get_addr_type(key_file);
>>>>>>>>>>
>>>>>>>>>> - bdaddr_type = get_le_addr_type(key_file);
>>>>>>>>>> + key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
>>>>>>>>>>
>>>>>>>>>>        ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> It seems like the above commit introduced a regression where the initial auto-connect for Bluetooth devices doesn't work anymore.
>>>>>>>>>>
>>>>>>>>>> Indeed, at system startup, starting a Bluetooth device will cause it to go in a "connected/disconnected" state loop, requiring to initialize a manual connection first (with sometimes multiple attempts needed) before getting it connected correctly and working as intended.
>>>>>>>>>>
>>>>>>>>>> `systemctl status bluetooth` reports the following error:
>>>>>>>>>>
>>>>>>>>>> [...]
>>>>>>>>>> déc. 15 11:03:18 Arch-Desktop bluetoothd[592]: Failed to load link keys for hci0: Invalid Parameters (0x0d)
>>>>>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>> I bisected the bug with `git bisect` and it pointed out this commit [1] as the "faulty" one.
>>>>>>>>>> I can confirm that reverting it solves the issue.
>>>>>>>>>>
>>>>>>>>>> I reported this bug including details in the GitHub repo [2].
>>>>>>>>>>
>>>>>>>>>> I remain available if any additional information are needed.
>>>>>>>>>>
>>>>>>>>>> [1] https://github.com/bluez/bluez/commit/d5536e0cd431e22be9a1132be6d4af2445590633
>>>>>>>>>> [2] https://github.com/bluez/bluez/issues/686
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Regards,
>>>>>>>>>> Robin Candau / Antiz
>>>>>>>>>
>>>>>>>>> Perhaps related to:
>>>>>>>>> https://github.com/bluez/bluez/issues/875#issuecomment-2311100872?
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
>

Patch

diff --git a/src/adapter.c b/src/adapter.c
index 86fff72bc..ee70b00d2 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -170,6 +170,7 @@  static GSList *conn_fail_list = NULL;
 
 struct link_key_info {
 	bdaddr_t bdaddr;
+	uint8_t bdaddr_type;
 	unsigned char key[16];
 	uint8_t type;
 	uint8_t pin_len;
@@ -3964,7 +3965,9 @@  static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
 	return false;
 }
 
-static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
+static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer,
+					uint8_t bdaddr_type)
+
 {
 	struct link_key_info *info = NULL;
 	char *str;
@@ -3976,6 +3979,7 @@  static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
 	info = g_new0(struct link_key_info, 1);
 
 	str2ba(peer, &info->bdaddr);
+	info->bdaddr_type = bdaddr_type;
 
 	if (!strncmp(str, "0x", 2))
 		str2buf(&str[2], info->key, sizeof(info->key));
@@ -4343,7 +4347,7 @@  static void load_link_keys(struct btd_adapter *adapter, GSList *keys,
 		struct link_key_info *info = l->data;
 
 		bacpy(&key->addr.bdaddr, &info->bdaddr);
-		key->addr.type = BDADDR_BREDR;
+		key->addr.type = info->bdaddr_type;
 		key->type = info->type;
 		memcpy(key->val, info->key, 16);
 		key->pin_len = info->pin_len;
@@ -4598,14 +4602,18 @@  static void load_conn_params(struct btd_adapter *adapter, GSList *params)
 		btd_error(adapter->dev_id, "Load connection parameters failed");
 }
 
-static uint8_t get_le_addr_type(GKeyFile *keyfile)
+static uint8_t get_addr_type(GKeyFile *keyfile)
 {
 	uint8_t addr_type;
 	char *type;
 
+	/* The AddressType is written to file only When dev->le is
+	 * set to true, as referenced in the update_technologies().
+	 * Therefore, When type is NULL, it default to BDADDR_BREDR.
+	 */
 	type = g_key_file_get_string(keyfile, "General", "AddressType", NULL);
 	if (!type)
-		return BDADDR_LE_PUBLIC;
+		return BDADDR_BREDR;
 
 	if (g_str_equal(type, "public"))
 		addr_type = BDADDR_LE_PUBLIC;
@@ -4914,9 +4922,9 @@  static void load_devices(struct btd_adapter *adapter)
 			g_clear_error(&gerr);
 		}
 
-		key_info = get_key_info(key_file, entry->d_name);
+		bdaddr_type = get_addr_type(key_file);
 
-		bdaddr_type = get_le_addr_type(key_file);
+		key_info = get_key_info(key_file, entry->d_name, bdaddr_type);
 
 		ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);