[PATCH v6 0/6] Bluetooth: fix locking and socket killing in SCO and RFCOMM

[PATCH v6 0/6] Bluetooth: fix locking and socket killing in SCO and RFCOMM

[Desmond Cheong Zhi Xi]

Hi,

This patch series started out as a fix for "inconsistent lock state in
sco_sock_timeout" reported by Syzbot [1].

Patch 1 is sufficient to fix this error. This was also confirmed by the
reproducer for "BUG: corrupted list in kobject_add_internal (3)" [2]
which consistently hits the inconsistent lock state error.

However, while testing the proposed fix, the reproducer for [1] would
randomly return a human-unreadable error [3]. After further
investigation, this bug seems to be caused by an unrelated error with
forking [4].

While trying to fix the mysterious error, additional fixes were added,
such as switching to lock_sock and serializing _{set,clear}_timer.

Additionally, as the reproducer kept hitting the oom-killer, a fix for
SCO socket killing was also added.

The reproducer for [1] was robust enough to catch errors with these
additional fixes, hence all the patches in this series were squashed then
tested with the reproducer for [1].

Overall, this series makes the following changes:

- Patch 1: Schedule SCO sock timeouts with delayed_work to avoid
inconsistent lock usage (removes SOFTIRQs from SCO)

- Patch 2: Avoid a circular dependency between hci_dev_lock and
lock_sock (enables the switch to lock_sock)

- Patch 3: Switch to lock_sock in SCO now that SOFTIRQs and potential
deadlocks are removed

- Patch 4: Serialize calls to sco_sock_{set,clear}_timer

- Patch 5: Switch to lock_sock in RFCOMM

- Patch 6: fix SCO socket killing

v5 -> v6:
- Removed hard tab characters from patch 2's commit message. As
suggested by the Bluez test bot.
- Removed unnecessary dedicated variables for struct delayed_work* in
sco_sock_{set,clear}_timer as suggested by Luiz Augusto von Dentz.

v4 -> v5:
- Renamed the delayed_work variable, moved checks for sco_pi(sk)->conn
into sco_sock_{clear,set}_timer, as suggested by Luiz Augusto von Dentz
and Marcel Holtmann.
- Added check for conn->sk in sco_sock_timeout, accompanied by a
sock_hold to avoid UAF errors.
- Added check to flush work items before freeing conn.
- Avoid a circular dependency between hci_dev_lock and lock_sock.
- Switch to lock_sock in SCO, as suggested by Marcel Holtmann.
- Serial calls to sco_sock_{set,clear}_timer.
- Switch to lock_sock in RFCOMM, as suggested by Marcel Holtmann.
- Add a fix for SCO socket killing.

v3 -> v4:
- Switch to using delayed_work to schedule SCO sock timeouts instead
of using local_bh_disable. As suggested by Luiz Augusto von Dentz.

v2 -> v3:
- Split SCO and RFCOMM code changes, as suggested by Luiz Augusto von
Dentz.
- Simplify local bh disabling in SCO by using local_bh_disable/enable
inside sco_chan_del since local_bh_disable/enable pairs are reentrant.

v1 -> v2:
- Instead of pulling out the clean-up code out from sco_chan_del and
using it directly in sco_conn_del, disable local softirqs for relevant
sections.
- Disable local softirqs more thoroughly for instances of
bh_lock_sock/bh_lock_sock_nested in the bluetooth subsystem.
Specifically, the calls in af_bluetooth.c and rfcomm/sock.c are now made
with local softirqs disabled as well.

Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]

Link: https://syzkaller.appspot.com/bug?extid=66264bf2fd0476be7e6c [2]

Link: https://syzkaller.appspot.com/text?tag=CrashReport&x=172d819a300000 [3]

Link: https://syzkaller.appspot.com/bug?id=e1bf7ba90d8dafcf318666192aba1cfd65507377 [4]

Best wishes,
Desmond

Desmond Cheong Zhi Xi (6):
  Bluetooth: schedule SCO timeouts with delayed_work
  Bluetooth: avoid circular locks in sco_sock_connect
  Bluetooth: switch to lock_sock in SCO
  Bluetooth: serialize calls to sco_sock_{set,clear}_timer
  Bluetooth: switch to lock_sock in RFCOMM
  Bluetooth: fix repeated calls to sco_sock_kill

 net/bluetooth/rfcomm/sock.c |   8 +--
 net/bluetooth/sco.c         | 101 ++++++++++++++++++++----------------
 2 files changed, 60 insertions(+), 49 deletions(-)

-- 
2.25.1

[PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work

[Desmond Cheong Zhi Xi]

struct sock.sk_timer should be used as a sock cleanup timer. However,
SCO uses it to implement sock timeouts.

This causes issues because struct sock.sk_timer's callback is run in
an IRQ context, and the timer callback function sco_sock_timeout takes
a spin lock on the socket. However, other functions such as
sco_conn_del and sco_conn_ready take the spin lock with interrupts
enabled.

This inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock usage could
lead to deadlocks as reported by Syzbot [1]:
       CPU0
       ----
  lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
  <Interrupt>
    lock(slock-AF_BLUETOOTH-BTPROTO_SCO);

To fix this, we use delayed work to implement SCO sock timouts
instead. This allows us to avoid taking the spin lock on the socket in
an IRQ context, and corrects the misuse of struct sock.sk_timer.

As a note, cancel_delayed_work is used instead of
cancel_delayed_work_sync in sco_sock_set_timer and
sco_sock_clear_timer to avoid a deadlock. In the future, the call to
bh_lock_sock inside sco_sock_timeout should be changed to lock_sock to
synchronize with other functions using lock_sock. However, since
sco_sock_set_timer and sco_sock_clear_timer are sometimes called under
the locked socket (in sco_connect and __sco_sock_close),
cancel_delayed_work_sync might cause them to sleep until an
sco_sock_timeout that has started finishes running. But
sco_sock_timeout would also sleep until it can grab the lock_sock.

Using cancel_delayed_work is fine because sco_sock_timeout does not
change from run to run, hence there is no functional difference
between:
1. waiting for a timeout to finish running before scheduling another
timeout
2. scheduling another timeout while a timeout is running.

Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Tested-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 net/bluetooth/sco.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index ffa2a77a3e4c..62e638f971a9 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -48,6 +48,8 @@ struct sco_conn {
 	spinlock_t	lock;
 	struct sock	*sk;
 
+	struct delayed_work	timeout_work;
+
 	unsigned int    mtu;
 };
 
@@ -74,9 +76,20 @@ struct sco_pinfo {
 #define SCO_CONN_TIMEOUT	(HZ * 40)
 #define SCO_DISCONN_TIMEOUT	(HZ * 2)
 
-static void sco_sock_timeout(struct timer_list *t)
+static void sco_sock_timeout(struct work_struct *work)
 {
-	struct sock *sk = from_timer(sk, t, sk_timer);
+	struct sco_conn *conn = container_of(work, struct sco_conn,
+					     timeout_work.work);
+	struct sock *sk;
+
+	sco_conn_lock(conn);
+	sk = conn->sk;
+	if (sk)
+		sock_hold(sk);
+	sco_conn_unlock(conn);
+
+	if (!sk)
+		return;
 
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
 
@@ -91,14 +104,21 @@ static void sco_sock_timeout(struct timer_list *t)
 
 static void sco_sock_set_timer(struct sock *sk, long timeout)
 {
+	if (!sco_pi(sk)->conn)
+		return;
+
 	BT_DBG("sock %p state %d timeout %ld", sk, sk->sk_state, timeout);
-	sk_reset_timer(sk, &sk->sk_timer, jiffies + timeout);
+	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
+	schedule_delayed_work(&sco_pi(sk)->conn->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);
-	sk_stop_timer(sk, &sk->sk_timer);
+	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
 }
 
 /* ---- SCO connections ---- */
@@ -179,6 +199,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
 		bh_unlock_sock(sk);
 		sco_sock_kill(sk);
 		sock_put(sk);
+
+		/* Ensure no more work items will run before freeing conn. */
+		cancel_delayed_work_sync(&conn->timeout_work);
 	}
 
 	hcon->sco_data = NULL;
@@ -193,6 +216,8 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
 	sco_pi(sk)->conn = conn;
 	conn->sk = sk;
 
+	INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
+
 	if (parent)
 		bt_accept_enqueue(parent, sk, true);
 }
@@ -500,8 +525,6 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,
 
 	sco_pi(sk)->setting = BT_VOICE_CVSD_16BIT;
 
-	timer_setup(&sk->sk_timer, sco_sock_timeout, 0);
-
 	bt_sock_link(&sco_sk_list, sk);
 	return sk;
 }
-- 
2.25.1

[PATCH v6 2/6] Bluetooth: avoid circular locks in sco_sock_connect

[Desmond Cheong Zhi Xi]

In a future patch, calls to bh_lock_sock in sco.c should be replaced
by lock_sock now that none of the functions are run in IRQ context.

