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

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

[Ali Firas]

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

RE: Bluetooth: SCO: avoid deadlock in sco_conn_free()

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      0.70 seconds
VerifyFixes                   PASS      0.07 seconds
VerifySignedoff               PASS      0.07 seconds
GitLint                       PASS      0.20 seconds
SubjectPrefix                 PASS      0.06 seconds
BuildKernel                   PASS      27.72 seconds
CheckAllWarning               PASS      30.45 seconds
CheckSparse                   PASS      28.92 seconds
BuildKernel32                 PASS      26.52 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      510.39 seconds
TestRunner_sco-tester         PASS      34.57 seconds
IncrementalBuild              PASS      26.14 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


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

---
Regards,
Linux Bluetooth