netdev - Re: [PATCH] Bluetooth: clean up connection in hci_cs

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] [day] [month] [year] [list]

Message-Id: <C60D1463-5B04-4C25-B4A2-0A9DD25A67B9@holtmann.org>
Date:   Wed, 11 Mar 2020 15:31:26 +0100
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Manish Mandlik <mmandlik@...gle.com>
Cc:     Alain Michaud <alainm@...omium.org>,
        Bluez mailing list <linux-bluetooth@...r.kernel.org>,
        Miao-chen Chou <mcchou@...omium.org>,
        Joseph Hwang <josephsih@...omium.org>,
        Yoni Shavit <yshavit@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Johan Hedberg <johan.hedberg@...il.com>,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH] Bluetooth: clean up connection in hci_cs_disconnect

Hi Manish,

> In bluetooth core specification 4.2,
> Vol 2, Part E, 7.8.9 LE Set Advertise Enable Command, it says
> 
>    The Controller shall continue advertising until ...
>    or until a connection is created or ...
>    In these cases, advertising is then disabled.
> 
> Hence, advertising would be disabled before a connection is
> established. In current kernel implementation, advertising would
> be re-enabled when all connections are terminated.
> 
> The correct disconnection flow looks like
> 
>  < HCI Command: Disconnect
> 
>> HCI Event: Command Status
>      Status: Success
> 
>> HCI Event: Disconnect Complete
>      Status: Success
> 
> Specifically, the last Disconnect Complete Event would trigger a
> callback function hci_event.c:hci_disconn_complete_evt() to
> cleanup the connection and re-enable advertising when proper.
> 
> However, sometimes, there might occur an exception in the controller
> when disconnection is being executed. The disconnection flow might
> then look like
> 
>  < HCI Command: Disconnect
> 
>> HCI Event: Command Status
>      Status: Unknown Connection Identifier
> 
>  Note that "> HCI Event: Disconnect Complete" is missing when such an
> exception occurs. This would result in advertising staying disabled
> forever since the connection in question is not cleaned up correctly.
> 
> To fix the controller exception issue, we need to do some connection
> cleanup when the disconnect command status indicates an error.
> 
> Signed-off-by: Joseph Hwang <josephsih@...omium.org>
> Signed-off-by: Manish Mandlik <mmandlik@...gle.com>
> ---
> 
> net/bluetooth/hci_event.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index a40ed31f6eb8f..7f7e5ba3974a8 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2191,6 +2191,7 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
> {
> 	struct hci_cp_disconnect *cp;
> 	struct hci_conn *conn;
> +	u8 type;

remove this please.

> 
> 	if (!status)
> 		return;
> @@ -2202,10 +2203,21 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
> 	hci_dev_lock(hdev);
> 
> 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
> -	if (conn)
> +	if (conn) {

And add this.

		u8 type = conn->type;

> 		mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
> 				       conn->dst_type, status);
> 
> +		/* If the disconnection failed for any reason, the upper layer
> +		 * does not retry to disconnect in current implementation.
> +		 * Hence, we need to do some basic cleanup here and re-enable
> +		 * advertising if necessary.
> +		 */
> +		type = conn->type;

And then remove this.

> +		hci_conn_del(conn);
> +		if (type == LE_LINK)
> +			hci_req_reenable_advertising(hdev);
> +	}
> +
> 	hci_dev_unlock(hdev);
> }

Otherwise this looks good.

Regards

Marcel