Bluetooth: RFCOMM: missing sock_hold() in rfcomm_get_sock_by_channel()

Bluetooth: RFCOMM: missing sock_hold() in rfcomm_get_sock_by_channel()

[y2k]

Commit 4e37f6452d58 ("Bluetooth: SCO: hold sk properly in sco_conn_ready")
fixed a UAF in SCO by adding sock_hold() before returning a socket pointer
from sco_get_sock_listen(). The RFCOMM counterpart has the same problem.

rfcomm_get_sock_by_channel() returns a socket pointer without holding a
reference. The caller rfcomm_connect_ind() then calls lock_sock() on the
returned pointer:

  parent = rfcomm_get_sock_by_channel(BT_LISTEN, channel, &src);
  if (!parent)
      return 0;
  lock_sock(parent);   /* UAF if parent was freed */

rfcomm_get_sock_by_channel() holds read_lock(&rfcomm_sk_list.lock) during
the search but releases it before returning. Between the return and the
lock_sock() call, the socket can be closed and freed by another thread
since rfcomm_sock_release() does not take rfcomm_lock().

rfcomm_connect_ind() is called under rfcomm_lock() but the socket close
path does not take rfcomm_lock(), so there is no mutual exclusion.

The race condition is:

  Thread 1 (rfcomm_connect_ind)    Thread 2 (rfcomm_sock_release)
  rfcomm_get_sock_by_channel()
    returns parent (no sock_hold)
                                    __rfcomm_sock_close()
                                      SOCK_ZAPPED set
                                    sock_orphan() -> sk_socket = NULL
                                    rfcomm_sock_kill()
                                      sock_put() -> refcount=0 -> FREE
  lock_sock(parent)  <- UAF

The fix should mirror 4e37f6452d58: add sock_hold() before returning
the socket in rfcomm_get_sock_by_channel(), and sock_put() after
lock_sock() in rfcomm_connect_ind().

Fixes: b7ce436a5d79 ("Bluetooth: switch to lock_sock in RFCOMM")
Reported-by: y2k <y2k@desarrollaria.com>

Thanks,
y2k
y2k@desarrollaria.com