linux-kernel - Re: [PATCH] Bluetooth: Always call advertising disable before setting params

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: <8E70C497-BDCE-471F-9ECD-790E2FE3B024@holtmann.org>
Date:   Wed, 24 Mar 2021 08:06:32 +0100
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Daniel Winkler <danielwinkler@...gle.com>
Cc:     linux-bluetooth <linux-bluetooth@...r.kernel.org>,
        CrosBT Upstreaming <chromeos-bluetooth-upstreaming@...omium.org>,
        Miao-chen Chou <mcchou@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        LKML <linux-kernel@...r.kernel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH] Bluetooth: Always call advertising disable before setting
 params

Hi Daniel,

> In __hci_req_enable_advertising, the HCI_LE_ADV hdev flag is temporarily
> cleared to allow the random address to be set, which exposes a race
> condition when an advertisement is configured immediately (<10ms) after
> software rotation starts to refresh an advertisement.
> 
> In normal operation, the HCI_LE_ADV flag is updated as follows:
> 
> 1. adv_timeout_expire is called, HCI_LE_ADV gets cleared in
>   __hci_req_enable_advertising, but hci_req configures an enable
>   request
> 2. hci_req is run, enable callback re-sets HCI_LE_ADV flag
> 
> However, in this race condition, the following occurs:
> 
> 1. adv_timeout_expire is called, HCI_LE_ADV gets cleared in
>   __hci_req_enable_advertising, but hci_req configures an enable
>   request
> 2. add_advertising is called, which also calls
>   __hci_req_enable_advertising. Because HCI_LE_ADV was cleared in Step
>   1, no "disable" command is queued.
> 3. hci_req for adv_timeout_expire is run, which enables advertising and
>   re-sets HCI_LE_ADV
> 4. hci_req for add_advertising is run, but because no "disable" command
>   was queued, we try to set advertising parameters while advertising is
>   active, causing a Command Disallowed error, failing the registration.
> 
> To resolve the issue, this patch removes the check for the HCI_LE_ADV
> flag, and always queues the "disable" request, since HCI_LE_ADV could be
> very temporarily out-of-sync. According to the spec, there is no harm in
> calling "disable" when advertising is not active.
> 
> Reviewed-by: Miao-chen Chou <mcchou@...omium.org>
> Signed-off-by: Daniel Winkler <danielwinkler@...gle.com>
> ---
> 
> net/bluetooth/hci_request.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index 8ace5d34b01efe..2b4b99f4cedf21 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -1547,8 +1547,10 @@ void __hci_req_enable_advertising(struct hci_request *req)
> 	if (!is_advertising_allowed(hdev, connectable))
> 		return;
> 
> -	if (hci_dev_test_flag(hdev, HCI_LE_ADV))
> -		__hci_req_disable_advertising(req);
> +	/* Request that the controller stop advertising. This can be called
> +	 * whether or not there is an active advertisement.
> +	 */
> +	__hci_req_disable_advertising(req);

can you include a btmon trace that shows that we don’t get a HCI error. Since if we get one, then the complete request will fail. And that has further side effects.

Regards

Marcel