[PATCH] Bluetooth: SCO: serialise sco_conn lifetime against sco_recv_scodata()

[PATCH] Bluetooth: SCO: serialise sco_conn lifetime against sco_recv_scodata()

[Aldo Ariel Panzardo]

sco_recv_scodata() upgrades the weak hcon->sco_data back-pointer to a
strong reference under hdev->lock:

	hci_dev_lock(hdev);
	hcon = hci_conn_hash_lookup_handle(hdev, handle);
	...
	conn = sco_conn_hold_unless_zero(hcon->sco_data);
	hci_dev_unlock(hdev);

but the pointer is cleared from the other side without that lock. When
the last sco_conn reference is dropped, sco_conn_free() ran

	conn->hcon->sco_data = NULL;

with no hdev->lock held, so the RX path could read hcon->sco_data and
call kref_get_unless_zero() on an sco_conn that was concurrently freed:

	BUG: KASAN: slab-use-after-free in sco_conn_hold_unless_zero+0xbe/0x160
	Write of size 4 by task kworker/u17:0
	Workqueue: hci0 hci_rx_work
	Call Trace:
	 sco_conn_hold_unless_zero+0xbe/0x160
	 sco_recv_scodata+0x13f/0x490
	 hci_rx_work+0x3af/0x730

kref_get_unless_zero() only guards against a zero refcount, not against
the backing memory already being freed.

Give hcon->sco_data an actual reference on the sco_conn it points to, so
the object cannot be freed while the pointer is still installed, and only
clear and drop it from sco_conn_del(), which runs under hdev->lock (its
callers, sco_connect_cfm() and sco_disconn_cfm(), hold it). The reader in
sco_recv_scodata() also takes hdev->lock, so the store and the read are
now serialised: the RX path either observes NULL or a reference that is
guaranteed to stay valid until it drops its own.

sco_conn_add() no longer hands its kref_init() reference to the caller as
the connection's only reference; that initial reference is the one owned
by hcon->sco_data, and callers get their own via sco_conn_hold().

Because the sco_conn can now outlive the socket (the association keeps it
alive until the link goes down), the hci_conn can no longer be dropped
from sco_conn_free() without regressing socket close: closing a connected
SCO socket must still tear the link down. Move hci_conn ownership to the
socket instead: __sco_chan_add() takes an hci_conn reference and it is
released from sco_chan_del() and sco_sock_destruct(), exactly once. The
sco_conn no longer owns an hci_conn reference, so sco_conn_add() stops
consuming one and sco_connect() drops the hci_connect_sco() reference on
every path; sco_connect_cfm() no longer needs its hci_conn_hold() dance.

Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
Cc: stable@vger.kernel.org
Suggested-by: Pauli Virtanen <pav@iki.fi>
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 net/bluetooth/sco.c | 84 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 69 insertions(+), 15 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 3d4362a09..8df0fa48f 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -84,12 +84,14 @@ static void sco_conn_free(struct kref *ref)
 	if (conn->sk)
 		sco_pi(conn->sk)->conn = NULL;
 
-	if (conn->hcon) {
-		conn->hcon->sco_data = NULL;
-		hci_conn_drop(conn->hcon);
-	}
+	/* hcon->sco_data is cleared and the association's reference on the
+	 * sco_conn is dropped in sco_conn_del() under hdev->lock, and the
+	 * hci_conn is now owned by the socket (held in __sco_chan_add() and
+	 * dropped in sco_chan_del()/sco_sock_destruct()), so there is nothing
+	 * left to release towards hcon here.
+	 */
 
-	/* Ensure no more work items will run since hci_conn has been dropped */
+	/* Ensure no more work items will run before the connection is freed */
 	disable_delayed_work_sync(&conn->timeout_work);
 
 	kfree(conn);
@@ -188,8 +190,11 @@ static void sco_sock_clear_timer(struct sock *sk)
 }
 
 /* ---- SCO connections ---- */