However, doing so results in a circular locking dependency:

======================================================
WARNING: possible circular locking dependency detected
5.14.0-rc4-syzkaller #0 Not tainted
------------------------------------------------------
syz-executor.2/14867 is trying to acquire lock:
ffff88803e3c1120 (sk_lock-AF_BLUETOOTH-BTPROTO_SCO){+.+.}-{0:0}, at:
lock_sock include/net/sock.h:1613 [inline]
ffff88803e3c1120 (sk_lock-AF_BLUETOOTH-BTPROTO_SCO){+.+.}-{0:0}, at:
sco_conn_del+0x12a/0x2a0 net/bluetooth/sco.c:191

but task is already holding lock:
ffffffff8d2dc7c8 (hci_cb_list_lock){+.+.}-{3:3}, at:
hci_disconn_cfm include/net/bluetooth/hci_core.h:1497 [inline]
ffffffff8d2dc7c8 (hci_cb_list_lock){+.+.}-{3:3}, at:
hci_conn_hash_flush+0xda/0x260 net/bluetooth/hci_conn.c:1608

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #2 (hci_cb_list_lock){+.+.}-{3:3}:
       __mutex_lock_common kernel/locking/mutex.c:959 [inline]
       __mutex_lock+0x12a/0x10a0 kernel/locking/mutex.c:1104
       hci_connect_cfm include/net/bluetooth/hci_core.h:1482 [inline]
       hci_remote_features_evt net/bluetooth/hci_event.c:3263 [inline]
       hci_event_packet+0x2f4d/0x7c50 net/bluetooth/hci_event.c:6240
       hci_rx_work+0x4f8/0xd30 net/bluetooth/hci_core.c:5122
       process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
       worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
       kthread+0x3e5/0x4d0 kernel/kthread.c:319
       ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

-> #1 (&hdev->lock){+.+.}-{3:3}:
       __mutex_lock_common kernel/locking/mutex.c:959 [inline]
       __mutex_lock+0x12a/0x10a0 kernel/locking/mutex.c:1104
       sco_connect net/bluetooth/sco.c:245 [inline]
       sco_sock_connect+0x227/0xa10 net/bluetooth/sco.c:601
       __sys_connect_file+0x155/0x1a0 net/socket.c:1879
       __sys_connect+0x161/0x190 net/socket.c:1896
       __do_sys_connect net/socket.c:1906 [inline]
       __se_sys_connect net/socket.c:1903 [inline]
       __x64_sys_connect+0x6f/0xb0 net/socket.c:1903
       do_syscall_x64 arch/x86/entry/common.c:50 [inline]
       do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
       entry_SYSCALL_64_after_hwframe+0x44/0xae

-> #0 (sk_lock-AF_BLUETOOTH-BTPROTO_SCO){+.+.}-{0:0}:
       check_prev_add kernel/locking/lockdep.c:3051 [inline]
       check_prevs_add kernel/locking/lockdep.c:3174 [inline]
       validate_chain kernel/locking/lockdep.c:3789 [inline]
       __lock_acquire+0x2a07/0x54a0 kernel/locking/lockdep.c:5015
       lock_acquire kernel/locking/lockdep.c:5625 [inline]
       lock_acquire+0x1ab/0x510 kernel/locking/lockdep.c:5590
       lock_sock_nested+0xca/0x120 net/core/sock.c:3170
       lock_sock include/net/sock.h:1613 [inline]
       sco_conn_del+0x12a/0x2a0 net/bluetooth/sco.c:191
       sco_disconn_cfm+0x71/0xb0 net/bluetooth/sco.c:1202
       hci_disconn_cfm include/net/bluetooth/hci_core.h:1500 [inline]
       hci_conn_hash_flush+0x127/0x260 net/bluetooth/hci_conn.c:1608
       hci_dev_do_close+0x528/0x1130 net/bluetooth/hci_core.c:1778
       hci_unregister_dev+0x1c0/0x5a0 net/bluetooth/hci_core.c:4015
       vhci_release+0x70/0xe0 drivers/bluetooth/hci_vhci.c:340
       __fput+0x288/0x920 fs/file_table.c:280
       task_work_run+0xdd/0x1a0 kernel/task_work.c:164
       exit_task_work include/linux/task_work.h:32 [inline]
       do_exit+0xbd4/0x2a60 kernel/exit.c:825
       do_group_exit+0x125/0x310 kernel/exit.c:922
       get_signal+0x47f/0x2160 kernel/signal.c:2808
       arch_do_signal_or_restart+0x2a9/0x1c40 arch/x86/kernel/signal.c:865
       handle_signal_work kernel/entry/common.c:148 [inline]
       exit_to_user_mode_loop kernel/entry/common.c:172 [inline]
       exit_to_user_mode_prepare+0x17d/0x290 kernel/entry/common.c:209
       __syscall_exit_to_user_mode_work kernel/entry/common.c:291 [inline]
       syscall_exit_to_user_mode+0x19/0x60 kernel/entry/common.c:302
       ret_from_fork+0x15/0x30 arch/x86/entry/entry_64.S:288

other info that might help us debug this:

Chain exists of:
  sk_lock-AF_BLUETOOTH-BTPROTO_SCO --> &hdev->lock --> hci_cb_list_lock

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(hci_cb_list_lock);
                               lock(&hdev->lock);
                               lock(hci_cb_list_lock);
  lock(sk_lock-AF_BLUETOOTH-BTPROTO_SCO);

 *** DEADLOCK ***

The issue is that the lock hierarchy should go from &hdev->lock -->
hci_cb_list_lock --> sk_lock-AF_BLUETOOTH-BTPROTO_SCO. For example,
one such call trace is:

  hci_dev_do_close():
    hci_dev_lock();
    hci_conn_hash_flush():
      hci_disconn_cfm():
        mutex_lock(&hci_cb_list_lock);
        sco_disconn_cfm():
        sco_conn_del():
          lock_sock(sk);

However, in sco_sock_connect, we call lock_sock before calling
hci_dev_lock inside sco_connect, thus inverting the lock hierarchy.

We fix this by pulling the call to hci_dev_lock out from sco_connect.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 net/bluetooth/sco.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 62e638f971a9..94a3aa686556 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -237,44 +237,32 @@ static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
 	return err;
 }
 
-static int sco_connect(struct sock *sk)
+static int sco_connect(struct hci_dev *hdev, struct sock *sk)
 {
 	struct sco_conn *conn;
 	struct hci_conn *hcon;
-	struct hci_dev  *hdev;
 	int err, type;
 
 	BT_DBG("%pMR -> %pMR", &sco_pi(sk)->src, &sco_pi(sk)->dst);
 
-	hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src, BDADDR_BREDR);
-	if (!hdev)
-		return -EHOSTUNREACH;
-
-	hci_dev_lock(hdev);
-
 	if (lmp_esco_capable(hdev) && !disable_esco)
 		type = ESCO_LINK;
 	else
 		type = SCO_LINK;
 
 	if (sco_pi(sk)->setting == BT_VOICE_TRANSPARENT &&
-	    (!lmp_transp_capable(hdev) || !lmp_esco_capable(hdev))) {
-		err = -EOPNOTSUPP;
-		goto done;
-	}
+	    (!lmp_transp_capable(hdev) || !lmp_esco_capable(hdev)))
+		return -EOPNOTSUPP;
 
 	hcon = hci_connect_sco(hdev, type, &sco_pi(sk)->dst,
 			       sco_pi(sk)->setting);
-	if (IS_ERR(hcon)) {
-		err = PTR_ERR(hcon);
-		goto done;
-	}
+	if (IS_ERR(hcon))
+		return PTR_ERR(hcon);
 
 	conn = sco_conn_add(hcon);
 	if (!conn) {
 		hci_conn_drop(hcon);
-		err = -ENOMEM;
-		goto done;
+		return -ENOMEM;
 	}
 
 	/* Update source addr of the socket */
@@ -282,7 +270,7 @@ static int sco_connect(struct sock *sk)
 
 	err = sco_chan_add(conn, sk, NULL);
 	if (err)
-		goto done;
+		return err;
 
 	if (hcon->state == BT_CONNECTED) {
 		sco_sock_clear_timer(sk);
@@ -292,9 +280,6 @@ static int sco_connect(struct sock *sk)
 		sco_sock_set_timer(sk, sk->sk_sndtimeo);
 	}
 
-done:
-	hci_dev_unlock(hdev);
-	hci_dev_put(hdev);
 	return err;
 }
 
