Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2808-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <36b87a84-1979-4237-bd68-ddef2601951e@canonical.com>
Date: Wed, 27 Mar 2024 11:16:36 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Bluetooth: hci_event: conn is already encryped before
 conn establishes
To: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: linux-bluetooth@vger.kernel.org, luiz.dentz@gmail.com,
 marcel@holtmann.org, johan.hedberg@gmail.com
References: <20240325061841.22387-1-hui.wang@canonical.com>
 <bedd53fd-e034-4038-9180-5ee378460e09@molgen.mpg.de>
Content-Language: en-US
From: Hui Wang <hui.wang@canonical.com>
In-Reply-To: <bedd53fd-e034-4038-9180-5ee378460e09@molgen.mpg.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


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 <hui.wang@canonical.com>
>> ---
>> 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