[PATCH v4] Bluetooth: serialize accept_q access

[PATCH v4] Bluetooth: serialize accept_q access

[Ren Wei]

From: Jiexun Wang <wangjiexun2025@gmail.com>

bt_sock_poll() walks the accept queue without synchronization, while
child teardown can unlink the same socket and drop its last reference.
The unsynchronized accept queue walk has existed since the initial
Bluetooth import.

Protect accept_q with a dedicated lock for queue updates and polling.
Also rework bt_accept_dequeue() to take temporary child references under
the queue lock before dropping it and locking the child socket.

Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reported-by: Jann Horn <jannh@google.com>
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Jiexun Wang <wangjiexun2025@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
Changes in v4:
- no functional changes
- clarify that the race dates back to the initial Bluetooth import
- update trailers
  I noticed Jann also proposed a fix at
  https://patchwork.kernel.org/project/bluetooth/patch/20260504-bluetooth-accept-uaf-fix-v1-1-1ca63c0efadd@google.com/,
  so we're adding his Reported-by tag here. Please let us know if this
  isn't appropriate.
- v3 Link: https://lore.kernel.org/all/20260404162324.2789862-1-n05ec@lzu.edu.cn/

Changes in v3:
- move sk_acceptq_added()/sk_acceptq_removed() inside accept_q_lock
  critical sections to serialize sk_ack_backlog updates with accept_q
  operations
- v2 Link: https://lore.kernel.org/all/06a6b4549acba207847ce532dedbf1c95ab22d13.1774925231.git.wangjiexun2025@gmail.com/