-/* Consumes a reference on @hcon, which the returned sco_conn owns until it is
- * freed. On failure (NULL return) the reference is left for the caller to drop.
+/* Returns a new reference the caller must drop with sco_conn_put(). The
+ * hcon->sco_data association holds its own reference on the sco_conn for the
+ * connection's lifetime; it is dropped in sco_conn_del() under hdev->lock.
+ * @hcon is not consumed: the hci_conn reference is taken and owned by the
+ * socket in __sco_chan_add().
  */
 static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 {
@@ -201,9 +206,6 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 			sco_conn_lock(conn);
 			conn->hcon = hcon;
 			sco_conn_unlock(conn);
-		} else {
-			/* conn already owns a reference on hcon */
-			hci_conn_drop(hcon);
 		}
 		return conn;
 	}
@@ -227,7 +229,10 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 
 	BT_DBG("hcon %p conn %p", hcon, conn);
 
-	return conn;
+	/* kref_init() above set the association reference owned by
+	 * hcon->sco_data; hand the caller its own reference.
+	 */
+	return sco_conn_hold(conn);
 }
 
 /* Delete channel.
@@ -242,9 +247,20 @@ static void sco_chan_del(struct sock *sk, int err)
 	BT_DBG("sk %p, conn %p, err %d", sk, conn, err);
 
 	if (conn) {
+		struct hci_conn *hcon;
+
 		sco_conn_lock(conn);
 		conn->sk = NULL;
+		hcon = conn->hcon;
 		sco_conn_unlock(conn);
+
+		/* Release the socket's own reference on the hci_conn taken in
+		 * __sco_chan_add() so that closing the socket still tears the
+		 * link down even while the sco_conn is kept alive by the
+		 * hcon->sco_data association reference.
+		 */
+		if (hcon)
+			hci_conn_drop(hcon);
 		sco_conn_put(conn);
 	}
 
@@ -266,6 +282,15 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
 
 	BT_DBG("hcon %p conn %p, err %d", hcon, conn, err);
 
+	/* Detach the connection from the hci_conn and drop the reference held
+	 * by the hcon->sco_data association.  The caller holds hdev->lock,
+	 * which serialises this NULL store and put against the read of
+	 * hcon->sco_data in sco_recv_scodata(), closing the use-after-free
+	 * where the RX path could upgrade an already-freed sco_conn.
+	 */
+	hcon->sco_data = NULL;
+	sco_conn_put(conn);
+
 	sco_conn_lock(conn);
 	sk = sco_sock_hold(conn);
 	sco_conn_unlock(conn);
@@ -290,6 +315,13 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
 	sco_pi(sk)->conn = sco_conn_hold(conn);
 	conn->sk = sk;
 
+	/* The socket owns a reference on the hci_conn for as long as it stays
+	 * attached; it is dropped in sco_chan_del()/sco_sock_destruct().  This
+	 * keeps close() tearing the link down now that the sco_conn can outlive
+	 * the socket through the hcon->sco_data association reference.
+	 */
+	hci_conn_hold(conn->hcon);
+
 	if (parent)
 		bt_accept_enqueue(parent, sk, true);
 }
@@ -371,6 +403,7 @@ static int sco_connect(struct sock *sk)
 	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
 		release_sock(sk);
 		sco_conn_put(conn);
+		hci_conn_drop(hcon);
 		err = -EBADFD;
 		goto unlock;
 	}
@@ -379,6 +412,7 @@ static int sco_connect(struct sock *sk)
 	sco_conn_put(conn);
 	if (err) {
 		release_sock(sk);
+		hci_conn_drop(hcon);
 		goto unlock;
 	}
 
@@ -395,6 +429,11 @@ static int sco_connect(struct sock *sk)
 
 	release_sock(sk);
 
+	/* The socket took its own hci_conn reference in __sco_chan_add(); drop
+	 * the one returned by hci_connect_sco().
+	 */
+	hci_conn_drop(hcon);
+
 unlock:
 	hci_dev_unlock(hdev);
 	hci_dev_put(hdev);