@@ -589,6 +574,7 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen
 {
 	struct sockaddr_sco *sa = (struct sockaddr_sco *) addr;
 	struct sock *sk = sock->sk;
+	struct hci_dev  *hdev;
 	int err;
 
 	BT_DBG("sk %p", sk);
@@ -603,12 +589,19 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen
 	if (sk->sk_type != SOCK_SEQPACKET)
 		return -EINVAL;
 
+	hdev = hci_get_route(&sa->sco_bdaddr, &sco_pi(sk)->src, BDADDR_BREDR);
+	if (!hdev)
+		return -EHOSTUNREACH;
+	hci_dev_lock(hdev);
+
 	lock_sock(sk);
 
 	/* Set destination address and psm */
 	bacpy(&sco_pi(sk)->dst, &sa->sco_bdaddr);
 
-	err = sco_connect(sk);
+	err = sco_connect(hdev, sk);
+	hci_dev_unlock(hdev);
+	hci_dev_put(hdev);
 	if (err)
 		goto done;
 
-- 
2.25.1

[PATCH v6 3/6] Bluetooth: switch to lock_sock in SCO

[Desmond Cheong Zhi Xi]

Since sco_sock_timeout is now scheduled using delayed work, it is no
longer run in SOFTIRQ context. Hence bh_lock_sock is no longer
necessary in SCO to synchronise between user contexts and SOFTIRQ
processing.

As such, calls to bh_lock_sock should be replaced with lock_sock to
synchronize with other concurrent processes that use lock_sock.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 net/bluetooth/sco.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 94a3aa686556..68b51e321e82 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -93,10 +93,10 @@ static void sco_sock_timeout(struct work_struct *work)
 
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
 
-	bh_lock_sock(sk);
+	lock_sock(sk);
 	sk->sk_err = ETIMEDOUT;
 	sk->sk_state_change(sk);
-	bh_unlock_sock(sk);
+	release_sock(sk);
 
 	sco_sock_kill(sk);
 	sock_put(sk);
@@ -193,10 +193,10 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
 
 	if (sk) {
 		sock_hold(sk);
-		bh_lock_sock(sk);
+		lock_sock(sk);
 		sco_sock_clear_timer(sk);
 		sco_chan_del(sk, err);
-		bh_unlock_sock(sk);
+		release_sock(sk);
 		sco_sock_kill(sk);
 		sock_put(sk);
 
@@ -1105,10 +1105,10 @@ static void sco_conn_ready(struct sco_conn *conn)
 
 	if (sk) {
 		sco_sock_clear_timer(sk);
-		bh_lock_sock(sk);
+		lock_sock(sk);
 		sk->sk_state = BT_CONNECTED;
 		sk->sk_state_change(sk);
-		bh_unlock_sock(sk);
+		release_sock(sk);
 	} else {
 		sco_conn_lock(conn);
 
@@ -1123,12 +1123,12 @@ static void sco_conn_ready(struct sco_conn *conn)
 			return;
 		}
 
-		bh_lock_sock(parent);
+		lock_sock(parent);
 
 		sk = sco_sock_alloc(sock_net(parent), NULL,
 				    BTPROTO_SCO, GFP_ATOMIC, 0);
 		if (!sk) {
-			bh_unlock_sock(parent);
+			release_sock(parent);
 			sco_conn_unlock(conn);
 			return;
 		}
@@ -1149,7 +1149,7 @@ static void sco_conn_ready(struct sco_conn *conn)
 		/* Wake up parent */
 		parent->sk_data_ready(parent);
 
-		bh_unlock_sock(parent);
+		release_sock(parent);
 
 		sco_conn_unlock(conn);
 	}
-- 
2.25.1

[PATCH v6 4/6] Bluetooth: serialize calls to sco_sock_{set,clear}_timer

[Desmond Cheong Zhi Xi]

Currently, calls to sco_sock_set_timer are made under the locked
socket, but this does not apply to all calls to sco_sock_clear_timer.

Both sco_sock_{set,clear}_timer should be serialized by lock_sock to
prevent unexpected concurrent clearing/setting of timers.

Additionally, since sco_pi(sk)->conn is only cleared under the locked
socket, this change allows us to avoid races between
sco_sock_clear_timer and the call to kfree(conn) in sco_conn_del.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 net/bluetooth/sco.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 68b51e321e82..77490338f4fa 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -453,8 +453,8 @@ static void __sco_sock_close(struct sock *sk)
 /* Must be called on unlocked socket. */
 static void sco_sock_close(struct sock *sk)
 {
-	sco_sock_clear_timer(sk);
 	lock_sock(sk);
+	sco_sock_clear_timer(sk);
 	__sco_sock_close(sk);
 	release_sock(sk);
 	sco_sock_kill(sk);
@@ -1104,8 +1104,8 @@ static void sco_conn_ready(struct sco_conn *conn)
 	BT_DBG("conn %p", conn);
 
 	if (sk) {
-		sco_sock_clear_timer(sk);
 		lock_sock(sk);
+		sco_sock_clear_timer(sk);
 		sk->sk_state = BT_CONNECTED;
 		sk->sk_state_change(sk);
 		release_sock(sk);
-- 
2.25.1

[PATCH v6 5/6] Bluetooth: switch to lock_sock in RFCOMM

[Desmond Cheong Zhi Xi]

Other than rfcomm_sk_state_change and rfcomm_connect_ind, functions in
RFCOMM use lock_sock to lock the socket.

Since bh_lock_sock and spin_lock_bh do not provide synchronization
with lock_sock, these calls should be changed to lock_sock.

This is now safe to do because packet processing is now done in a
workqueue instead of a tasklet, so bh_lock_sock/spin_lock_bh are no
longer necessary to synchronise between user contexts and SOFTIRQ
processing.

Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 net/bluetooth/rfcomm/sock.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index ae6f80730561..2c95bb58f901 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -70,7 +70,7 @@ static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
 
 	BT_DBG("dlc %p state %ld err %d", d, d->state, err);
 
-	spin_lock_bh(&sk->sk_lock.slock);
+	lock_sock(sk);
 
 	if (err)
 		sk->sk_err = err;
@@ -91,7 +91,7 @@ static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
 		sk->sk_state_change(sk);
 	}
 
-	spin_unlock_bh(&sk->sk_lock.slock);
+	release_sock(sk);
 
 	if (parent && sock_flag(sk, SOCK_ZAPPED)) {
 		/* We have to drop DLC lock here, otherwise
@@ -974,7 +974,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 	if (!parent)
 		return 0;
 
-	bh_lock_sock(parent);
+	lock_sock(parent);
 
 	/* Check for backlog size */
 	if (sk_acceptq_is_full(parent)) {
@@ -1001,7 +1001,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 	result = 1;
 
 done:
-	bh_unlock_sock(parent);
+	release_sock(parent);
 
 	if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags))
 		parent->sk_state_change(parent);
-- 
2.25.1

[PATCH v6 6/6] Bluetooth: fix repeated calls to sco_sock_kill

[Desmond Cheong Zhi Xi]

In commit 4e1a720d0312 ("Bluetooth: avoid killing an already killed
socket"), a check was added to sco_sock_kill to skip killing a socket
if the SOCK_DEAD flag was set.

This was done after a trace for a use-after-free bug showed that the
same sock pointer was being killed twice.

Unfortunately, this check prevents sco_sock_kill from running on any
socket. sco_sock_kill kills a socket only if it's zapped and orphaned,
however sock_orphan announces that the socket is dead before detaching
it. i.e., orphaned sockets have the SOCK_DEAD flag set.

To fix this, we remove the check for SOCK_DEAD, and avoid repeated
calls to sco_sock_kill by removing incorrect calls in:

1. sco_sock_timeout. The socket should not be killed on timeout as
further processing is expected to be done. For example,
sco_sock_connect sets the timer then waits for the socket to be
connected or for an error to be returned.

2. sco_conn_del. This function should clean up resources for the
connection, but the socket itself should be cleaned up in
sco_sock_release.

3. sco_sock_close. Calls to sco_sock_close in sco_sock_cleanup_listen
and sco_sock_release are followed by sco_sock_kill. Hence the
duplicated call should be removed.

Fixes: 4e1a720d0312 ("Bluetooth: avoid killing an already killed socket")
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
---
 net/bluetooth/sco.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 77490338f4fa..98a881586512 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -97,8 +97,6 @@ static void sco_sock_timeout(struct work_struct *work)
 	sk->sk_err = ETIMEDOUT;
 	sk->sk_state_change(sk);
 	release_sock(sk);
-
-	sco_sock_kill(sk);
 	sock_put(sk);
 }
 
@@ -197,7 +195,6 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
 		sco_sock_clear_timer(sk);
 		sco_chan_del(sk, err);
 		release_sock(sk);
-		sco_sock_kill(sk);
 		sock_put(sk);
 
 		/* Ensure no more work items will run before freeing conn. */
@@ -404,8 +401,7 @@ static void sco_sock_cleanup_listen(struct sock *parent)
  */
 static void sco_sock_kill(struct sock *sk)
 {
-	if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket ||
-	    sock_flag(sk, SOCK_DEAD))
+	if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket)
 		return;
 
 	BT_DBG("sk %p state %d", sk, sk->sk_state);
@@ -457,7 +453,6 @@ static void sco_sock_close(struct sock *sk)
 	sco_sock_clear_timer(sk);
 	__sco_sock_close(sk);
 	release_sock(sk);
-	sco_sock_kill(sk);
 }
 
 static void sco_skb_put_cmsg(struct sk_buff *skb, struct msghdr *msg,
-- 
2.25.1

Re: [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work

[Eric Dumazet]

On 8/9/21 9:14 PM, Desmond Cheong Zhi Xi wrote:
> struct sock.sk_timer should be used as a sock cleanup timer. However,
> SCO uses it to implement sock timeouts.
> 
> This causes issues because struct sock.sk_timer's callback is run in
> an IRQ context, and the timer callback function sco_sock_timeout takes
> a spin lock on the socket. However, other functions such as
> sco_conn_del and sco_conn_ready take the spin lock with interrupts
> enabled.
> 
> This inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock usage could
> lead to deadlocks as reported by Syzbot [1]:
>        CPU0
>        ----
>   lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
>   <Interrupt>
>     lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
> 
> To fix this, we use delayed work to implement SCO sock timouts
> instead. This allows us to avoid taking the spin lock on the socket in
> an IRQ context, and corrects the misuse of struct sock.sk_timer.
> 
> As a note, cancel_delayed_work is used instead of
> cancel_delayed_work_sync in sco_sock_set_timer and
> sco_sock_clear_timer to avoid a deadlock. In the future, the call to
> bh_lock_sock inside sco_sock_timeout should be changed to lock_sock to
> synchronize with other functions using lock_sock. However, since
> sco_sock_set_timer and sco_sock_clear_timer are sometimes called under
> the locked socket (in sco_connect and __sco_sock_close),
> cancel_delayed_work_sync might cause them to sleep until an
> sco_sock_timeout that has started finishes running. But
> sco_sock_timeout would also sleep until it can grab the lock_sock.
> 
> Using cancel_delayed_work is fine because sco_sock_timeout does not
> change from run to run, hence there is no functional difference
> between:
> 1. waiting for a timeout to finish running before scheduling another
> timeout
> 2. scheduling another timeout while a timeout is running.
> 
> Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
> Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
> Tested-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
>  net/bluetooth/sco.c | 35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index ffa2a77a3e4c..62e638f971a9 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -48,6 +48,8 @@ struct sco_conn {
>  	spinlock_t	lock;
>  	struct sock	*sk;
>  
> +	struct delayed_work	timeout_work;
> +
>  	unsigned int    mtu;
>  };
>  
> @@ -74,9 +76,20 @@ struct sco_pinfo {
>  #define SCO_CONN_TIMEOUT	(HZ * 40)
>  #define SCO_DISCONN_TIMEOUT	(HZ * 2)
>  
> -static void sco_sock_timeout(struct timer_list *t)
> +static void sco_sock_timeout(struct work_struct *work)
>  {
> -	struct sock *sk = from_timer(sk, t, sk_timer);
> +	struct sco_conn *conn = container_of(work, struct sco_conn,
> +					     timeout_work.work);
> +	struct sock *sk;
> +
> +	sco_conn_lock(conn);
> +	sk = conn->sk;
> +	if (sk)
> +		sock_hold(sk);

syzbot complains here that sk refcount can be zero at this time.

refcount_t: addition on 0; use-after-free.
WARNING: CPU: 0 PID: 10451 at lib/refcount.c:25 refcount_warn_saturate+0x169/0x1e0 lib/refcount.c:25
Modules linked in:
CPU: 0 PID: 10451 Comm: kworker/0:8 Not tainted 5.14.0-rc7-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: events sco_sock_timeout
RIP: 0010:refcount_warn_saturate+0x169/0x1e0 lib/refcount.c:25
Code: 09 31 ff 89 de e8 d7 c9 9e fd 84 db 0f 85 36 ff ff ff e8 8a c3 9e fd 48 c7 c7 20 8f e3 89 c6 05 e8 7f 81 09 01 e8 f0 98 16 05 <0f> 0b e9 17 ff ff ff e8 6b c3 9e fd 0f b6 1d cd 7f 81 09 31 ff 89
RSP: 0018:ffffc9001766fce8 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff88802cea3880 RSI: ffffffff815d87a5 RDI: fffff52002ecdf8f
RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
R10: ffffffff815d25de R11: 0000000000000000 R12: ffff88806d23ce08
R13: ffff8880712c8080 R14: ffff88802edf4500 R15: ffff8880b9c51240
FS:  0000000000000000(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f3748c20000 CR3: 0000000017644000 CR4: 00000000001506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 __refcount_add include/linux/refcount.h:199 [inline]
 __refcount_inc include/linux/refcount.h:250 [inline]
 refcount_inc include/linux/refcount.h:267 [inline]
 sock_hold include/net/sock.h:702 [inline]
 sco_sock_timeout+0x216/0x290 net/bluetooth/sco.c:88
 process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
 worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
 kthread+0x3e5/0x4d0 kernel/kthread.c:319


> +	sco_conn_unlock(conn);
> +
> +	if (!sk)
> +		return;
>  
>  	BT_DBG("sock %p state %d", sk, sk->sk_state);
>  
> @@ -91,14 +104,21 @@ static void sco_sock_timeout(struct timer_list *t)
>  
>  static void sco_sock_set_timer(struct sock *sk, long timeout)
>  {
> +	if (!sco_pi(sk)->conn)
> +		return;
> +
>  	BT_DBG("sock %p state %d timeout %ld", sk, sk->sk_state, timeout);
> -	sk_reset_timer(sk, &sk->sk_timer, jiffies + timeout);
> +	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
> +	schedule_delayed_work(&sco_pi(sk)->conn->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);
> -	sk_stop_timer(sk, &sk->sk_timer);
> +	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);


>  }
>  
>  /* ---- SCO connections ---- */
> @@ -179,6 +199,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>  		bh_unlock_sock(sk);
>  		sco_sock_kill(sk);
>  		sock_put(sk);
> +
> +		/* Ensure no more work items will run before freeing conn. */

Maybe you should have done this cancel_delayed_work_sync() before the prior sock_put(sk) ?

> +		cancel_delayed_work_sync(&conn->timeout_work);
>  	}
>  
>  	hcon->sco_data = NULL;
> @@ -193,6 +216,8 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
>  	sco_pi(sk)->conn = conn;
>  	conn->sk = sk;
>  
> +	INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
> +
>  	if (parent)
>  		bt_accept_enqueue(parent, sk, true);
>  }
> @@ -500,8 +525,6 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,
>  
>  	sco_pi(sk)->setting = BT_VOICE_CVSD_16BIT;
>  
> -	timer_setup(&sk->sk_timer, sco_sock_timeout, 0);
> -
>  	bt_sock_link(&sco_sk_list, sk);
>  	return sk;
>  }
> 

Re: [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work

[Desmond Cheong Zhi Xi]

On 2/9/21 3:17 pm, Eric Dumazet wrote:
> 
> 
> On 8/9/21 9:14 PM, Desmond Cheong Zhi Xi wrote:
>> struct sock.sk_timer should be used as a sock cleanup timer. However,
>> SCO uses it to implement sock timeouts.
>>
>> This causes issues because struct sock.sk_timer's callback is run in
>> an IRQ context, and the timer callback function sco_sock_timeout takes
>> a spin lock on the socket. However, other functions such as
>> sco_conn_del and sco_conn_ready take the spin lock with interrupts
>> enabled.
>>
>> This inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} lock usage could
>> lead to deadlocks as reported by Syzbot [1]:
>>         CPU0
>>         ----
>>    lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
>>    <Interrupt>
>>      lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
>>
>> To fix this, we use delayed work to implement SCO sock timouts
>> instead. This allows us to avoid taking the spin lock on the socket in
>> an IRQ context, and corrects the misuse of struct sock.sk_timer.
>>
>> As a note, cancel_delayed_work is used instead of
>> cancel_delayed_work_sync in sco_sock_set_timer and
>> sco_sock_clear_timer to avoid a deadlock. In the future, the call to
>> bh_lock_sock inside sco_sock_timeout should be changed to lock_sock to
>> synchronize with other functions using lock_sock. However, since
>> sco_sock_set_timer and sco_sock_clear_timer are sometimes called under
>> the locked socket (in sco_connect and __sco_sock_close),
>> cancel_delayed_work_sync might cause them to sleep until an
>> sco_sock_timeout that has started finishes running. But
>> sco_sock_timeout would also sleep until it can grab the lock_sock.
>>
>> Using cancel_delayed_work is fine because sco_sock_timeout does not
>> change from run to run, hence there is no functional difference
>> between:
>> 1. waiting for a timeout to finish running before scheduling another
>> timeout
>> 2. scheduling another timeout while a timeout is running.
>>
>> Link: https://syzkaller.appspot.com/bug?id=9089d89de0502e120f234ca0fc8a703f7368b31e [1]
>> Reported-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
>> Tested-by: syzbot+2f6d7c28bb4bf7e82060@syzkaller.appspotmail.com
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
>> ---
>>   net/bluetooth/sco.c | 35 +++++++++++++++++++++++++++++------
>>   1 file changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>> index ffa2a77a3e4c..62e638f971a9 100644
>> --- a/net/bluetooth/sco.c
>> +++ b/net/bluetooth/sco.c
>> @@ -48,6 +48,8 @@ struct sco_conn {
>>   	spinlock_t	lock;
>>   	struct sock	*sk;
>>   
>> +	struct delayed_work	timeout_work;
>> +
>>   	unsigned int    mtu;
>>   };
>>   
>> @@ -74,9 +76,20 @@ struct sco_pinfo {
>>   #define SCO_CONN_TIMEOUT	(HZ * 40)
>>   #define SCO_DISCONN_TIMEOUT	(HZ * 2)
>>   
>> -static void sco_sock_timeout(struct timer_list *t)
>> +static void sco_sock_timeout(struct work_struct *work)
>>   {
>> -	struct sock *sk = from_timer(sk, t, sk_timer);
>> +	struct sco_conn *conn = container_of(work, struct sco_conn,
>> +					     timeout_work.work);
>> +	struct sock *sk;
>> +
>> +	sco_conn_lock(conn);
>> +	sk = conn->sk;
>> +	if (sk)
>> +		sock_hold(sk);
> 
> syzbot complains here that sk refcount can be zero at this time.
> 
> refcount_t: addition on 0; use-after-free.
> WARNING: CPU: 0 PID: 10451 at lib/refcount.c:25 refcount_warn_saturate+0x169/0x1e0 lib/refcount.c:25
> Modules linked in:
> CPU: 0 PID: 10451 Comm: kworker/0:8 Not tainted 5.14.0-rc7-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: events sco_sock_timeout
> RIP: 0010:refcount_warn_saturate+0x169/0x1e0 lib/refcount.c:25
> Code: 09 31 ff 89 de e8 d7 c9 9e fd 84 db 0f 85 36 ff ff ff e8 8a c3 9e fd 48 c7 c7 20 8f e3 89 c6 05 e8 7f 81 09 01 e8 f0 98 16 05 <0f> 0b e9 17 ff ff ff e8 6b c3 9e fd 0f b6 1d cd 7f 81 09 31 ff 89
> RSP: 0018:ffffc9001766fce8 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: ffff88802cea3880 RSI: ffffffff815d87a5 RDI: fffff52002ecdf8f
> RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffffff815d25de R11: 0000000000000000 R12: ffff88806d23ce08
> R13: ffff8880712c8080 R14: ffff88802edf4500 R15: ffff8880b9c51240
> FS:  0000000000000000(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f3748c20000 CR3: 0000000017644000 CR4: 00000000001506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>   __refcount_add include/linux/refcount.h:199 [inline]
>   __refcount_inc include/linux/refcount.h:250 [inline]
>   refcount_inc include/linux/refcount.h:267 [inline]
>   sock_hold include/net/sock.h:702 [inline]
>   sco_sock_timeout+0x216/0x290 net/bluetooth/sco.c:88
>   process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
>   worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
>   kthread+0x3e5/0x4d0 kernel/kthread.c:319
> 
> 
>> +	sco_conn_unlock(conn);
>> +
>> +	if (!sk)
>> +		return;
>>   
>>   	BT_DBG("sock %p state %d", sk, sk->sk_state);
>>   
>> @@ -91,14 +104,21 @@ static void sco_sock_timeout(struct timer_list *t)
>>   
>>   static void sco_sock_set_timer(struct sock *sk, long timeout)
>>   {
>> +	if (!sco_pi(sk)->conn)
>> +		return;
>> +
>>   	BT_DBG("sock %p state %d timeout %ld", sk, sk->sk_state, timeout);
>> -	sk_reset_timer(sk, &sk->sk_timer, jiffies + timeout);
>> +	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
>> +	schedule_delayed_work(&sco_pi(sk)->conn->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);
>> -	sk_stop_timer(sk, &sk->sk_timer);
>> +	cancel_delayed_work(&sco_pi(sk)->conn->timeout_work);
> 
> 
>>   }
>>   
>>   /* ---- SCO connections ---- */
>> @@ -179,6 +199,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>>   		bh_unlock_sock(sk);
>>   		sco_sock_kill(sk);
>>   		sock_put(sk);
>> +
>> +		/* Ensure no more work items will run before freeing conn. */
> 
> Maybe you should have done this cancel_delayed_work_sync() before the prior sock_put(sk) ?
> 
>> +		cancel_delayed_work_sync(&conn->timeout_work);
>>   	}
>>   
>>   	hcon->sco_data = NULL;
>> @@ -193,6 +216,8 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
>>   	sco_pi(sk)->conn = conn;
>>   	conn->sk = sk;
>>   
>> +	INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
>> +
>>   	if (parent)
>>   		bt_accept_enqueue(parent, sk, true);
>>   }
>> @@ -500,8 +525,6 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,
>>   
>>   	sco_pi(sk)->setting = BT_VOICE_CVSD_16BIT;
>>   
>> -	timer_setup(&sk->sk_timer, sco_sock_timeout, 0);
>> -
>>   	bt_sock_link(&sco_sk_list, sk);
>>   	return sk;
>>   }
>>

Hi Eric,

This actually seems to be a pre-existing error in sco_sock_connect that 
we now hit in sco_sock_timeout.

Any thoughts on the following patch to address the problem?

Link: 
https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/

Re: [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work

[Eric Dumazet]

On 9/2/21 12:32 PM, Desmond Cheong Zhi Xi wrote:
> 
> Hi Eric,
> 
> This actually seems to be a pre-existing error in sco_sock_connect that we now hit in sco_sock_timeout.
> 
> Any thoughts on the following patch to address the problem?
> 
> Link: https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/


syzbot is still working on finding a repro, this is obviously not trivial,
because this is a race window.

I think this can happen even with a single SCO connection.

This might be triggered more easily forcing a delay in sco_sock_timeout()

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 98a88158651281c9f75c4e0371044251e976e7ef..71ebe0243fab106c676c308724fe3a3f92a62cbd 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -84,8 +84,14 @@ static void sco_sock_timeout(struct work_struct *work)
 
        sco_conn_lock(conn);
        sk = conn->sk;
-       if (sk)
+       if (sk) {
+               // lets pretend cpu has been busy (in interrupts) for 100ms
+               int i;
+               for (i=0;i<100000;i++)
+                       udelay(1);
+
                sock_hold(sk);
+       }
        sco_conn_unlock(conn);
 
        if (!sk)


Stack trace tells us that sco_sock_timeout() is running after last reference
on socket has been released.

__refcount_add include/linux/refcount.h:199 [inline]
 __refcount_inc include/linux/refcount.h:250 [inline]
 refcount_inc include/linux/refcount.h:267 [inline]
 sock_hold include/net/sock.h:702 [inline]
 sco_sock_timeout+0x216/0x290 net/bluetooth/sco.c:88
 process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
 worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
 kthread+0x3e5/0x4d0 kernel/kthread.c:319
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

This is why I suggested to delay sock_put() to make sure this can not happen.

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 98a88158651281c9f75c4e0371044251e976e7ef..bd0222e3f05a6bcb40cffe8405c9dfff98d7afde 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -195,10 +195,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
                sco_sock_clear_timer(sk);
                sco_chan_del(sk, err);
                release_sock(sk);
-               sock_put(sk);
 
                /* Ensure no more work items will run before freeing conn. */
                cancel_delayed_work_sync(&conn->timeout_work);
+
+               sock_put(sk);
        }
 
        hcon->sco_data = NULL;

Re: [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work

[Desmond Cheong Zhi Xi]

On 2/9/21 5:41 pm, Eric Dumazet wrote:
> 
> 
> On 9/2/21 12:32 PM, Desmond Cheong Zhi Xi wrote:
>>
>> Hi Eric,
>>
>> This actually seems to be a pre-existing error in sco_sock_connect that we now hit in sco_sock_timeout.
>>
>> Any thoughts on the following patch to address the problem?
>>
>> Link: https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/
> 
> 
> syzbot is still working on finding a repro, this is obviously not trivial,
> because this is a race window.
> 
> I think this can happen even with a single SCO connection.
> 
> This might be triggered more easily forcing a delay in sco_sock_timeout()
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 98a88158651281c9f75c4e0371044251e976e7ef..71ebe0243fab106c676c308724fe3a3f92a62cbd 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -84,8 +84,14 @@ static void sco_sock_timeout(struct work_struct *work)
>   
>          sco_conn_lock(conn);
>          sk = conn->sk;
> -       if (sk)
> +       if (sk) {
> +               // lets pretend cpu has been busy (in interrupts) for 100ms
> +               int i;
> +               for (i=0;i<100000;i++)
> +                       udelay(1);
> +
>                  sock_hold(sk);
> +       }>          sco_conn_unlock(conn);
>   
>          if (!sk)
> 
> 
> Stack trace tells us that sco_sock_timeout() is running after last reference
> on socket has been released.
> 
> __refcount_add include/linux/refcount.h:199 [inline]
>   __refcount_inc include/linux/refcount.h:250 [inline]
>   refcount_inc include/linux/refcount.h:267 [inline]
>   sock_hold include/net/sock.h:702 [inline]
>   sco_sock_timeout+0x216/0x290 net/bluetooth/sco.c:88
>   process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
>   worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
>   kthread+0x3e5/0x4d0 kernel/kthread.c:319
>   ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
> 
> This is why I suggested to delay sock_put() to make sure this can not happen.
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 98a88158651281c9f75c4e0371044251e976e7ef..bd0222e3f05a6bcb40cffe8405c9dfff98d7afde 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -195,10 +195,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>                  sco_sock_clear_timer(sk);
>                  sco_chan_del(sk, err);
>                  release_sock(sk);
> -               sock_put(sk);
>   
>                  /* Ensure no more work items will run before freeing conn. */
>                  cancel_delayed_work_sync(&conn->timeout_work);
> +
> +               sock_put(sk);
>          }
>   
>          hcon->sco_data = NULL;
> 

I see where you're going with this, but once sco_chan_del returns, any
instance of sco_sock_timeout that hasn't yet called sock_hold will
simply return, because conn->sk is NULL. Adding a delay to the
sco_conn_lock critical section in sco_sock_timeout would not affect this
because sco_chan_del clears conn->sk while holding onto the lock.

The main reason that cancel_delayed_work_sync is run there is to make
sure that we don't have a UAF on the SCO connection itself after we free
conn.

For a single SCO connection with well-formed channel, I think there
can't be a race. Here's the reasoning:

- For the timeout to be scheduled, a socket must have a channel with a
connection.

- When a channel between a socket and connection is established, the
socket transitions from BT_OPEN to BT_CONNECTED, BT_CONNECT, or
BT_CONNECT2.

- For a socket to be released, it has to be zapped. For sockets that
have a state of BT_CONNECTED, BT_CONNECT, or BT_CONNECT2, they are
zapped only when the channel is deleted.

- If the channel is deleted (which is protected by sco_conn_lock), then
conn->sk is NULL, and sco_sock_timeout simply exits. If we had entered
the critical section in sco_sock_timeout before the channel was deleted,
then we increased the reference count on the socket, so it won't be
freed until sco_sock_timeout is done.

Hence, sco_sock_timeout doesn't race with the release of a socket that
has a well-formed channel with a connection.

But if multiple connections are allocated and overwritten in
sco_sock_connect, then none of the above assumptions hold because the
SCO connection can't be cleaned up (i.e. conn->sk cannot be set to NULL)
when the associated socket is released. This scenario happens in the
syzbot reproducer for the crash here:
https://syzkaller.appspot.com/bug?id=bcc246d137428d00ed14b476c2068579515fe2bc

That aside, upon taking a closer look, I think there is indeed a race
lurking in sco_conn_del, but it's not the one that syzbot is hitting.
Our sock_hold simply comes too late, and by the time it's called we
might have already have freed the socket.

So probably something like this needs to happen:

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index fa25b07120c9..ea18e5b56343 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -187,10 +187,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
  	/* Kill socket */
  	sco_conn_lock(conn);
  	sk = conn->sk;
+	if (sk)
+		sock_hold(sk);
  	sco_conn_unlock(conn);
  
  	if (sk) {
-		sock_hold(sk);
  		lock_sock(sk);
  		sco_sock_clear_timer(sk);
  		sco_chan_del(sk, err);

Re: [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work

[Desmond Cheong Zhi Xi]

On 2/9/21 6:53 pm, Desmond Cheong Zhi Xi wrote:
> On 2/9/21 5:41 pm, Eric Dumazet wrote:
>>
>>
>> On 9/2/21 12:32 PM, Desmond Cheong Zhi Xi wrote:
>>>
>>> Hi Eric,
>>>
>>> This actually seems to be a pre-existing error in sco_sock_connect 
>>> that we now hit in sco_sock_timeout.
>>>
>>> Any thoughts on the following patch to address the problem?
>>>
>>> Link: 
>>> https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/ 
>>>
>>
>>
>> syzbot is still working on finding a repro, this is obviously not 
>> trivial,
>> because this is a race window.
>>
>> I think this can happen even with a single SCO connection.
>>
>> This might be triggered more easily forcing a delay in sco_sock_timeout()
>>
>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>> index 
>> 98a88158651281c9f75c4e0371044251e976e7ef..71ebe0243fab106c676c308724fe3a3f92a62cbd 
>> 100644
>> --- a/net/bluetooth/sco.c
>> +++ b/net/bluetooth/sco.c
>> @@ -84,8 +84,14 @@ static void sco_sock_timeout(struct work_struct *work)
>>          sco_conn_lock(conn);
>>          sk = conn->sk;
>> -       if (sk)
>> +       if (sk) {
>> +               // lets pretend cpu has been busy (in interrupts) for 
>> 100ms
>> +               int i;
>> +               for (i=0;i<100000;i++)
>> +                       udelay(1);
>> +
>>                  sock_hold(sk);
>> +       }>          sco_conn_unlock(conn);
>>          if (!sk)
>>
>>
>> Stack trace tells us that sco_sock_timeout() is running after last 
>> reference
>> on socket has been released.
>>
>> __refcount_add include/linux/refcount.h:199 [inline]
>>   __refcount_inc include/linux/refcount.h:250 [inline]
>>   refcount_inc include/linux/refcount.h:267 [inline]
>>   sock_hold include/net/sock.h:702 [inline]
>>   sco_sock_timeout+0x216/0x290 net/bluetooth/sco.c:88
>>   process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
>>   worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
>>   kthread+0x3e5/0x4d0 kernel/kthread.c:319
>>   ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
>>
>> This is why I suggested to delay sock_put() to make sure this can not 
>> happen.
>>
>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>> index 
>> 98a88158651281c9f75c4e0371044251e976e7ef..bd0222e3f05a6bcb40cffe8405c9dfff98d7afde 
>> 100644
>> --- a/net/bluetooth/sco.c
>> +++ b/net/bluetooth/sco.c
>> @@ -195,10 +195,11 @@ static void sco_conn_del(struct hci_conn *hcon, 
>> int err)
>>                  sco_sock_clear_timer(sk);
>>                  sco_chan_del(sk, err);
>>                  release_sock(sk);
>> -               sock_put(sk);
>>                  /* Ensure no more work items will run before freeing 
>> conn. */
>>                  cancel_delayed_work_sync(&conn->timeout_work);
>> +
>> +               sock_put(sk);
>>          }
>>          hcon->sco_data = NULL;
>>
> 
> I see where you're going with this, but once sco_chan_del returns, any
> instance of sco_sock_timeout that hasn't yet called sock_hold will
> simply return, because conn->sk is NULL. Adding a delay to the
> sco_conn_lock critical section in sco_sock_timeout would not affect this
> because sco_chan_del clears conn->sk while holding onto the lock.
> 
> The main reason that cancel_delayed_work_sync is run there is to make
> sure that we don't have a UAF on the SCO connection itself after we free
> conn.
> 

Now that I think about this, the init and cleanup isn't quite right
either. The delayed work should be initialized when the connection is
allocated, and we should always cancel all work before freeing:

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index ea18e5b56343..bba5cdb4cb4a 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -133,6 +133,7 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
  		return NULL;
  
  	spin_lock_init(&conn->lock);
+	INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
  
  	hcon->sco_data = conn;
  	conn->hcon = hcon;
@@ -197,11 +198,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
  		sco_chan_del(sk, err);
  		release_sock(sk);
  		sock_put(sk);
-
-		/* Ensure no more work items will run before freeing conn. */
-		cancel_delayed_work_sync(&conn->timeout_work);
  	}
  
+	/* Ensure no more work items will run before freeing conn. */
+	cancel_delayed_work_sync(&conn->timeout_work);
+
  	hcon->sco_data = NULL;
  	kfree(conn);
  }
