[PATCH] Bluetooth: SCO: avoid deadlock in sco_conn_free()

[Ali Firas <alishmery18@gmail.com>]

sco_conn_free() (the sco_conn kref release) synchronously cancels the
connection timeout work with disable_delayed_work_sync() while the caller
holds lock_sock(sk).  The timeout work, sco_sock_timeout(), itself takes
lock_sock(sk).  When a teardown path drops the connection's last reference
under the socket lock -- e.g. sco_connect_cfm() -> hci_conn_failed() on a
failed link, or a Disconnection Complete via sco_conn_del() -- the ensuing
sco_conn_free() waits for a sco_sock_timeout() that is blocked on the same
socket lock: an AB/BA deadlock.

On a PROVE_LOCKING kernel, lockdep reports:

  WARNING: possible circular locking dependency detected
  kworker/u9:1 is trying to acquire lock:
   ((work_completion)(&(&conn->timeout_work)->work)), at: __flush_work
  but task is already holding lock:
   (sk_lock-AF_BLUETOOTH-BTPROTO_SCO), at: sco_conn_del
  -> #1 (sk_lock-AF_BLUETOOTH-BTPROTO_SCO):
         lock_sock_nested ; sco_sock_timeout ; process_one_work
  -> #0 ((work_completion)(&conn->timeout_work)):
         __flush_work ; disable_delayed_work_sync ; sco_conn_put ;
         sco_chan_del ; sco_conn_del ; sco_connect_cfm

The bug is reachable by an unprivileged local process: opening an
AF_BLUETOOTH/BTPROTO_SCO socket needs no capability, and a short
SO_SNDTIMEO makes the timeout fire during connection setup/teardown.
(syzbot logged the same chain in 2022, closed obsolete with no reproducer.)

The identical problem in the ISO transport was fixed in commit 200fa1629c57
("Bluetooth: ISO: avoid deadlocks in iso_sock_timeout") by moving the
timeout work out of the refcounted, freed-under-lock iso_conn and into
iso_pinfo, so its lifetime follows the socket, and by cancelling it outside
lock_sock().  Do the same for SCO: move timeout_work into struct sco_pinfo,
drop the sync cancel from sco_conn_free(), and add sco_sock_disable_timer()
-- which asserts the socket lock is NOT held -- on the teardown paths
(sco_conn_del(), sco_sock_kill(), sco_sock_close()).

Note that merely making the cancel asynchronous is not sufficient: with
disable_delayed_work() the work can still run against freed memory, which
KASAN reports as a use-after-free in sco_conn_hold_unless_zero().  The
timer's lifetime has to follow the socket, as it now does for ISO.

Reproducer available on request.

Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
Assisted-by: Claude:claude-opus-4-6 [claude-code]
Signed-off-by: Ali Firas <alishmery18@gmail.com>
---
 net/bluetooth/sco.c | 76 +++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 48 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 3d4362a09df4..c87ad72af783 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -46,8 +46,6 @@ struct sco_conn {
 	spinlock_t	lock;
 	struct sock	*sk;
 
-	struct delayed_work	timeout_work;
-
 	unsigned int    mtu;
 	struct kref	ref;
 };
@@ -69,6 +67,7 @@ struct sco_pinfo {
 	__u16		setting;
 	struct bt_codec codec;
 	struct sco_conn	*conn;
+	struct delayed_work	timeout_work;
 };
 
 /* ---- SCO timers ---- */
@@ -89,9 +88,6 @@ static void sco_conn_free(struct kref *ref)
 		hci_conn_drop(conn->hcon);
 	}
 
-	/* Ensure no more work items will run since hci_conn has been dropped */
-	disable_delayed_work_sync(&conn->timeout_work);
-
 	kfree(conn);
 }
 