Changes in v2:
- add Tested-by: Ren Wei <enjou1224z@gmail.com>
- resend to the public Bluetooth/netdev lists

 include/net/bluetooth/bluetooth.h |  1 +
 net/bluetooth/af_bluetooth.c      | 87 +++++++++++++++++++++++--------
 2 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 69eed69f7f26..3faea66b1979 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -398,6 +398,7 @@ void baswap(bdaddr_t *dst, const bdaddr_t *src);
 struct bt_sock {
 	struct sock sk;
 	struct list_head accept_q;
+	spinlock_t accept_q_lock; /* protects accept_q */
 	struct sock *parent;
 	unsigned long flags;
 	void (*skb_msg_name)(struct sk_buff *, void *, int *);
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 2b94e2077203..fa14b9a915eb 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -154,6 +154,7 @@ struct sock *bt_sock_alloc(struct net *net, struct socket *sock,
 
 	sock_init_data(sock, sk);
 	INIT_LIST_HEAD(&bt_sk(sk)->accept_q);
+	spin_lock_init(&bt_sk(sk)->accept_q_lock);
 
 	sock_reset_flag(sk, SOCK_ZAPPED);
 
@@ -214,6 +215,7 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
 {
 	const struct cred *old_cred;
 	struct pid *old_pid;
+	struct bt_sock *par = bt_sk(parent);
 
 	BT_DBG("parent %p, sk %p", parent, sk);
 
@@ -224,9 +226,13 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
 	else
 		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
 
-	list_add_tail(&bt_sk(sk)->accept_q, &bt_sk(parent)->accept_q);
 	bt_sk(sk)->parent = parent;
 
+	spin_lock_bh(&par->accept_q_lock);
+	list_add_tail(&bt_sk(sk)->accept_q, &par->accept_q);
+	sk_acceptq_added(parent);
+	spin_unlock_bh(&par->accept_q_lock);
+
 	/* Copy credentials from parent since for incoming connections the
 	 * socket is allocated by the kernel.
 	 */
@@ -244,8 +250,6 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
 		bh_unlock_sock(sk);
 	else
 		release_sock(sk);
-
-	sk_acceptq_added(parent);
 }
 EXPORT_SYMBOL(bt_accept_enqueue);
 
@@ -254,45 +258,72 @@ EXPORT_SYMBOL(bt_accept_enqueue);
  */
 void bt_accept_unlink(struct sock *sk)
 {
+	struct sock *parent = bt_sk(sk)->parent;
+
 	BT_DBG("sk %p state %d", sk, sk->sk_state);
 
+	spin_lock_bh(&bt_sk(parent)->accept_q_lock);
 	list_del_init(&bt_sk(sk)->accept_q);
-	sk_acceptq_removed(bt_sk(sk)->parent);
+	sk_acceptq_removed(parent);
+	spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
 	bt_sk(sk)->parent = NULL;
 	sock_put(sk);
 }
 EXPORT_SYMBOL(bt_accept_unlink);
 
+static struct sock *bt_accept_get(struct sock *parent, struct sock *sk)
+{
+	struct bt_sock *bt = bt_sk(parent);
+	struct sock *next = NULL;
+
+	/* accept_q is modified from child teardown paths too, so take a
+	 * temporary reference before dropping the queue lock.
+	 */
+	spin_lock_bh(&bt->accept_q_lock);
+
+	if (sk) {
+		if (bt_sk(sk)->parent != parent)
+			goto out;
+
+		if (!list_is_last(&bt_sk(sk)->accept_q, &bt->accept_q)) {
+			next = &list_next_entry(bt_sk(sk), accept_q)->sk;
+			sock_hold(next);
+		}
+	} else if (!list_empty(&bt->accept_q)) {
+		next = &list_first_entry(&bt->accept_q,
+					 struct bt_sock, accept_q)->sk;
+		sock_hold(next);
+	}
+
+out:
+	spin_unlock_bh(&bt->accept_q_lock);
+	return next;
+}
+
 struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 {
-	struct bt_sock *s, *n;
-	struct sock *sk;
+	struct sock *sk, *next;
 
 	BT_DBG("parent %p", parent);
 
 restart:
-	list_for_each_entry_safe(s, n, &bt_sk(parent)->accept_q, accept_q) {
-		sk = (struct sock *)s;
-
+	for (sk = bt_accept_get(parent, NULL); sk; sk = next) {
 		/* Prevent early freeing of sk due to unlink and sock_kill */
-		sock_hold(sk);
 		lock_sock(sk);
 
 		/* Check sk has not already been unlinked via
 		 * bt_accept_unlink() due to serialisation caused by sk locking
 		 */
-		if (!bt_sk(sk)->parent) {
+		if (bt_sk(sk)->parent != parent) {
 			BT_DBG("sk %p, already unlinked", sk);
 			release_sock(sk);
 			sock_put(sk);
 
-			/* Restart the loop as sk is no longer in the list
-			 * and also avoid a potential infinite loop because
-			 * list_for_each_entry_safe() is not thread safe.
-			 */
 			goto restart;
 		}
 
+		next = bt_accept_get(parent, sk);
+
 		/* sk is safely in the parent list so reduce reference count */
 		sock_put(sk);
 
@@ -310,6 +341,8 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 				sock_graft(sk, newsock);
 
 			release_sock(sk);
+			if (next)
+				sock_put(next);
 			return sk;
 		}
 
@@ -518,18 +551,28 @@ EXPORT_SYMBOL(bt_sock_stream_recvmsg);
 
 static inline __poll_t bt_accept_poll(struct sock *parent)
 {
-	struct bt_sock *s, *n;
+	struct bt_sock *bt = bt_sk(parent);
+	struct bt_sock *s;
 	struct sock *sk;
+	__poll_t mask = 0;
+
+	spin_lock_bh(&bt->accept_q_lock);
+	list_for_each_entry(s, &bt->accept_q, accept_q) {
+		int state;
 
-	list_for_each_entry_safe(s, n, &bt_sk(parent)->accept_q, accept_q) {
 		sk = (struct sock *)s;
-		if (sk->sk_state == BT_CONNECTED ||
-		    (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags) &&
-		     sk->sk_state == BT_CONNECT2))
-			return EPOLLIN | EPOLLRDNORM;
+		state = READ_ONCE(sk->sk_state);
+
+		if (state == BT_CONNECTED ||
+		    (test_bit(BT_SK_DEFER_SETUP, &bt->flags) &&
+		     state == BT_CONNECT2)) {
+			mask = EPOLLIN | EPOLLRDNORM;
+			break;
+		}
 	}
+	spin_unlock_bh(&bt->accept_q_lock);
 
-	return 0;
+	return mask;
 }
 
 __poll_t bt_sock_poll(struct file *file, struct socket *sock,
-- 
2.34.1

Re: [PATCH v4] Bluetooth: serialize accept_q access

[Jann Horn]

[-- Attachment #1: Type: text/plain, Size: 10085 bytes --]

On Wed, May 6, 2026 at 1:43 PM Ren Wei <n05ec@lzu.edu.cn> wrote:
> bt_sock_poll() walks the accept queue without synchronization, while
> child teardown can unlink the same socket and drop its last reference.
> The unsynchronized accept queue walk has existed since the initial
> Bluetooth import.
>
> Protect accept_q with a dedicated lock for queue updates and polling.
> Also rework bt_accept_dequeue() to take temporary child references under
> the queue lock before dropping it and locking the child socket.
>
> Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Reported-by: Jann Horn <jannh@google.com>
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Signed-off-by: Jiexun Wang <wangjiexun2025@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>

The patch looks good to me. I have some comments below, but they're
not important - from my perspective, this patch is ready to land in
the tree.

Reviewed-by: Jann Horn <jannh@google.com>

> ---
> Changes in v4:
> - no functional changes
> - clarify that the race dates back to the initial Bluetooth import
> - update trailers
>   I noticed Jann also proposed a fix at
>   https://patchwork.kernel.org/project/bluetooth/patch/20260504-bluetooth-accept-uaf-fix-v1-1-1ca63c0efadd@google.com/,
>   so we're adding his Reported-by tag here. Please let us know if this
>   isn't appropriate.

Thanks for letting me know that my patch was redundant, and for
listing me in Reported-by.
This addresses the race I described.
(You could add the line
"Closes: https://lore.kernel.org/r/20260504-bluetooth-accept-uaf-fix-v1-1-1ca63c0efadd@google.com"
after the "Reported-by: Jann Horn <jannh@google.com>".)

[...]
> @@ -254,45 +258,72 @@ EXPORT_SYMBOL(bt_accept_enqueue);
>   */
>  void bt_accept_unlink(struct sock *sk)
>  {
> +       struct sock *parent = bt_sk(sk)->parent;
> +
>         BT_DBG("sk %p state %d", sk, sk->sk_state);
>
> +       spin_lock_bh(&bt_sk(parent)->accept_q_lock);
>         list_del_init(&bt_sk(sk)->accept_q);
> -       sk_acceptq_removed(bt_sk(sk)->parent);
> +       sk_acceptq_removed(parent);
> +       spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
>         bt_sk(sk)->parent = NULL;
>         sock_put(sk);
>  }
>  EXPORT_SYMBOL(bt_accept_unlink);
>
> +static struct sock *bt_accept_get(struct sock *parent, struct sock *sk)
> +{
> +       struct bt_sock *bt = bt_sk(parent);
> +       struct sock *next = NULL;
> +
> +       /* accept_q is modified from child teardown paths too, so take a
> +        * temporary reference before dropping the queue lock.
> +        */
> +       spin_lock_bh(&bt->accept_q_lock);
> +
> +       if (sk) {
> +               if (bt_sk(sk)->parent != parent)
> +                       goto out;

This check seems redundant? The caller already bailed out if
"bt_sk(sk)->parent != parent", and lock_sock(sk) ensures that
bt_sk(sk)->parent can't change concurrently because bt_accept_unlink()
is also protected by lock_sock() or lock_sock_nested(), as the comment
above bt_accept_unlink() documents.

> +
> +               if (!list_is_last(&bt_sk(sk)->accept_q, &bt->accept_q)) {
> +                       next = &list_next_entry(bt_sk(sk), accept_q)->sk;
> +                       sock_hold(next);
> +               }
> +       } else if (!list_empty(&bt->accept_q)) {
> +               next = &list_first_entry(&bt->accept_q,
> +                                        struct bt_sock, accept_q)->sk;
> +               sock_hold(next);
> +       }
> +
> +out:
> +       spin_unlock_bh(&bt->accept_q_lock);
> +       return next;
> +}

Hmm. This looks a bit complicated to me, and I find it hard to reason
about how accept_q walks are restarted after temporarily dropping the
lock; I think it would be nice if you could instead walk the
->accept_q while holding the accept_q_lock until you identify a socket
with the right ->sk_state, then drop the accept_q_lock and lock the
sock. Something like this diff on top of your patch (completely
untested); I have attached a properly formatted version of this diff
that you can apply with "git apply":

```
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 9d68dd86023c..26e7c7198522 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -271,50 +271,36 @@ void bt_accept_unlink(struct sock *sk)
 }
 EXPORT_SYMBOL(bt_accept_unlink);

-static struct sock *bt_accept_get(struct sock *parent, struct sock *sk)
-{
-       struct bt_sock *bt = bt_sk(parent);
-       struct sock *next = NULL;
-
-       /* accept_q is modified from child teardown paths too, so take a
-        * temporary reference before dropping the queue lock.
-        */
-       spin_lock_bh(&bt->accept_q_lock);
-
-       if (sk) {
-               if (bt_sk(sk)->parent != parent)
-                       goto out;
-
-               if (!list_is_last(&bt_sk(sk)->accept_q, &bt->accept_q)) {
-                       next = &list_next_entry(bt_sk(sk), accept_q)->sk;
-                       sock_hold(next);
-               }
-       } else if (!list_empty(&bt->accept_q)) {
-               next = &list_first_entry(&bt->accept_q,
-                                        struct bt_sock, accept_q)->sk;
-               sock_hold(next);
-       }
-
-out:
-       spin_unlock_bh(&bt->accept_q_lock);
-       return next;
-}
-
 struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 {
-       struct sock *sk, *next;
+       struct bt_sock *s;
+       struct sock *sk;

        BT_DBG("parent %p", parent);

 restart:
-       for (sk = bt_accept_get(parent, NULL); sk; sk = next) {
+       spin_lock_bh(&bt_sk(parent)->accept_q_lock);
+       list_for_each_entry(s, &bt_sk(parent)->accept_q, accept_q) {
+               unsigned char state;
+
+               sk = &s->sk;
+
+               /* lockless version of the checks below */
+               state = data_race(READ_ONCE(sk->sk_state));
+               if (state != BT_CLOSED && state != BT_CONNECTED && newsock &&
+                   !test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags))
+                       continue;
+
                /* Prevent early freeing of sk due to unlink and sock_kill */
+               sock_hold(sk);
+               spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
                lock_sock(sk);
+               /* socket is now locked, redo checks reliably */

                /* Check sk has not already been unlinked via
                 * bt_accept_unlink() due to serialisation caused by sk locking
                 */
-               if (bt_sk(sk)->parent != parent) {
+               if (s->parent != parent) {
                        BT_DBG("sk %p, already unlinked", sk);
                        release_sock(sk);
                        sock_put(sk);
@@ -322,8 +308,6 @@ struct sock *bt_accept_dequeue(struct sock
*parent, struct socket *newsock)
                        goto restart;
                }

-               next = bt_accept_get(parent, sk);
-
                /* sk is safely in the parent list so reduce reference count */
                sock_put(sk);

@@ -331,7 +315,7 @@ struct sock *bt_accept_dequeue(struct sock
*parent, struct socket *newsock)
                if (sk->sk_state == BT_CLOSED) {
                        bt_accept_unlink(sk);
                        release_sock(sk);
-                       continue;
+                       goto restart;
                }

                if (sk->sk_state == BT_CONNECTED || !newsock ||
@@ -341,12 +325,11 @@ struct sock *bt_accept_dequeue(struct sock
*parent, struct socket *newsock)
                                sock_graft(sk, newsock);

                        release_sock(sk);
-                       if (next)
-                               sock_put(next);
                        return sk;
                }

                release_sock(sk);
+               goto restart;
        }

        return NULL;
```

I think this makes the code simpler, and it reduces the line count;
however, I think your approach is okay too, so it would also be fine
to keep your approach if you prefer.

[...]
> @@ -518,18 +551,28 @@ EXPORT_SYMBOL(bt_sock_stream_recvmsg);
>
>  static inline __poll_t bt_accept_poll(struct sock *parent)
>  {
> -       struct bt_sock *s, *n;
> +       struct bt_sock *bt = bt_sk(parent);
> +       struct bt_sock *s;
>         struct sock *sk;
> +       __poll_t mask = 0;
> +
> +       spin_lock_bh(&bt->accept_q_lock);
> +       list_for_each_entry(s, &bt->accept_q, accept_q) {
> +               int state;
>
> -       list_for_each_entry_safe(s, n, &bt_sk(parent)->accept_q, accept_q) {
>                 sk = (struct sock *)s;
> -               if (sk->sk_state == BT_CONNECTED ||
> -                   (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags) &&
> -                    sk->sk_state == BT_CONNECT2))
> -                       return EPOLLIN | EPOLLRDNORM;
> +               state = READ_ONCE(sk->sk_state);

nitpick: This READ_ONCE() is not synchronized with a corresponding
WRITE_ONCE(); that's not really clean, and it might be appropriate to
mark this with data_race() if this is intentionally racy with
potentially-torn stores. But that's a minor detail.


> +
> +               if (state == BT_CONNECTED ||
> +                   (test_bit(BT_SK_DEFER_SETUP, &bt->flags) &&
> +                    state == BT_CONNECT2)) {
> +                       mask = EPOLLIN | EPOLLRDNORM;
> +                       break;
> +               }
>         }
> +       spin_unlock_bh(&bt->accept_q_lock);
>
> -       return 0;
> +       return mask;
>  }
>
>  __poll_t bt_sock_poll(struct file *file, struct socket *sock,
> --
> 2.34.1
>

[-- Attachment #2: locked-walk.diff --]
[-- Type: application/x-patch, Size: 2914 bytes --]

Re: [PATCH v4] Bluetooth: serialize accept_q access

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Wed,  6 May 2026 19:43:30 +0800 you wrote:
> From: Jiexun Wang <wangjiexun2025@gmail.com>
> 
> bt_sock_poll() walks the accept queue without synchronization, while
> child teardown can unlink the same socket and drop its last reference.
> The unsynchronized accept queue walk has existed since the initial
> Bluetooth import.
> 
> [...]

Here is the summary with links:
  - [v4] Bluetooth: serialize accept_q access
    https://git.kernel.org/bluetooth/bluetooth-next/c/303bd23ee2e9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html