@@ -214,8 +215,6 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
  	sco_pi(sk)->conn = conn;
  	conn->sk = sk;
  
-	INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
-
  	if (parent)
  		bt_accept_enqueue(parent, sk, true);
  }

> For a single SCO connection with well-formed channel, I think there
> can't be a race. Here's the reasoning:
> 
> - For the timeout to be scheduled, a socket must have a channel with a
> connection.
> 
> - When a channel between a socket and connection is established, the
> socket transitions from BT_OPEN to BT_CONNECTED, BT_CONNECT, or
> BT_CONNECT2.
> 
> - For a socket to be released, it has to be zapped. For sockets that
> have a state of BT_CONNECTED, BT_CONNECT, or BT_CONNECT2, they are
> zapped only when the channel is deleted.
> 
> - If the channel is deleted (which is protected by sco_conn_lock), then
> conn->sk is NULL, and sco_sock_timeout simply exits. If we had entered
> the critical section in sco_sock_timeout before the channel was deleted,
> then we increased the reference count on the socket, so it won't be
> freed until sco_sock_timeout is done.
> 
> Hence, sco_sock_timeout doesn't race with the release of a socket that
> has a well-formed channel with a connection.
> 
> But if multiple connections are allocated and overwritten in
> sco_sock_connect, then none of the above assumptions hold because the
> SCO connection can't be cleaned up (i.e. conn->sk cannot be set to NULL)
> when the associated socket is released. This scenario happens in the
> syzbot reproducer for the crash here:
> https://syzkaller.appspot.com/bug?id=bcc246d137428d00ed14b476c2068579515fe2bc 
> 
> 
> That aside, upon taking a closer look, I think there is indeed a race
> lurking in sco_conn_del, but it's not the one that syzbot is hitting.
> Our sock_hold simply comes too late, and by the time it's called we
> might have already have freed the socket.
> 
> So probably something like this needs to happen:
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index fa25b07120c9..ea18e5b56343 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -187,10 +187,11 @@ static void sco_conn_del(struct hci_conn *hcon, 
> int err)
>       /* Kill socket */
>       sco_conn_lock(conn);
>       sk = conn->sk;
> +    if (sk)
> +        sock_hold(sk);
>       sco_conn_unlock(conn);
> 
>       if (sk) {
> -        sock_hold(sk);
>           lock_sock(sk);
>           sco_sock_clear_timer(sk);
>           sco_chan_del(sk, err);

Re: [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work

[Luiz Augusto von Dentz]

Hi Desmond,

On Thu, Sep 2, 2021 at 4:05 PM Desmond Cheong Zhi Xi
<desmondcheongzx@gmail.com> wrote:
>
> On 2/9/21 6:53 pm, Desmond Cheong Zhi Xi wrote:
> > On 2/9/21 5:41 pm, Eric Dumazet wrote:
> >>
> >>
> >> On 9/2/21 12:32 PM, Desmond Cheong Zhi Xi wrote:
> >>>
> >>> Hi Eric,
> >>>
> >>> This actually seems to be a pre-existing error in sco_sock_connect
> >>> that we now hit in sco_sock_timeout.
> >>>
> >>> Any thoughts on the following patch to address the problem?
> >>>
> >>> Link:
> >>> https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/
> >>>
> >>
> >>
> >> syzbot is still working on finding a repro, this is obviously not
> >> trivial,
> >> because this is a race window.
> >>
> >> I think this can happen even with a single SCO connection.
> >>
> >> This might be triggered more easily forcing a delay in sco_sock_timeout()
> >>
> >> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> >> index
> >> 98a88158651281c9f75c4e0371044251e976e7ef..71ebe0243fab106c676c308724fe3a3f92a62cbd
> >> 100644
> >> --- a/net/bluetooth/sco.c
> >> +++ b/net/bluetooth/sco.c
> >> @@ -84,8 +84,14 @@ static void sco_sock_timeout(struct work_struct *work)
> >>          sco_conn_lock(conn);
> >>          sk = conn->sk;
> >> -       if (sk)
> >> +       if (sk) {
> >> +               // lets pretend cpu has been busy (in interrupts) for
> >> 100ms
> >> +               int i;
> >> +               for (i=0;i<100000;i++)
> >> +                       udelay(1);
> >> +
> >>                  sock_hold(sk);
> >> +       }>          sco_conn_unlock(conn);
> >>          if (!sk)
> >>
> >>
> >> Stack trace tells us that sco_sock_timeout() is running after last
> >> reference
> >> on socket has been released.
> >>
> >> __refcount_add include/linux/refcount.h:199 [inline]
> >>   __refcount_inc include/linux/refcount.h:250 [inline]
> >>   refcount_inc include/linux/refcount.h:267 [inline]
> >>   sock_hold include/net/sock.h:702 [inline]
> >>   sco_sock_timeout+0x216/0x290 net/bluetooth/sco.c:88
> >>   process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
> >>   worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
> >>   kthread+0x3e5/0x4d0 kernel/kthread.c:319
> >>   ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
> >>
> >> This is why I suggested to delay sock_put() to make sure this can not
> >> happen.
> >>
> >> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> >> index
> >> 98a88158651281c9f75c4e0371044251e976e7ef..bd0222e3f05a6bcb40cffe8405c9dfff98d7afde
> >> 100644
> >> --- a/net/bluetooth/sco.c
> >> +++ b/net/bluetooth/sco.c
> >> @@ -195,10 +195,11 @@ static void sco_conn_del(struct hci_conn *hcon,
> >> int err)
> >>                  sco_sock_clear_timer(sk);
> >>                  sco_chan_del(sk, err);
> >>                  release_sock(sk);
> >> -               sock_put(sk);
> >>                  /* Ensure no more work items will run before freeing
> >> conn. */
> >>                  cancel_delayed_work_sync(&conn->timeout_work);
> >> +
> >> +               sock_put(sk);
> >>          }
> >>          hcon->sco_data = NULL;
> >>
> >
> > I see where you're going with this, but once sco_chan_del returns, any
> > instance of sco_sock_timeout that hasn't yet called sock_hold will
> > simply return, because conn->sk is NULL. Adding a delay to the
> > sco_conn_lock critical section in sco_sock_timeout would not affect this
> > because sco_chan_del clears conn->sk while holding onto the lock.
> >
> > The main reason that cancel_delayed_work_sync is run there is to make
> > sure that we don't have a UAF on the SCO connection itself after we free
> > conn.
> >
>
> Now that I think about this, the init and cleanup isn't quite right
> either. The delayed work should be initialized when the connection is
> allocated, and we should always cancel all work before freeing:
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index ea18e5b56343..bba5cdb4cb4a 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -133,6 +133,7 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
>                 return NULL;
>
>         spin_lock_init(&conn->lock);
> +       INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
>
>         hcon->sco_data = conn;
>         conn->hcon = hcon;
> @@ -197,11 +198,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>                 sco_chan_del(sk, err);
>                 release_sock(sk);
>                 sock_put(sk);
> -
> -               /* Ensure no more work items will run before freeing conn. */
> -               cancel_delayed_work_sync(&conn->timeout_work);
>         }
>
> +       /* Ensure no more work items will run before freeing conn. */
> +       cancel_delayed_work_sync(&conn->timeout_work);
> +
>         hcon->sco_data = NULL;
>         kfree(conn);
>   }
> @@ -214,8 +215,6 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
>         sco_pi(sk)->conn = conn;
>         conn->sk = sk;
>
> -       INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
> -
>         if (parent)
>                 bt_accept_enqueue(parent, sk, true);
>   }

