Bluetooth: L2CAP: reject accept queue add unless BT_LISTEN

New sk should not be added to parent socket accept queue after last
l2cap_sock_cleanup_listen() has run in l2cap_sock_teardown_cb() and
state set to BT_CLOSED, as that can result to UAF on dereferencing the
dangling parent reference.

l2cap_sock_new_connection_cb() may race with parent l2cap_chan teardown,
due to chan->state accessed without consistent locking:

  [Task 1]                           [Task 2]
  l2cap_sock_release(parent)         l2cap_connect
    l2cap_sock_shutdown                pchan = l2cap_global_chan_by_psm
      l2cap_chan_lock(pchan)
      l2cap_chan_close
        l2cap_sock_teardown_cb
          pchan->state = BT_CLOSED
      l2cap_chan_unlock(pchan) ------> l2cap_chan_lock(pchan)
                                       l2cap_new_connection
                                         l2cap_sock_new_connection_cb
      l2cap_chan_lock(pchan) <-------- l2cap_chan_unlock(pchan)
      l2cap_sock_kill(parent)          /* bt_sk(sk)->parent dangling */

Fix by adding check for sk_state == BT_LISTEN after acquiring sk lock in
l2cap_sock_new_connection_cb().  Add lock_sock() around sk_state writes
where missing, to avoid data races.

Although the data races on pchan->state should be fixed too, this
defensive sk_state check probably makes sense in any case.

Fixes: 2ff1a41a912d ("Bluetooth: L2CAP: Fix null-ptr-deref in l2cap_sock_state_change_cb()")
Reported-by: syzbot+9265e754091c2d27ea29@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9265e754091c2d27ea29
Signed-off-by: Pauli Virtanen <pav@iki.fi>
Reported-by: syzbot+9265e754091c2d27ea29@syzkaller.appspotmail.com
Tested-by: syzbot+9265e754091c2d27ea29@syzkaller.appspotmail.com
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index cca6201..8bf35bc 100644 (file)
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1600,6 +1600,11 @@ static int l2cap_sock_new_connection_cb(struct l2cap_chan *chan,
 
        lock_sock(parent);
 
+       if (parent->sk_state != BT_LISTEN) {
+               release_sock(parent);
+               return -EINVAL;
+       }
+
        /* Check for backlog size */
        if (sk_acceptq_is_full(parent)) {
                BT_DBG("backlog full %d", parent->sk_ack_backlog);
@@ -1763,10 +1768,14 @@ static void l2cap_sock_state_change_cb(struct l2cap_chan *chan, int state,
        if (!sk)
                return;
 
+       lock_sock(sk);
+
        sk->sk_state = state;
 
        if (err)
                sk->sk_err = err;
+
+       release_sock(sk);
 }
 
 static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
@@ -1842,6 +1851,8 @@ static void l2cap_sock_resume_cb(struct l2cap_chan *chan)
        if (!sk)
                return;
 
+       lock_sock(sk);
+
        if (test_and_clear_bit(FLAG_PENDING_SECURITY, &chan->flags)) {
                sk->sk_state = BT_CONNECTED;
                chan->state = BT_CONNECTED;
@@ -1849,6 +1860,8 @@ static void l2cap_sock_resume_cb(struct l2cap_chan *chan)
 
        clear_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags);
        sk->sk_state_change(sk);
+
+       release_sock(sk);
 }
 
 static void l2cap_sock_set_shutdown_cb(struct l2cap_chan *chan)