[PATCH] Bluetooth: RFCOMM: take rfcomm_mutex for the deferred setup accept

[Ali Ahmet Memis <ali@iusegentoo.com>]

rfcomm_sock_recvmsg() completes a deferred setup by calling
rfcomm_dlc_accept() without holding any RFCOMM lock:

	if (test_and_clear_bit(RFCOMM_DEFER_SETUP, &d->flags)) {
		rfcomm_dlc_accept(d);
		return 0;
	}

and rfcomm_dlc_accept() dereferences the session on its first line:

	struct sock *sk = d->session->sock->sk;

Every other path that touches d->session runs under rfcomm_mutex:
rfcomm_dlc_open(), rfcomm_dlc_close(), rfcomm_dlc_exists(),
rfcomm_dlc_send_rpn(), and the RFCOMM thread through
rfcomm_process_sessions(). rfcomm_connect_ind() is even documented as
"called under rfcomm_lock()". This call site is the only one that skips
it.

The RFCOMM_DEFER_SETUP bit looks like it serialises the accept against
teardown, since __rfcomm_dlc_close() returns early when it wins the
test_and_clear. But rfcomm_recv_disc() forces the state first:

	d->state = BT_CLOSED;
	__rfcomm_dlc_close(d, err);

and the early return only covers BT_CONNECT, BT_CONFIG, BT_OPEN and
BT_CONNECT2. With the state already BT_CLOSED that switch does not
match, the bit is never consulted, and __rfcomm_dlc_close() falls
through to rfcomm_dlc_unlink(), which sets d->session = NULL.

So a remote DISC on a deferred dlc clears the session while leaving
RFCOMM_DEFER_SETUP set. The next recvmsg() then passes the
test_and_clear and dereferences a NULL session. No timing window is
needed: once the DISC has been processed, the dereference is
unconditional.

Give rfcomm_dlc_accept() the same shape as rfcomm_dlc_open() and
rfcomm_dlc_close(): an exported wrapper that takes rfcomm_mutex and
re-checks the session, around a __rfcomm_dlc_accept() that the two
in-core callers, which already hold the mutex, keep using.

Reproduced on a KASAN + PROVE_LOCKING kernel with a BR/EDR peer emulated
over /dev/vhci: the peer brings up an ACL link, opens L2CAP on the
RFCOMM PSM, starts a session, opens a dlc on a channel bound with
BT_DEFER_SETUP, and sends DISC after the socket is accepted. recv() on
the accepted socket then hits:

  Oops: general protection fault
  KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
  RIP: 0010:rfcomm_dlc_accept+0x54/0x350
  Call Trace:
    rfcomm_sock_recvmsg+0x1cd/0x230
    sock_recvmsg+0x166/0x1c0
    __sys_recvfrom+0x20d/0x300

0x10 is the offset of sock in struct rfcomm_session. With this patch the
same run completes with recv() returning 0 and no report, and lockdep
stays quiet, confirming rfcomm_mutex is still taken before lock_sock on
this path as it is on the thread side.

Fixes: bb23c0ab8246 ("Bluetooth: Add support for deferring RFCOMM connection setup")
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 net/bluetooth/rfcomm/core.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 2e8c080b4d9e..9cdfea666a2c 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -1331,7 +1331,10 @@ static struct rfcomm_session *rfcomm_recv_disc(struct rfcomm_session *s,
 	return s;
 }
 
-void rfcomm_dlc_accept(struct rfcomm_dlc *d)
+/* Must be called with rfcomm_mutex held, so that the session cannot be
+ * unlinked from under us.
+ */
+static void __rfcomm_dlc_accept(struct rfcomm_dlc *d)
 {
 	struct sock *sk = d->session->sock->sk;
 	struct l2cap_conn *conn = l2cap_pi(sk)->chan->conn;
@@ -1353,6 +1356,21 @@ void rfcomm_dlc_accept(struct rfcomm_dlc *d)
 	rfcomm_send_msc(d->session, 1, d->dlci, d->v24_sig);
 }
 
+void rfcomm_dlc_accept(struct rfcomm_dlc *d)
+{
+	rfcomm_lock();
+
+	/* rfcomm_recv_disc() sets the dlc state to BT_CLOSED before calling
+	 * __rfcomm_dlc_close(), so the RFCOMM_DEFER_SETUP handshake there is
+	 * skipped and the session can already be unlinked by the time the
+	 * deferred accept runs from rfcomm_sock_recvmsg().
+	 */
+	if (d->session)
+		__rfcomm_dlc_accept(d);
+
+	rfcomm_unlock();
+}
+
 static void rfcomm_check_accept(struct rfcomm_dlc *d)
 {
 	if (rfcomm_check_security(d)) {
@@ -1365,7 +1383,7 @@ static void rfcomm_check_accept(struct rfcomm_dlc *d)
 			d->state_change(d, 0);
 			rfcomm_dlc_unlock(d);
 		} else
-			rfcomm_dlc_accept(d);
+			__rfcomm_dlc_accept(d);
 	} else {
 		set_bit(RFCOMM_AUTH_PENDING, &d->flags);
 		rfcomm_dlc_set_timer(d, RFCOMM_AUTH_TIMEOUT);
@@ -1958,7 +1976,7 @@ static void rfcomm_process_dlcs(struct rfcomm_session *s)
 					d->state_change(d, 0);
 					rfcomm_dlc_unlock(d);
 				} else
-					rfcomm_dlc_accept(d);
+					__rfcomm_dlc_accept(d);
 			}
 			continue;
 		} else if (test_and_clear_bit(RFCOMM_AUTH_REJECT, &d->flags)) {
-- 
2.55.0