I have come to something similar, do you care to send a proper patch
so we can get this merged.

> > For a single SCO connection with well-formed channel, I think there
> > can't be a race. Here's the reasoning:
> >
> > - For the timeout to be scheduled, a socket must have a channel with a
> > connection.
> >
> > - When a channel between a socket and connection is established, the
> > socket transitions from BT_OPEN to BT_CONNECTED, BT_CONNECT, or
> > BT_CONNECT2.
> >
> > - For a socket to be released, it has to be zapped. For sockets that
> > have a state of BT_CONNECTED, BT_CONNECT, or BT_CONNECT2, they are
> > zapped only when the channel is deleted.
> >
> > - If the channel is deleted (which is protected by sco_conn_lock), then
> > conn->sk is NULL, and sco_sock_timeout simply exits. If we had entered
> > the critical section in sco_sock_timeout before the channel was deleted,
> > then we increased the reference count on the socket, so it won't be
> > freed until sco_sock_timeout is done.
> >
> > Hence, sco_sock_timeout doesn't race with the release of a socket that
> > has a well-formed channel with a connection.
> >
> > But if multiple connections are allocated and overwritten in
> > sco_sock_connect, then none of the above assumptions hold because the
> > SCO connection can't be cleaned up (i.e. conn->sk cannot be set to NULL)
> > when the associated socket is released. This scenario happens in the
> > syzbot reproducer for the crash here:
> > https://syzkaller.appspot.com/bug?id=bcc246d137428d00ed14b476c2068579515fe2bc
> >
> >
> > That aside, upon taking a closer look, I think there is indeed a race
> > lurking in sco_conn_del, but it's not the one that syzbot is hitting.
> > Our sock_hold simply comes too late, and by the time it's called we
> > might have already have freed the socket.
> >
> > So probably something like this needs to happen:
> >
> > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> > index fa25b07120c9..ea18e5b56343 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -187,10 +187,11 @@ static void sco_conn_del(struct hci_conn *hcon,
> > int err)
> >       /* Kill socket */
> >       sco_conn_lock(conn);
> >       sk = conn->sk;
> > +    if (sk)
> > +        sock_hold(sk);
> >       sco_conn_unlock(conn);
> >
> >       if (sk) {
> > -        sock_hold(sk);
> >           lock_sock(sk);
> >           sco_sock_clear_timer(sk);
> >           sco_chan_del(sk, err);
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v6 1/6] Bluetooth: schedule SCO timeouts with delayed_work