@@ -495,9 +534,26 @@ static struct sock *sco_get_sock_listen(bdaddr_t *src)
 
 static void sco_sock_destruct(struct sock *sk)
 {
+	struct sco_conn *conn = sco_pi(sk)->conn;
+
 	BT_DBG("sk %p", sk);
 
-	sco_conn_put(sco_pi(sk)->conn);
+	/* If the channel was not already torn down via sco_chan_del(), drop the
+	 * socket's own references here.  sco_chan_del() clears sco_pi(sk)->conn,
+	 * so the hci_conn and sco_conn references are released exactly once.
+	 */
+	if (conn) {
+		struct hci_conn *hcon;
+
+		sco_conn_lock(conn);
+		hcon = conn->hcon;
+		sco_conn_unlock(conn);
+
+		if (hcon)
+			hci_conn_drop(hcon);
+		sco_pi(sk)->conn = NULL;
+		sco_conn_put(conn);
+	}
 
 	skb_queue_purge(&sk->sk_receive_queue);
 	skb_queue_purge(&sk->sk_write_queue);
@@ -1511,12 +1567,10 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status)
 	if (!status) {
 		struct sco_conn *conn;
 
-		conn = sco_conn_add(hci_conn_hold(hcon));
+		conn = sco_conn_add(hcon);
 		if (conn) {
 			sco_conn_ready(conn);
 			sco_conn_put(conn);
-		} else {
-			hci_conn_drop(hcon);
 		}
 	} else
 		sco_conn_del(hcon, bt_to_errno(status));
-- 
2.43.0

RE: Bluetooth: SCO: serialise sco_conn lifetime against sco_recv_scodata()

[bluez.test.bot]

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

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1135493

---Test result---

Test Summary:
CheckPatch                    PASS      0.59 seconds
VerifyFixes                   PASS      0.07 seconds
VerifySignedoff               PASS      0.10 seconds
GitLint                       FAIL      0.20 seconds
SubjectPrefix                 PASS      0.06 seconds
BuildKernel                   PASS      27.52 seconds
CheckAllWarning               PASS      30.22 seconds
CheckSparse                   PASS      28.79 seconds
BuildKernel32                 PASS      26.82 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      507.27 seconds
TestRunner_sco-tester         PASS      32.47 seconds
IncrementalBuild              PASS      26.15 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: SCO: serialise sco_conn lifetime against sco_recv_scodata()

6: B3 Line contains hard tab characters (\t): "	hci_dev_lock(hdev);"
7: B3 Line contains hard tab characters (\t): "	hcon = hci_conn_hash_lookup_handle(hdev, handle);"
8: B3 Line contains hard tab characters (\t): "	..."
9: B3 Line contains hard tab characters (\t): "	conn = sco_conn_hold_unless_zero(hcon->sco_data);"
10: B3 Line contains hard tab characters (\t): "	hci_dev_unlock(hdev);"
15: B3 Line contains hard tab characters (\t): "	conn->hcon->sco_data = NULL;"
20: B3 Line contains hard tab characters (\t): "	BUG: KASAN: slab-use-after-free in sco_conn_hold_unless_zero+0xbe/0x160"
21: B3 Line contains hard tab characters (\t): "	Write of size 4 by task kworker/u17:0"
22: B3 Line contains hard tab characters (\t): "	Workqueue: hci0 hci_rx_work"
23: B3 Line contains hard tab characters (\t): "	Call Trace:"
24: B3 Line contains hard tab characters (\t): "	 sco_conn_hold_unless_zero+0xbe/0x160"
25: B3 Line contains hard tab characters (\t): "	 sco_recv_scodata+0x13f/0x490"
26: B3 Line contains hard tab characters (\t): "	 hci_rx_work+0x3af/0x730"
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


https://github.com/bluez/bluetooth-next/pull/502

---
Regards,
Linux Bluetooth