Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1221-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Andrei Volkov <andrey.volkov@mobile-devices.fr>
Message-ID: <747030d8-a06f-4d7d-bf93-0e95f54b9bbf@munic.io>
Date: Mon, 22 Jan 2024 15:45:46 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: regression in c7f59461f5a78: Bluetooth: Fix a refcnt underflow
 problem for hci_conn
Content-Language: en-US, fr
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
References: <a3a31b2d-6039-4b50-af81-47f7ea1a0461@munic.io>
 <CABBYNZ+9UdG1cMZVmdtN3U2aS16AKMCyTARZZyFX7xTEDWcMOw@mail.gmail.com>
Organization: MUNIC
In-Reply-To: <CABBYNZ+9UdG1cMZVmdtN3U2aS16AKMCyTARZZyFX7xTEDWcMOw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi Luiz,

Wouldn't it be better to add a 'yet-another' flag as an addition to 
HCI_CONN_SSP_ENABLED, and clear it unconditionally at the top of 
'hci_remote_ext_features_evt', before

"        conn = hci_conn_hash_lookup_handle "

check?

In this case broken ext_features response (with ev->status != 0 or conn 
== NULL) will not indirectly enable the SSP feature.

Regards
Andrei VOLKOV

Le 22/01/2024 à 15:02, Luiz Augusto von Dentz a écrit :

> Hi Andrei,
>
> On Mon, Jan 22, 2024 at 7:18 AM Andrei Volkov
> <andrey.volkov@mobile-devices.fr> wrote:
>> Hello,
>>
>> Lately we've bumped with regression introduced by commit:
>>
>>    c7f59461f5a78 ("Bluetooth: Fix a refcnt underflow problem for
>> hci_conn", 2023-10-04)
>>
>> The regression related with adding "hci_conn_ssp_enabled()" check in
>> "hci_io_capa_request_evt()" handler, and broke pairing process initiated
>> by the external device.
>>
>> Precisely, some ext. devices, like any phone equipped with Android ver <
>> 14 (we have not latest one, so we didn't check), always send "IO
>> Capability Request" before "Read Remote Extended Features" command, as
>> consequence the flag "HCI_CONN_SSP_ENABLED" not yet activated at the
>> time of "hci_io_capa_request_evt()" execution  and
>> "hci_conn_ssp_enabled()" always returns false in time of the pairing.
>>
>> As a result, pairing always fails. The quick and dirty fix is revert the
>> ssp check introduced in the subj. commit, like below:
>>
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -5329,7 +5329,7 @@ static void hci_io_capa_request_evt(struct hci_dev
>> *hdev, void *data,
>>           hci_dev_lock(hdev);
>>
>>           conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
>> -       if (!conn || !hci_conn_ssp_enabled(conn))
>> +       if (!conn)
>>                   goto unlock;
>>
>>           hci_conn_hold(conn);
>>
>>
>> However, a more thorough and correct fix requires discussion and
>> testing. Therefore, I would like to get any comments/suggestion from the
>> community before doing this.
> I think we need to do something like this, so we consider only the
> local SSP flag when evaluating if we should proceed to respond or just
> drop:
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 6130c969f361..a15924db83d9 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5327,9 +5327,12 @@ static void hci_io_capa_request_evt(struct
> hci_dev *hdev, void *data,
>          hci_dev_lock(hdev);
>
>          conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
> -       if (!conn || !hci_conn_ssp_enabled(conn))
> +       if (!conn || !hci_dev_test_flag(hdev, HCI_SSP_ENABLED))
>                  goto unlock;
>
> +       /* Assume remote supports SSP since it has triggered this event */
> +       set_bit(HCI_CONN_SSP_ENABLED, &conn->flags);
> +
>          hci_conn_hold(conn);
>
>          if (!hci_dev_test_flag(hdev, HCI_MGMT))
>
>
>> Regards
>> Andrey VOLKOV
>>
>>
>