2024-03-25 13:04:04

by Hui Wang

[permalink] [raw]
Subject: [PATCH] Bluetooth: hci_event: conn is already encryped before conn establishes

We have a BT headset (Lenovo Thinkplus XT99), the pairing and
connecting has no problem, once this headset is paired, bluez will
remember this device and will auto re-connect it whenever the device
is power on. The auto re-connecting works well with Windows and
Android, but with Linux, it always fails. Through debugging, we found
at the rfcomm connection stage, the bluetooth stack reports
"Connection refused - security block (0x0003)".

For this device, the re-connecting negotiation process is different
from other BT headsets, it sends the Link_KEY_REQUEST command before
the CONNECT_REQUEST completes, and it doesn't send ENCRYPT_CHANGE
command during the negotiation. When the device sends the "connect
complete" to hci, the ev->encr_mode is 1.

So here in the conn_complete_evt(), if ev->encr_mode is 1, link type
is ACL and HCI_CONN_ENCRYPT is not set, we set HCI_CONN_ENCRYPT to
this conn, and update conn->enc_key_size accordingly.

After this change, this BT headset could re-connect with Linux
successfully.

Signed-off-by: Hui Wang <[email protected]>
---
This is the btmon log for auto re-connecting negotiation:
Before applying this patch:
https://pastebin.com/NUa9RJv8

After applying the patch:
https://pastebin.com/GqviChTC

net/bluetooth/hci_event.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 4ae224824012..0c45beb08cb2 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3208,6 +3208,32 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
if (test_bit(HCI_ENCRYPT, &hdev->flags))
set_bit(HCI_CONN_ENCRYPT, &conn->flags);

+ /* "Link key request" completed ahead of "connect request" completes */
+ if (ev->encr_mode == 1 && !test_bit(HCI_CONN_ENCRYPT, &conn->flags) &&
+ ev->link_type == ACL_LINK) {
+ struct link_key *key;
+ struct hci_cp_read_enc_key_size cp;
+
+ key = hci_find_link_key(hdev, &ev->bdaddr);
+ if (key) {
+ set_bit(HCI_CONN_ENCRYPT, &conn->flags);
+
+ if (!(hdev->commands[20] & 0x10)) {
+ conn->enc_key_size = HCI_LINK_KEY_SIZE;
+ goto notify;
+ }
+
+ cp.handle = cpu_to_le16(conn->handle);
+ if (hci_send_cmd(hdev, HCI_OP_READ_ENC_KEY_SIZE,
+ sizeof(cp), &cp)) {
+ bt_dev_err(hdev, "sending read key size failed");
+ conn->enc_key_size = HCI_LINK_KEY_SIZE;
+ }
+notify:
+ hci_encrypt_cfm(conn, ev->status);
+ }
+ }
+
/* Get remote features */
if (conn->type == ACL_LINK) {
struct hci_cp_read_remote_features cp;
--
2.34.1



2024-03-25 13:58:05

by bluez.test.bot

[permalink] [raw]
Subject: RE: Bluetooth: hci_event: conn is already encryped before conn establishes

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

---Test result---

Test Summary:
CheckPatch PASS 0.67 seconds
GitLint PASS 0.32 seconds
SubjectPrefix PASS 0.13 seconds
BuildKernel PASS 26.60 seconds
CheckAllWarning PASS 29.09 seconds
CheckSparse WARNING 34.58 seconds
CheckSmatch WARNING 94.16 seconds
BuildKernel32 PASS 25.82 seconds
TestRunnerSetup PASS 485.43 seconds
TestRunner_l2cap-tester PASS 20.75 seconds
TestRunner_iso-tester PASS 29.18 seconds
TestRunner_bnep-tester PASS 4.50 seconds
TestRunner_mgmt-tester PASS 110.90 seconds
TestRunner_rfcomm-tester PASS 7.07 seconds
TestRunner_sco-tester PASS 14.61 seconds
TestRunner_ioctl-tester PASS 7.38 seconds
TestRunner_mesh-tester PASS 5.57 seconds
TestRunner_smp-tester PASS 6.55 seconds
TestRunner_userchan-tester PASS 4.73 seconds
IncrementalBuild PASS 24.78 seconds

Details
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):


