[PATCH] Bluetooth: hci_conn: verify connection is to be aborted before doing it

[Pauli Virtanen <pav@xxxxxx>]

There is a race condition where a connection handle is reused, after
hci_abort_conn but before abort_conn_sync is processed in hci_sync. In
this case, hci_abort_conn_sync ends up calling hci_connect_cfm with
success status and then delete the connection, which causes
use-after-free.

Fix by checking abort_reason before calling hci_abort_conn_sync.

Also fix some theoretical UAF / races, where something frees the conn
while hci_abort_conn_sync is working on it.

Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
Reported-by: syzbot+a0c80b06ae2cb8895bc4@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://lore.kernel.org/linux-bluetooth/0000000000005ab984060371583e@xxxxxxxxxx/
Signed-off-by: Pauli Virtanen <pav@xxxxxx>
---

Notes:
    Not sure how you'd hit this condition in real controller, but syzbot
    does end up calling hci_abort_conn_sync with reason == 0 which then
    causes havoc.

    This can be verified: with a patch that changes abort_conn_sync to

        2874	conn = hci_conn_hash_lookup_handle(hdev, handle);
        2875	if (!conn || WARN_ON(!conn->abort_reason))
        2876		return 0;

    https://syzkaller.appspot.com/text?tag=Patch&x=16eff740680000

    it hits that WARN_ON:

    https://syzkaller.appspot.com/x/log.txt?x=10affb97a80000

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master

 net/bluetooth/hci_conn.c | 17 ++++++++++++++++-
 net/bluetooth/hci_sync.c |  2 ++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 9d5057cef30a..8622eddb946a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2886,12 +2886,25 @@ static int abort_conn_sync(struct hci_dev *hdev, void *data)
 {
 	struct hci_conn *conn;
 	u16 handle = PTR_UINT(data);
+	u8 reason;
+	int err;
+
+	rcu_read_lock();
 
 	conn = hci_conn_hash_lookup_handle(hdev, handle);
+	if (conn) {
+		reason = READ_ONCE(conn->abort_reason);
+		conn = reason ? hci_conn_get(conn) : NULL;
+	}
+
+	rcu_read_unlock();
+
 	if (!conn)
 		return 0;
 
-	return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
+	err = hci_abort_conn_sync(hdev, conn, reason);
+	hci_conn_put(conn);
+	return err;
 }
 
 int hci_abort_conn(struct hci_conn *conn, u8 reason)
@@ -2903,6 +2916,8 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
 	 */
 	if (conn->abort_reason)
 		return 0;
+	if (WARN_ON(!reason))
+		reason = HCI_ERROR_UNSPECIFIED;
 
 	bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
 
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 9b93653c6197..a93096c5cbfd 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5375,6 +5375,8 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
 	u16 handle = conn->handle;
 	struct hci_conn *c;
 
+	WARN_ON(!reason);
+
 	switch (conn->state) {
 	case BT_CONNECTED:
 	case BT_CONFIG:
-- 
2.41.0