linux-kernel - Re: [PATCH v1] Bluetooth: hci_sync: clear cmd_sync_work

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f96edd22-c7d1-4d83-96d6-8138fbb1ff71@quicinc.com>
Date: Wed, 4 Dec 2024 21:47:33 +0800
From: Jiayang Mao <quic_jiaymao@...cinc.com>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
CC: Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg
	<johan.hedberg@...il.com>,
        <linux-bluetooth@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <quic_chejiang@...cinc.com>, <quic_shuaz@...cinc.com>
Subject: Re: [PATCH v1] Bluetooth: hci_sync: clear cmd_sync_work_list when
 power off

Hi Luiz,

On 2024/12/4 1:28, Luiz Augusto von Dentz wrote:
> Hi Jiayang,
> 
> On Tue, Dec 3, 2024 at 12:19 PM Jiayang Mao <quic_jiaymao@...cinc.com> wrote:
>>
>> Hi Luiz,
>>
>> On 2024/12/3 4:41, Luiz Augusto von Dentz wrote:
>>> Hi Jiayang,
>>>
>>> On Mon, Nov 25, 2024 at 12:51 PM Jiayang Mao <quic_jiaymao@...cinc.com> wrote:
>>>>
>>>> Clear the remaining command in cmd_sync_work_list when BT is
>>>> performing power off. In some cases, this list is not empty after
>>>> power off. BT host will try to send more HCI commands.
>>>> This can cause unexpected results.
>>>
>>> What commands are in the queue?
>>
>> If turning off BT during pairing, "hci_acl_create_conn_sync" has chances
>> to be left in the queue. Then the driver will try to send the HCI
>> command of creating connection but failed.
> 
> There shouldn't be happening though:
> 
>      /* Terminated due to Power Off */
>      err = hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE_POWER_OFF);
>      if (err)
>          goto out;
> 
>      err = hci_dev_close_sync(hdev);
> 
> Perhaps there is something attempting to connect after
> hci_disconnect_all_sync has completed, in that case there is a bug
> around this sequence or we need to check HCI_POWERING_DOWN to not
> attempt to process the connection attempts.
> 
After pairing, an L2CAP channel is to be created by l2cap_sock_create
function. It eventually calls hci_connect_acl_sync, which adds
hci_acl_create_conn_sync to the cmd_sync_work_list.

The issue arises if BT is turned off after this addition but before
hci_acl_create_conn_sync is execute.

Your suggestion to check HCI_POWERING_DOWN seems more appropriate
for addressing this issue. We can try incorporating this check into
hci_acl_create_conn_sync.

>>>
>>>> Signed-off-by: Jiayang Mao <quic_jiaymao@...cinc.com>
>>>> ---
>>>>    net/bluetooth/hci_sync.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
>>>> index c86f4e42e..bc622d074 100644
>>>> --- a/net/bluetooth/hci_sync.c
>>>> +++ b/net/bluetooth/hci_sync.c
>>>> @@ -5139,6 +5139,7 @@ int hci_dev_close_sync(struct hci_dev *hdev)
>>>>    {
>>>>           bool auto_off;
>>>>           int err = 0;
>>>> +       struct hci_cmd_sync_work_entry *entry, *tmp;
>>>>
>>>>           bt_dev_dbg(hdev, "");
>>>>
>>>> @@ -5258,6 +5259,11 @@ int hci_dev_close_sync(struct hci_dev *hdev)
>>>>           clear_bit(HCI_RUNNING, &hdev->flags);
>>>>           hci_sock_dev_event(hdev, HCI_DEV_CLOSE);
>>>>
>>>> +       mutex_lock(&hdev->cmd_sync_work_lock);
>>>> +       list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list)
>>>> +               _hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
>>>> +       mutex_unlock(&hdev->cmd_sync_work_lock);
>>>
>>> Seems equivalent to hci_cmd_sync_clear, that said we should have been
>>> running with that lock already, also if there is a sequence like
>>> close/open the close may cancel the subsequent open, so I don't think
>>> we should be canceling every subsequent callback like this.
>>
>> In hci_cmd_sync_clear, the work cmd_sync_work and reenable_adv_work are
>> canceled. hci_cmd_sync_clear is not directly called because these two
>> works should not be canceled during power off.
>> Do you mean the added code should be moved to other functions to avoid
>> the risk of lock?
>>
>> Yes. This change lacks considering sequence of close/open. I will update
>> the implementation to ensure it does not remove the opening and the
>> operations after re-opening.
>>>
>>>>           /* After this point our queues are empty and no tasks are scheduled. */
>>>>           hdev->close(hdev);
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>
>>>
>>
> 
>