@@ -138,53 +134,45 @@ static struct sock *sco_sock_hold(struct sco_conn *conn)
 
 static void sco_sock_timeout(struct work_struct *work)
 {
-	struct sco_conn *conn = container_of(work, struct sco_conn,
-					     timeout_work.work);
-	struct sock *sk;
-
-	conn = sco_conn_hold_unless_zero(conn);
-	if (!conn)
-		return;
-
-	sco_conn_lock(conn);
-	if (!conn->hcon) {
-		sco_conn_unlock(conn);
-		sco_conn_put(conn);
-		return;
-	}
-	sk = sco_sock_hold(conn);
-	sco_conn_unlock(conn);
-	sco_conn_put(conn);
-
-	if (!sk)
-		return;
+	struct sco_pinfo *pi = container_of(work, struct sco_pinfo,
+					    timeout_work.work);
+	struct sock *sk = &pi->bt.sk;
 
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
 
 	lock_sock(sk);
-	sk->sk_err = ETIMEDOUT;
-	sk->sk_state_change(sk);
+	if (!sock_flag(sk, SOCK_ZAPPED)) {
+		sk->sk_err = ETIMEDOUT;
+		sk->sk_state_change(sk);
+	}
 	release_sock(sk);
-	sock_put(sk);
 }
 
 static void sco_sock_set_timer(struct sock *sk, long timeout)
 {
+	lockdep_assert(lockdep_sock_is_held(sk));
+
+	cancel_delayed_work(&sco_pi(sk)->timeout_work);
+
 	if (!sco_pi(sk)->conn)
 		return;
 
 	BT_DBG("sock %p state %d timeout %ld", sk, sk->sk_state, timeout);
-	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
-	schedule_delayed_work(&sco_pi(sk)->conn->timeout_work, timeout);
+	schedule_delayed_work(&sco_pi(sk)->timeout_work, timeout);
 }
 
 static void sco_sock_clear_timer(struct sock *sk)
 {
-	if (!sco_pi(sk)->conn)
-		return;
+	BT_DBG("sock %p state %d", sk, sk->sk_state);
+	cancel_delayed_work(&sco_pi(sk)->timeout_work);
+}
+
+static void sco_sock_disable_timer(struct sock *sk)
+{
+	lockdep_assert(!lockdep_sock_is_held(sk));
 
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
-	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
+	disable_delayed_work_sync(&sco_pi(sk)->timeout_work);
 }
 
 /* ---- SCO connections ---- */
@@ -214,7 +202,6 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 
 	kref_init(&conn->ref);
 	spin_lock_init(&conn->lock);
-	INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
 
 	hcon->sco_data = conn;
 	conn->hcon = hcon;
@@ -274,9 +261,10 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
 	if (!sk)
 		return;
 
+	sco_sock_disable_timer(sk);
+
 	/* Kill socket */
 	lock_sock(sk);
-	sco_sock_clear_timer(sk);
 	sco_chan_del(sk, err);
 	release_sock(sk);
 	sock_put(sk);
@@ -532,6 +520,8 @@ static void sco_sock_kill(struct sock *sk)
 
 	BT_DBG("sk %p state %d", sk, sk->sk_state);
 
+	sco_sock_disable_timer(sk);
+
 	/* Sock is dead, so set conn->sk to NULL to avoid possible UAF */
 	lock_sock(sk);
 	if (sco_pi(sk)->conn) {
@@ -574,23 +564,11 @@ static void __sco_sock_close(struct sock *sk)
 /* Must be called on unlocked socket. */
 static void sco_sock_close(struct sock *sk)
 {
-	struct sco_conn *conn;
-
-	lock_sock(sk);
-	conn = sco_pi(sk)->conn;
-	if (conn)
-		sco_conn_hold(conn);
-	release_sock(sk);
-
-	if (conn)
-		disable_delayed_work_sync(&conn->timeout_work);
+	sco_sock_disable_timer(sk);
 
 	lock_sock(sk);
 	__sco_sock_close(sk);
 	release_sock(sk);
-
-	if (conn)
-		sco_conn_put(conn);
 }
 
 static void sco_sock_init(struct sock *sk, struct sock *parent)
@@ -622,6 +600,8 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,
 	sk->sk_destruct = sco_sock_destruct;
 	sk->sk_sndtimeo = SCO_CONN_TIMEOUT;
 
+	INIT_DELAYED_WORK(&sco_pi(sk)->timeout_work, sco_sock_timeout);
+
 	sco_pi(sk)->setting = BT_VOICE_CVSD_16BIT;
 	sco_pi(sk)->codec.id = BT_CODEC_CVSD;
 	sco_pi(sk)->codec.cid = 0xffff;
-- 
2.53.0