---
Regards,
Linux Bluetooth

2024-03-25 16:24:11

by Paul Menzel

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_event: conn is already encryped before conn establishes

Dear Hui,


Thank you for your patch. Some minor nits from my side.

Regarding the summary (encryp*t*ed), please describe the action of the
change and not the issue.

Am 25.03.24 um 07:18 schrieb Hui Wang:
> We have a BT headset (Lenovo Thinkplus XT99), the pairing and
> connecting has no problem, once this headset is paired, bluez will
> remember this device and will auto re-connect it whenever the device
> is power on. The auto re-connecting works well with Windows and

power*ed*

> Android, but with Linux, it always fails. Through debugging, we found
> at the rfcomm connection stage, the bluetooth stack reports
> "Connection refused - security block (0x0003)".
>
> For this device, the re-connecting negotiation process is different
> from other BT headsets, it sends the Link_KEY_REQUEST command before
> the CONNECT_REQUEST completes, and it doesn't send ENCRYPT_CHANGE
> command during the negotiation. When the device sends the "connect
> complete" to hci, the ev->encr_mode is 1.

Is that in accordance with the specification or a violation?

> So here in the conn_complete_evt(), if ev->encr_mode is 1, link type
> is ACL and HCI_CONN_ENCRYPT is not set, we set HCI_CONN_ENCRYPT to
> this conn, and update conn->enc_key_size accordingly.
>
> After this change, this BT headset could re-connect with Linux
> successfully.

Despite this being a generic issue, could you please document with what
controller you tested this?

Do you have any bug/issue tracker URL, you can reference?

