diff mbox series

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

Message ID 20231211162729.1183207-1-xiaokeqinhealth@126.com (mailing list archive)
State Accepted
Commit d5536e0cd431e22be9a1132be6d4af2445590633
Headers show
Series [1/2] adapter: Fix addr_type for smp_irk/ltk_info/link_key | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Felix Qin Dec. 11, 2023, 4:27 p.m. UTC 
From: Xiao Yao <xiaoyao@rock-chips.com>

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@rock-chips.com>
---
 src/adapter.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

bluez.test.bot@gmail.com 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
patchwork-bot+bluetooth@kernel.org Dec. 11, 2023, 6:40 p.m. UTC | #2 
Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Tue, 12 Dec 2023 00:27:28 +0800 you wrote:
> From: Xiao Yao <xiaoyao@rock-chips.com>
> 
> 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<xiaoyao@rock-chips.com>
>
> 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@rock-chips.com>
> ---
>   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|
|
diff mbox series

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