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 <antiz@xxxxxxxxxxxxx> wrote:
On 12/11/23 17:27, Xiao Yao wrote:
From: Xiao Yao <xiaoyao@xxxxxxxxxxxxxx>
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 <xiaoyao@xxxxxxxxxxxxxx>
---
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?
