[PATCH v3] Bluetooth: ISO: fix UAF on socket close before shutdown completes

[PATCH v3] Bluetooth: ISO: fix UAF on socket close before shutdown completes

[Pauli Virtanen]

iso_sock_disconn() aims to disconnect the hcon by dropping it, which
triggers iso_conn_del() to tear down the iso_conn once the HCI operation
completes, which requires valid hcon->iso_data.  iso_sock_disconn() sets
conn->hcon = NULL to avoid a second drop, but also preventing clearing
hcon->iso_data on socket release. Closing the socket before
iso_conn_del() runs then results to UAF.

Fix by using a separate flag to track the hcon drop status, instead of
clearing conn->hcon.

Log: (BlueZ iso-tester ISO Connect Close - Success)
BUG: KASAN: slab-use-after-free in iso_conn_hold_unless_zero
...
 iso_conn_hold_unless_zero (net/bluetooth/iso.c:138)
 iso_conn_del (net/bluetooth/iso.c:270)
 hci_conn_failed (net/bluetooth/hci_conn.c:1408)
 hci_abort_conn_sync (net/bluetooth/hci_sync.c:5817)

Allocated by task 34:
 iso_conn_add (net/bluetooth/iso.c:216)
 iso_connect_cis (net/bluetooth/iso.c:507)
 iso_sock_connect (net/bluetooth/iso.c:1211)
 __sys_connect (net/socket.c:2148)

Freed by task 34:
 iso_chan_del (net/bluetooth/iso.c:248)
 iso_sock_close (net/bluetooth/iso.c:885)
 iso_sock_release (net/bluetooth/iso.c:2022)
 sock_close (net/socket.c:722)

Fixes: fbdc4bc47268 ("Bluetooth: ISO: Use defer setup to separate PA sync and BIG sync")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v3:
    - use test_and_set_bit() instead of lock + bool
    v2:
    - use flag to track dropping instead of detaching sk
      in iso_sock_disconn()
    
    On previous Sashiko comment (IIUC this note text is included in the
    context, maybe generates better review): iso_connect_cfm() with status
    != 0 or iso_disconn_cfm() is guaranteed to be called before the ISO
    hci_conn is deleted. It should then go to iso_conn_del() which is
    supposed to limit the lifetime of the associated iso_conn to be shorter
    than hci_conn.
    
    hci_conn::iso_data is not associated with iso_conn refcount, but
    iso_pi(sk)->conn is supposed to hold one when non-NULL.

 net/bluetooth/iso.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 2e95a153912c..5dc2915f7b32 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -24,8 +24,14 @@ static struct bt_sock_list iso_sk_list = {
 };
 
 /* ---- ISO connections ---- */
+enum {
+	ISO_CONN_DROPPED,
+	__ISO_CONN_NUM_FLAGS
+};
+
 struct iso_conn {
 	struct hci_conn	*hcon;
+	DECLARE_BITMAP(flags, __ISO_CONN_NUM_FLAGS);
 
 	/* @lock: spinlock protecting changes to iso_conn fields */
 	spinlock_t	lock;
@@ -107,7 +113,8 @@ static void iso_conn_free(struct kref *ref)
 
 	if (conn->hcon) {
 		conn->hcon->iso_data = NULL;
-		hci_conn_drop(conn->hcon);
+		if (!test_and_set_bit(ISO_CONN_DROPPED, conn->flags))
+			hci_conn_drop(conn->hcon);
 	}
 
 	/* Ensure no more work items will run since hci_conn has been dropped */
@@ -306,6 +313,7 @@ static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,
 
 	iso_pi(sk)->conn = conn;
 	conn->sk = sk;
+	clear_bit(ISO_CONN_DROPPED, conn->flags);
 
 	if (parent)
 		bt_accept_enqueue(parent, sk, true);
@@ -835,10 +843,8 @@ static void iso_sock_disconn(struct sock *sk)
 	}
 
 	sk->sk_state = BT_DISCONN;
-	iso_conn_lock(iso_pi(sk)->conn);
-	hci_conn_drop(iso_pi(sk)->conn->hcon);
-	iso_pi(sk)->conn->hcon = NULL;
-	iso_conn_unlock(iso_pi(sk)->conn);
+	if (!test_and_set_bit(ISO_CONN_DROPPED, iso_pi(sk)->conn->flags))
+		hci_conn_drop(iso_pi(sk)->conn->hcon);
 }
 
 static void __iso_sock_close(struct sock *sk)
-- 
2.55.0

RE: [v3] Bluetooth: ISO: fix UAF on socket close before shutdown completes

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1835 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=1131484

---Test result---

Test Summary:
CheckPatch                    PASS      0.57 seconds
VerifyFixes                   PASS      0.07 seconds
VerifySignedoff               PASS      0.07 seconds
GitLint                       FAIL      0.21 seconds
SubjectPrefix                 PASS      0.07 seconds
BuildKernel                   PASS      30.03 seconds
CheckAllWarning               PASS      29.52 seconds
CheckSparse                   PASS      28.50 seconds
BuildKernel32                 PASS      26.53 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PENDING   566.96 seconds
TestRunner_iso-tester         PENDING   144.76 seconds
IncrementalBuild              PENDING   61.72 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v3] Bluetooth: ISO: fix UAF on socket close before shutdown completes

42: B2 Line has trailing whitespace: "    "
49: B2 Line has trailing whitespace: "    "
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunnerSetup - PENDING
Desc: Setup kernel and bluez for test-runner
Output:

##############################
Test: TestRunner_iso-tester - PENDING
Desc: Run iso-tester with test-runner
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



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

---
Regards,
Linux Bluetooth