> Signed-off-by: Hui Wang <[email protected]>
> ---
> This is the btmon log for auto re-connecting negotiation:
> Before applying this patch:
> https://pastebin.com/NUa9RJv8
>
> After applying the patch:
> https://pastebin.com/GqviChTC
>
> net/bluetooth/hci_event.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 4ae224824012..0c45beb08cb2 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3208,6 +3208,32 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
> if (test_bit(HCI_ENCRYPT, &hdev->flags))
> set_bit(HCI_CONN_ENCRYPT, &conn->flags);
>
> + /* "Link key request" completed ahead of "connect request" completes */
> + if (ev->encr_mode == 1 && !test_bit(HCI_CONN_ENCRYPT, &conn->flags) &&
> + ev->link_type == ACL_LINK) {
> + struct link_key *key;
> + struct hci_cp_read_enc_key_size cp;
> +
> + key = hci_find_link_key(hdev, &ev->bdaddr);
> + if (key) {
> + set_bit(HCI_CONN_ENCRYPT, &conn->flags);
> +
> + if (!(hdev->commands[20] & 0x10)) {
> + conn->enc_key_size = HCI_LINK_KEY_SIZE;
> + goto notify;
> + }
> +
> + cp.handle = cpu_to_le16(conn->handle);
> + if (hci_send_cmd(hdev, HCI_OP_READ_ENC_KEY_SIZE,
> + sizeof(cp), &cp)) {
> + bt_dev_err(hdev, "sending read key size failed");
> + conn->enc_key_size = HCI_LINK_KEY_SIZE;
> + }
> +notify:
> + hci_encrypt_cfm(conn, ev->status);


Is goto and labels necessary?

> + }
> + }
> +
> /* Get remote features */
> if (conn->type == ACL_LINK) {
> struct hci_cp_read_remote_features cp;


Kind regards,

Paul

2024-03-25 17:56:33

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_event: conn is already encryped before conn establishes

Hi Hui,

On Mon, Mar 25, 2024 at 2:19 AM Hui Wang <[email protected]> wrote:
>
> We have a BT headset (Lenovo Thinkplus XT99), the pairing and
> connecting has no problem, once this headset is paired, bluez will
> remember this device and will auto re-connect it whenever the device
> is power on. The auto re-connecting works well with Windows and
> Android, but with Linux, it always fails. Through debugging, we found
> at the rfcomm connection stage, the bluetooth stack reports
> "Connection refused - security block (0x0003)".
>
> For this device, the re-connecting negotiation process is different
> from other BT headsets, it sends the Link_KEY_REQUEST command before
> the CONNECT_REQUEST completes, and it doesn't send ENCRYPT_CHANGE
> command during the negotiation. When the device sends the "connect
> complete" to hci, the ev->encr_mode is 1.
>
> So here in the conn_complete_evt(), if ev->encr_mode is 1, link type
> is ACL and HCI_CONN_ENCRYPT is not set, we set HCI_CONN_ENCRYPT to
> this conn, and update conn->enc_key_size accordingly.
>
> After this change, this BT headset could re-connect with Linux
> successfully.

I suspect it is doing Security Mode 3, so it establishes the
encryption _before_ the connection handle which probably disables
EncryptionChange since there is no handle at that point. That said I
don't remember what we shall do with respect to AES since the
Encryption_Enabled field can only be set to 0x00 or 0x01 so I assume
the later can only be:

Link Level Encryption is ON with E0 for BR/EDR.

> Signed-off-by: Hui Wang <[email protected]>
> ---
> This is the btmon log for auto re-connecting negotiation:
> Before applying this patch:
> https://pastebin.com/NUa9RJv8
>
> After applying the patch:
> https://pastebin.com/GqviChTC

Lets add these to the patch description to be more informative about
the change, you can strip the unrelated traffic at the start and end
and just focus on what changes to the traffic the patches introduces.


> net/bluetooth/hci_event.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 4ae224824012..0c45beb08cb2 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3208,6 +3208,32 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
> if (test_bit(HCI_ENCRYPT, &hdev->flags))
> set_bit(HCI_CONN_ENCRYPT, &conn->flags);
>
> + /* "Link key request" completed ahead of "connect request" completes */
> + if (ev->encr_mode == 1 && !test_bit(HCI_CONN_ENCRYPT, &conn->flags) &&
> + ev->link_type == ACL_LINK) {
> + struct link_key *key;
> + struct hci_cp_read_enc_key_size cp;
> +
> + key = hci_find_link_key(hdev, &ev->bdaddr);
> + if (key) {
> + set_bit(HCI_CONN_ENCRYPT, &conn->flags);
> +
> + if (!(hdev->commands[20] & 0x10)) {
> + conn->enc_key_size = HCI_LINK_KEY_SIZE;
> + goto notify;
> + }
> +
> + cp.handle = cpu_to_le16(conn->handle);
> + if (hci_send_cmd(hdev, HCI_OP_READ_ENC_KEY_SIZE,
> + sizeof(cp), &cp)) {
> + bt_dev_err(hdev, "sending read key size failed");
> + conn->enc_key_size = HCI_LINK_KEY_SIZE;
> + }
> +notify:
> + hci_encrypt_cfm(conn, ev->status);
> + }
> + }
> +
> /* Get remote features */
> if (conn->type == ACL_LINK) {
> struct hci_cp_read_remote_features cp;
> --
> 2.34.1
>


--
Luiz Augusto von Dentz

2024-03-27 03:17:03

by Hui Wang

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_event: conn is already encryped before conn establishes


On 3/25/24 22:18, Paul Menzel wrote:
> Dear Hui,
>
>
> Thank you for your patch. Some minor nits from my side.
>
> Regarding the summary (encryp*t*ed), please describe the action of the
> change and not the issue.
>
> Am 25.03.24 um 07:18 schrieb Hui Wang:
>> We have a BT headset (Lenovo Thinkplus XT99), the pairing and
>> connecting has no problem, once this headset is paired, bluez will
>> remember this device and will auto re-connect it whenever the device
>> is power on. The auto re-connecting works well with Windows and
>
> power*ed*
>
>> Android, but with Linux, it always fails. Through debugging, we found
>> at the rfcomm connection stage, the bluetooth stack reports
>> "Connection refused - security block (0x0003)".
>>
>> For this device, the re-connecting negotiation process is different
>> from other BT headsets, it sends the Link_KEY_REQUEST command before
>> the CONNECT_REQUEST completes, and it doesn't send ENCRYPT_CHANGE
>> command during the negotiation. When the device sends the "connect
>> complete" to hci, the ev->encr_mode is 1.
>
> Is that in accordance with the specification or a violation?
>
According to Luiz's comment, looks like it follows the Security Mode 3
policy, Encryption is established during Link Level.
>> So here in the conn_complete_evt(), if ev->encr_mode is 1, link type
>> is ACL and HCI_CONN_ENCRYPT is not set, we set HCI_CONN_ENCRYPT to
>> this conn, and update conn->enc_key_size accordingly.
>>
>> After this change, this BT headset could re-connect with Linux
>> successfully.
>
> Despite this being a generic issue, could you please document with
> what controller you tested this?
>
> Do you have any bug/issue tracker URL, you can reference?
OK, will address them in the v2, and will also fix those misspell.
>
>> Signed-off-by: Hui Wang <[email protected]>
>> ---
>> This is the btmon log for auto re-connecting negotiation:
>> Before applying this patch:
>> https://pastebin.com/NUa9RJv8
>>
>> After applying the patch:
>> https://pastebin.com/GqviChTC
>>
>> net/bluetooth/hci_event.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 4ae224824012..0c45beb08cb2 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -3208,6 +3208,32 @@ static void hci_conn_complete_evt(struct
>> hci_dev *hdev, void *data,
>>           if (test_bit(HCI_ENCRYPT, &hdev->flags))
>>               set_bit(HCI_CONN_ENCRYPT, &conn->flags);
>>   +        /* "Link key request" completed ahead of "connect request"
>> completes */
>> +        if (ev->encr_mode == 1 && !test_bit(HCI_CONN_ENCRYPT,
>> &conn->flags) &&
>> +            ev->link_type == ACL_LINK) {
>> +            struct link_key *key;
>> +            struct hci_cp_read_enc_key_size cp;
>> +
>> +            key = hci_find_link_key(hdev, &ev->bdaddr);
>> +            if (key) {
>> +                set_bit(HCI_CONN_ENCRYPT, &conn->flags);
>> +
>> +                if (!(hdev->commands[20] & 0x10)) {
>> +                    conn->enc_key_size = HCI_LINK_KEY_SIZE;
>> +                    goto notify;
>> +                }
>> +
>> +                cp.handle = cpu_to_le16(conn->handle);
>> +                if (hci_send_cmd(hdev, HCI_OP_READ_ENC_KEY_SIZE,
>> +                         sizeof(cp), &cp)) {
>> +                    bt_dev_err(hdev, "sending read key size failed");
>> +                    conn->enc_key_size = HCI_LINK_KEY_SIZE;
>> +                }
>> +notify:
>> +                hci_encrypt_cfm(conn, ev->status);
>
>
> Is goto and labels necessary?

Will drop them in the v2, replace them with if {} else {}

Thanks,

Hui.

>
>> +            }
>> +        }
>> +
>>           /* Get remote features */
>>           if (conn->type == ACL_LINK) {
>>               struct hci_cp_read_remote_features cp;
>
>
> Kind regards,
>
> Paul

2024-03-27 03:22:24

by Hui Wang

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_event: conn is already encryped before conn establishes


On 3/26/24 00:59, Luiz Augusto von Dentz wrote:
> Hi Hui,
>
> On Mon, Mar 25, 2024 at 2:19 AM Hui Wang <[email protected]> wrote:
>> We have a BT headset (Lenovo Thinkplus XT99), the pairing and
>> connecting has no problem, once this headset is paired, bluez will
>> remember this device and will auto re-connect it whenever the device
>> is power on. The auto re-connecting works well with Windows and
>> Android, but with Linux, it always fails. Through debugging, we found
>> at the rfcomm connection stage, the bluetooth stack reports
>> "Connection refused - security block (0x0003)".
>>
>> For this device, the re-connecting negotiation process is different
>> from other BT headsets, it sends the Link_KEY_REQUEST command before
>> the CONNECT_REQUEST completes, and it doesn't send ENCRYPT_CHANGE
>> command during the negotiation. When the device sends the "connect
>> complete" to hci, the ev->encr_mode is 1.
>>
>> So here in the conn_complete_evt(), if ev->encr_mode is 1, link type
>> is ACL and HCI_CONN_ENCRYPT is not set, we set HCI_CONN_ENCRYPT to
>> this conn, and update conn->enc_key_size accordingly.
>>
>> After this change, this BT headset could re-connect with Linux
>> successfully.
> I suspect it is doing Security Mode 3, so it establishes the
> encryption _before_ the connection handle which probably disables
> EncryptionChange since there is no handle at that point. That said I
> don't remember what we shall do with respect to AES since the
> Encryption_Enabled field can only be set to 0x00 or 0x01 so I assume
> the later can only be:
>
> Link Level Encryption is ON with E0 for BR/EDR.
Thanks for sharing this, it is sth I will study. :-)
>> Signed-off-by: Hui Wang <[email protected]>
>> ---
>> This is the btmon log for auto re-connecting negotiation:
>> Before applying this patch:
>> https://pastebin.com/NUa9RJv8
>>
>> After applying the patch:
>> https://pastebin.com/GqviChTC
> Lets add these to the patch description to be more informative about
> the change, you can strip the unrelated traffic at the start and end
> and just focus on what changes to the traffic the patches introduces.

OK, will address it in the v2.

Thanks,

Hui.

>
>> net/bluetooth/hci_event.c | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 4ae224824012..0c45beb08cb2 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -3208,6 +3208,32 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
>> if (test_bit(HCI_ENCRYPT, &hdev->flags))
>> set_bit(HCI_CONN_ENCRYPT, &conn->flags);
>>
>> + /* "Link key request" completed ahead of "connect request" completes */
>> + if (ev->encr_mode == 1 && !test_bit(HCI_CONN_ENCRYPT, &conn->flags) &&
>> + ev->link_type == ACL_LINK) {
>> + struct link_key *key;
>> + struct hci_cp_read_enc_key_size cp;
>> +
>> + key = hci_find_link_key(hdev, &ev->bdaddr);
>> + if (key) {
>> + set_bit(HCI_CONN_ENCRYPT, &conn->flags);
>> +
>> + if (!(hdev->commands[20] & 0x10)) {
>> + conn->enc_key_size = HCI_LINK_KEY_SIZE;
>> + goto notify;
>> + }
>> +
>> + cp.handle = cpu_to_le16(conn->handle);
>> + if (hci_send_cmd(hdev, HCI_OP_READ_ENC_KEY_SIZE,
>> + sizeof(cp), &cp)) {
>> + bt_dev_err(hdev, "sending read key size failed");
>> + conn->enc_key_size = HCI_LINK_KEY_SIZE;
>> + }
>> +notify:
>> + hci_encrypt_cfm(conn, ev->status);
>> + }
>> + }
>> +
>> /* Get remote features */
>> if (conn->type == ACL_LINK) {
>> struct hci_cp_read_remote_features cp;
>> --
>> 2.34.1
>>
>
> --
> Luiz Augusto von Dentz