[Desmond Cheong Zhi Xi]

Hi Luiz,

On 2/9/21 7:42 pm, Luiz Augusto von Dentz wrote:
> Hi Desmond,
> 
> On Thu, Sep 2, 2021 at 4:05 PM Desmond Cheong Zhi Xi
> <desmondcheongzx@gmail.com> wrote:
>>
>> On 2/9/21 6:53 pm, Desmond Cheong Zhi Xi wrote:
>>> On 2/9/21 5:41 pm, Eric Dumazet wrote:
>>>>
>>>>
>>>> On 9/2/21 12:32 PM, Desmond Cheong Zhi Xi wrote:
>>>>>
>>>>> Hi Eric,
>>>>>
>>>>> This actually seems to be a pre-existing error in sco_sock_connect
>>>>> that we now hit in sco_sock_timeout.
>>>>>
>>>>> Any thoughts on the following patch to address the problem?
>>>>>
>>>>> Link:
>>>>> https://lore.kernel.org/lkml/20210831065601.101185-1-desmondcheongzx@gmail.com/
>>>>>
>>>>
>>>>
>>>> syzbot is still working on finding a repro, this is obviously not
>>>> trivial,
>>>> because this is a race window.
>>>>
>>>> I think this can happen even with a single SCO connection.
>>>>
>>>> This might be triggered more easily forcing a delay in sco_sock_timeout()
>>>>
>>>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>>>> index
>>>> 98a88158651281c9f75c4e0371044251e976e7ef..71ebe0243fab106c676c308724fe3a3f92a62cbd
>>>> 100644
>>>> --- a/net/bluetooth/sco.c
>>>> +++ b/net/bluetooth/sco.c
>>>> @@ -84,8 +84,14 @@ static void sco_sock_timeout(struct work_struct *work)
>>>>           sco_conn_lock(conn);
>>>>           sk = conn->sk;
>>>> -       if (sk)
>>>> +       if (sk) {
>>>> +               // lets pretend cpu has been busy (in interrupts) for
>>>> 100ms
>>>> +               int i;
>>>> +               for (i=0;i<100000;i++)
>>>> +                       udelay(1);
>>>> +
>>>>                   sock_hold(sk);
>>>> +       }>          sco_conn_unlock(conn);
>>>>           if (!sk)
>>>>
>>>>
>>>> Stack trace tells us that sco_sock_timeout() is running after last
>>>> reference
>>>> on socket has been released.
>>>>
>>>> __refcount_add include/linux/refcount.h:199 [inline]
>>>>    __refcount_inc include/linux/refcount.h:250 [inline]
>>>>    refcount_inc include/linux/refcount.h:267 [inline]
>>>>    sock_hold include/net/sock.h:702 [inline]
>>>>    sco_sock_timeout+0x216/0x290 net/bluetooth/sco.c:88
>>>>    process_one_work+0x98d/0x1630 kernel/workqueue.c:2276
>>>>    worker_thread+0x658/0x11f0 kernel/workqueue.c:2422
>>>>    kthread+0x3e5/0x4d0 kernel/kthread.c:319
>>>>    ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
>>>>
>>>> This is why I suggested to delay sock_put() to make sure this can not
>>>> happen.
>>>>
>>>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>>>> index
>>>> 98a88158651281c9f75c4e0371044251e976e7ef..bd0222e3f05a6bcb40cffe8405c9dfff98d7afde
>>>> 100644
>>>> --- a/net/bluetooth/sco.c
>>>> +++ b/net/bluetooth/sco.c
>>>> @@ -195,10 +195,11 @@ static void sco_conn_del(struct hci_conn *hcon,
>>>> int err)
>>>>                   sco_sock_clear_timer(sk);
>>>>                   sco_chan_del(sk, err);
>>>>                   release_sock(sk);
>>>> -               sock_put(sk);
>>>>                   /* Ensure no more work items will run before freeing
>>>> conn. */
>>>>                   cancel_delayed_work_sync(&conn->timeout_work);
>>>> +
>>>> +               sock_put(sk);
>>>>           }
>>>>           hcon->sco_data = NULL;
>>>>
>>>
>>> I see where you're going with this, but once sco_chan_del returns, any
>>> instance of sco_sock_timeout that hasn't yet called sock_hold will
>>> simply return, because conn->sk is NULL. Adding a delay to the
>>> sco_conn_lock critical section in sco_sock_timeout would not affect this
>>> because sco_chan_del clears conn->sk while holding onto the lock.
>>>
>>> The main reason that cancel_delayed_work_sync is run there is to make
>>> sure that we don't have a UAF on the SCO connection itself after we free
>>> conn.
>>>
>>
>> Now that I think about this, the init and cleanup isn't quite right
>> either. The delayed work should be initialized when the connection is
>> allocated, and we should always cancel all work before freeing:
>>
>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>> index ea18e5b56343..bba5cdb4cb4a 100644
>> --- a/net/bluetooth/sco.c
>> +++ b/net/bluetooth/sco.c
>> @@ -133,6 +133,7 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
>>                  return NULL;
>>
>>          spin_lock_init(&conn->lock);
>> +       INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
>>
>>          hcon->sco_data = conn;
>>          conn->hcon = hcon;
>> @@ -197,11 +198,11 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>>                  sco_chan_del(sk, err);
>>                  release_sock(sk);
>>                  sock_put(sk);
>> -
>> -               /* Ensure no more work items will run before freeing conn. */
>> -               cancel_delayed_work_sync(&conn->timeout_work);
>>          }
>>
>> +       /* Ensure no more work items will run before freeing conn. */
>> +       cancel_delayed_work_sync(&conn->timeout_work);
>> +
>>          hcon->sco_data = NULL;
>>          kfree(conn);
>>    }
>> @@ -214,8 +215,6 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
>>          sco_pi(sk)->conn = conn;
>>          conn->sk = sk;
>>
>> -       INIT_DELAYED_WORK(&conn->timeout_work, sco_sock_timeout);
>> -
>>          if (parent)
>>                  bt_accept_enqueue(parent, sk, true);
>>    }
> 
> I have come to something similar, do you care to send a proper patch
> so we can get this merged.
> 

