[PATCH 1/3] Bluetooth: hci_conn: fix UAF in create_big_sync and create_big_complete

[PATCH 1/3] Bluetooth: hci_conn: fix UAF in create_big_sync and create_big_complete

[Aaron Esau]

From: Aaron Esau <aaron1esau@gmail.com>

hci_connect_bis() queues create_big_sync with a raw conn pointer.
create_big_sync blocks in __hci_cmd_sync_sk while a concurrent
hci_conn_del on hdev->workqueue frees conn. On timeout, freed memory
is dereferenced. The dequeue path also double-frees: hci_conn_del
invokes create_big_complete via hci_cmd_sync_dequeue, which calls
hci_conn_del again.

Take hci_conn_get before queueing, add hci_conn_valid checks, handle
-ECANCELED, drop with hci_conn_put in create_big_complete. Follows
the create_le_conn_complete pattern.

Fixes: eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections")
Signed-off-by: Aaron Esau <aaron1esau@gmail.com>
---
 net/bluetooth/hci_conn.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index dc08585..59f5451 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2119,11 +2119,16 @@ static void hci_iso_qos_setup(struct hci_dev *hdev, struct hci_conn *conn,
 static int create_big_sync(struct hci_dev *hdev, void *data)
 {
 	struct hci_conn *conn = data;
-	struct bt_iso_qos *qos = &conn->iso_qos;
+	struct bt_iso_qos *qos;
 	u16 interval, sync_interval = 0;
 	u32 flags = 0;
 	int err;
 
+	if (!hci_conn_valid(hdev, conn))
+		return -ECANCELED;
+
+	qos = &conn->iso_qos;
+
 	if (qos->bcast.out.phy == 0x02)
 		flags |= MGMT_ADV_FLAG_SEC_2M;
 
@@ -2198,11 +2203,24 @@ static void create_big_complete(struct hci_dev *hdev, void *data, int err)
 
 	bt_dev_dbg(hdev, "conn %p", conn);
 
+	if (err == -ECANCELED)
+		goto done;
+
+	hci_dev_lock(hdev);
+
+	if (!hci_conn_valid(hdev, conn))
+		goto unlock;
+
 	if (err) {
 		bt_dev_err(hdev, "Unable to create BIG: %d", err);
 		hci_connect_cfm(conn, err);
 		hci_conn_del(conn);
 	}
+
+unlock:
+	hci_dev_unlock(hdev);
+done:
+	hci_conn_put(conn);
 }
 
 struct hci_conn *hci_bind_bis(struct hci_dev *hdev, bdaddr_t *dst, __u8 sid,
@@ -2331,9 +2349,11 @@ struct hci_conn *hci_connect_bis(struct hci_dev *hdev, bdaddr_t *dst,
 				 BT_BOUND, &data);
 
 	/* Queue start periodic advertising and create BIG */
+	hci_conn_get(conn);
 	err = hci_cmd_sync_queue(hdev, create_big_sync, conn,
 				 create_big_complete);
 	if (err < 0) {
+		hci_conn_put(conn);
 		hci_conn_drop(conn);
 		return ERR_PTR(err);
 	}
-- 
2.52.0

[PATCH 3/3] Bluetooth: hci_conn: fix UAF in hci_enhanced_setup_sync

[Aaron Esau]

From: Aaron Esau <aaron1esau@gmail.com>

hci_setup_sync queues hci_enhanced_setup_sync with conn_handle as data
without taking a reference on conn. hci_conn_del tries to dequeue with
conn as data, but the pointer comparison fails (data is conn_handle).
The existing hci_conn_valid check has a TOCTOU gap since conn can be
freed after the check passes. conn_handle also leaks on cancellation
because no destroy callback is set.

Take hci_conn_get on conn, add a destroy callback that frees
conn_handle and drops the reference, and move kfree(conn_handle) from
the sync function to the destroy callback.

Fixes: e07a06b4eb41 ("Bluetooth: Convert SCO configure_datapath to hci_sync")
Signed-off-by: Aaron Esau <aaron1esau@gmail.com>
---
 net/bluetooth/hci_conn.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index a7faa4c..6a567d6 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -278,6 +278,15 @@ error:
 	return err;
 }
 
+static void hci_enhanced_setup_sync_complete(struct hci_dev *hdev,
+					     void *data, int err)
+{
+	struct conn_handle_t *conn_handle = data;
+
+	hci_conn_put(conn_handle->conn);
+	kfree(conn_handle);
+}
+
 static int hci_enhanced_setup_sync(struct hci_dev *hdev, void *data)
 {
 	struct conn_handle_t *conn_handle = data;
@@ -286,8 +295,6 @@ static int hci_enhanced_setup_sync(struct hci_dev *hdev, void *data)
 	struct hci_cp_enhanced_setup_sync_conn cp;
 	const struct sco_param *param;
 
-	kfree(conn_handle);
-
 	if (!hci_conn_valid(hdev, conn))
 		return -ECANCELED;
 
@@ -467,12 +474,15 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 		if (!conn_handle)
 			return false;
 
-		conn_handle->conn = conn;
+		conn_handle->conn = hci_conn_get(conn);
 		conn_handle->handle = handle;
 		result = hci_cmd_sync_queue(conn->hdev, hci_enhanced_setup_sync,
-					    conn_handle, NULL);
-		if (result < 0)
+					    conn_handle,
+					    hci_enhanced_setup_sync_complete);
+		if (result < 0) {
+			hci_conn_put(conn);
 			kfree(conn_handle);
+		}
 
 		return result == 0;
 	}
-- 
2.52.0

RE: [1/3] Bluetooth: hci_conn: fix UAF in create_big_sync and create_big_complete

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: net/bluetooth/hci_conn.c:2119
error: net/bluetooth/hci_conn.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth