Bluetooth: serialize accept_q access

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>
Signed-off-by: Jiexun Wang <wangjiexun2025@gmail.com>
Reviewed-by: Jann Horn <jannh@google.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 69eed69f7f265667409cc8ba19d944c093ada0cc..3faea66b19799af241bc0479cf9e90d5066f5c0e 100644 (file)
--- 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 33d053d634072fee79bb62c003980c203fb30111..9d68dd86023c9d1d19bc27644d0d4e8dfdcac536 100644 (file)
--- 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 @@ restart:
                                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,