Bluetooth: fix UAF in bt_accept_dequeue()

bt_accept_get() takes a temporary reference before dropping the accept
queue lock. bt_accept_dequeue() currently drops that reference before
bt_accept_unlink(), leaving only the queue reference.

bt_accept_unlink() drops the queue reference. The subsequent
sock_hold() therefore accesses freed memory if it was the final
reference, as observed by KASAN during listening L2CAP socket cleanup.

Retain the temporary queue-walk reference through unlink and hand it to
the caller on success. Drop it explicitly on the closed and
not-yet-connected paths.

Fixes: ab1513597c6c ("Bluetooth: fix UAF in l2cap_sock_cleanup_listen() vs l2cap_conn_del()")
Reported-by: syzbot+674ff7e4d7fdfd572afc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=674ff7e4d7fdfd572afc
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index bcbc11c9cb156b609d769736152b9123eb96f4af..a2290ffdc2c1a3b7e68b288cfd92519bac417bf8 100644 (file)
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -305,7 +305,7 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
 
 restart:
        for (sk = bt_accept_get(parent, NULL); sk; sk = next) {
-               /* Prevent early freeing of sk due to unlink and sock_kill */
+               /* The reference from bt_accept_get() keeps sk alive. */
                lock_sock(sk);
 
                /* Check sk has not already been unlinked via
@@ -321,13 +321,11 @@ restart:
 
                next = bt_accept_get(parent, sk);
 
-               /* sk is safely in the parent list so reduce reference count */
-               sock_put(sk);
-
                /* FIXME: Is this check still needed */
                if (sk->sk_state == BT_CLOSED) {
                        bt_accept_unlink(sk);
                        release_sock(sk);
+                       sock_put(sk);
                        continue;
                }
 
@@ -337,16 +335,6 @@ restart:
                        if (newsock)
                                sock_graft(sk, newsock);
 
-                       /* Hand the caller a reference taken while sk is
-                        * still locked.  bt_accept_unlink() just dropped
-                        * the accept-queue reference; without this hold a
-                        * concurrent teardown (e.g. l2cap_conn_del() ->
-                        * l2cap_sock_kill()) could free sk between
-                        * release_sock() and the caller using it.  Every
-                        * caller drops this with sock_put() when done.
-                        */
-                       sock_hold(sk);
-
                        release_sock(sk);
                        if (next)
                                sock_put(next);
@@ -354,6 +342,7 @@ restart:
                }
 
                release_sock(sk);
+               sock_put(sk);
        }
 
        return NULL;

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 4853f1b33449f0cfa235e3d308fa213809603bf0..de56ca691afa0b9c73030b26763448b500d83978 100644 (file)
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1492,8 +1492,8 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
 
        /* Close not yet accepted channels.
         *
-        * bt_accept_dequeue() now returns sk with an extra reference held
-        * (taken while sk was still locked) so a concurrent l2cap_conn_del()
+        * bt_accept_dequeue() returns sk with its temporary queue-walk
+        * reference held, so a concurrent l2cap_conn_del()
         * -> l2cap_sock_kill() cannot free sk under us.
         *
         * cleanup_listen() runs under the parent sk lock, so unlike