Bluetooth: RFCOMM: hold listener socket in rfcomm_connect_ind()

rfcomm_get_sock_by_channel() scans rfcomm_sk_list under the list lock,
but returns the selected listener after dropping that lock without
taking a reference. rfcomm_connect_ind() then locks the listener,
queues a child socket on it, and may notify it after unlocking it.

The buggy scenario involves two paths, with each column showing the
order within that path:

rfcomm_connect_ind():            listener close:
  1. Find parent in              1. close() enters
     rfcomm_get_sock_by_channel()   rfcomm_sock_release().
  2. Drop rfcomm_sk_list.lock    2. rfcomm_sock_shutdown()
     without pinning parent.        closes the listener.
  3. Call lock_sock(parent) and  3. rfcomm_sock_kill()
     bt_accept_enqueue(parent,      unlinks and puts parent.
     sk, true).
  4. Read parent flags and may   4. parent can be freed.
     call sk_state_change().

If close wins the race, parent can be freed before
rfcomm_connect_ind() reaches lock_sock(), bt_accept_enqueue(), or the
deferred-setup callback.

Take a reference on the listener before leaving rfcomm_sk_list.lock.
After lock_sock() succeeds, recheck that it is still in BT_LISTEN
before queueing a child, cache the deferred-setup bit while the parent
is locked, and drop the reference after the last parent use.

KASAN reported a slab-use-after-free in lock_sock_nested() from
rfcomm_connect_ind(), with the freeing stack going through
rfcomm_sock_kill() and rfcomm_sock_release().

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Zhang Cen <rollkingzzc@gmail.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index bd7d959c6e9eb883358c49f48c0a89116304dcea..805ed5d28ed6687a632ee64b8b36864140d4ab2d 100644 (file)
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -122,7 +122,7 @@ static struct sock *__rfcomm_get_listen_sock_by_addr(u8 channel, bdaddr_t *src)
 }
 
 /* Find socket with channel and source bdaddr.
- * Returns closest match.
+ * Returns closest match with an extra reference held.
  */
 static struct sock *rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *src)
 {
@@ -136,15 +136,25 @@ static struct sock *rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *
 
                if (rfcomm_pi(sk)->channel == channel) {
                        /* Exact match. */
-                       if (!bacmp(&rfcomm_pi(sk)->src, src))
+                       if (!bacmp(&rfcomm_pi(sk)->src, src)) {
+                               sock_hold(sk);
                                break;
+                       }
 
                        /* Closest match */
-                       if (!bacmp(&rfcomm_pi(sk)->src, BDADDR_ANY))
+                       if (!bacmp(&rfcomm_pi(sk)->src, BDADDR_ANY)) {
+                               if (sk1)
+                                       sock_put(sk1);
+
                                sk1 = sk;
+                               sock_hold(sk1);
+                       }
                }
        }
 
+       if (sk && sk1)
+               sock_put(sk1);
+
        read_unlock(&rfcomm_sk_list.lock);
 
        return sk ? sk : sk1;
@@ -941,6 +951,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 {
        struct sock *sk, *parent;
        bdaddr_t src, dst;
+       bool defer_setup = false;
        int result = 0;
 
        BT_DBG("session %p channel %d", s, channel);
@@ -954,6 +965,11 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 
        lock_sock(parent);
 
+       if (parent->sk_state != BT_LISTEN)
+               goto done;
+
+       defer_setup = test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags);
+
        /* Check for backlog size */
        if (sk_acceptq_is_full(parent)) {
                BT_DBG("backlog full %d", parent->sk_ack_backlog);
@@ -981,9 +997,11 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 done:
        release_sock(parent);
 
-       if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags))
+       if (defer_setup)
                parent->sk_state_change(parent);
 
+       sock_put(parent);
+
        return result;
 }