Sounds good. Just finished running some tests locally, I'll send out the 
patches now.

>>> For a single SCO connection with well-formed channel, I think there
>>> can't be a race. Here's the reasoning:
>>>
>>> - For the timeout to be scheduled, a socket must have a channel with a
>>> connection.
>>>
>>> - When a channel between a socket and connection is established, the
>>> socket transitions from BT_OPEN to BT_CONNECTED, BT_CONNECT, or
>>> BT_CONNECT2.
>>>
>>> - For a socket to be released, it has to be zapped. For sockets that
>>> have a state of BT_CONNECTED, BT_CONNECT, or BT_CONNECT2, they are
>>> zapped only when the channel is deleted.
>>>
>>> - If the channel is deleted (which is protected by sco_conn_lock), then
>>> conn->sk is NULL, and sco_sock_timeout simply exits. If we had entered
>>> the critical section in sco_sock_timeout before the channel was deleted,
>>> then we increased the reference count on the socket, so it won't be
>>> freed until sco_sock_timeout is done.
>>>
>>> Hence, sco_sock_timeout doesn't race with the release of a socket that
>>> has a well-formed channel with a connection.
>>>
>>> But if multiple connections are allocated and overwritten in
>>> sco_sock_connect, then none of the above assumptions hold because the
>>> SCO connection can't be cleaned up (i.e. conn->sk cannot be set to NULL)
>>> when the associated socket is released. This scenario happens in the
>>> syzbot reproducer for the crash here:
>>> https://syzkaller.appspot.com/bug?id=bcc246d137428d00ed14b476c2068579515fe2bc
>>>
>>>
>>> That aside, upon taking a closer look, I think there is indeed a race
>>> lurking in sco_conn_del, but it's not the one that syzbot is hitting.
>>> Our sock_hold simply comes too late, and by the time it's called we
>>> might have already have freed the socket.
>>>
>>> So probably something like this needs to happen:
>>>
>>> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>>> index fa25b07120c9..ea18e5b56343 100644
>>> --- a/net/bluetooth/sco.c
>>> +++ b/net/bluetooth/sco.c
>>> @@ -187,10 +187,11 @@ static void sco_conn_del(struct hci_conn *hcon,
>>> int err)
>>>        /* Kill socket */
>>>        sco_conn_lock(conn);
>>>        sk = conn->sk;
>>> +    if (sk)
>>> +        sock_hold(sk);
>>>        sco_conn_unlock(conn);
>>>
>>>        if (sk) {
>>> -        sock_hold(sk);
>>>            lock_sock(sk);
>>>            sco_sock_clear_timer(sk);
>>>            sco_chan_del(sk, err);
>>
> 
>