[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@xxxxxx>
---

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