[PATCH] Bluetooth: RFCOMM: defer security confirmation to krfcommd

[Mikhail Gavrilov]

An RFCOMM connect() issued while a BR/EDR link is being authenticated
makes lockdep report a circular dependency, and the reported cycle is a
real AB/BA between rfcomm_mutex and hdev->lock.

rfcomm_security_cfm() is called from the HCI event path, which already
holds hdev->lock:

  hci_rx_work()
    hci_event_packet()
      hci_cc_read_enc_key_size()   [hdev->lock]
        hci_encrypt_cfm()          [hci_cb_list_lock]
          rfcomm_security_cfm()    [rfcomm_mutex]

while an RFCOMM connect() from userspace takes the same two locks the
other way round:

  rfcomm_sock_connect()
    rfcomm_dlc_open()              [rfcomm_mutex]
      __rfcomm_dlc_open()
        rfcomm_session_create()
          kernel_connect()
            l2cap_sock_connect()
              l2cap_chan_connect() [hdev->lock]

  WARNING: possible circular locking dependency detected
  kworker/u131:1/1128 is trying to acquire lock:
  rfcomm_mutex, at: rfcomm_security_cfm+0x31/0x3e0 [rfcomm]
  but task is already holding lock:
  hci_cb_list_lock, at: hci_cc_read_enc_key_size+0x1d2/0xcc0
  Chain exists of:
    rfcomm_mutex --> &hdev->lock --> hci_cb_list_lock

hci_auth_complete_evt() and hci_encrypt_change_evt() reach the callback
the same way.

Both orders have to be seen in the same boot, which is why a BR/EDR
connection alone is not enough to show it: a session set up by the
remote side is created by rfcomm_accept_connection() in krfcommd, which
calls kernel_accept() and never takes hdev->lock under rfcomm_mutex.
Connecting a device that authenticates and encrypts the link and then
calling connect() on an RFCOMM socket towards any address - the connect
does not have to succeed, the order is recorded before the page timeout
- reports it every time.

The callback does not have to run in the HCI event context at all: it
only updates DLC flags and timers that krfcommd consumes in
rfcomm_process_dlcs(), and it already ends with rfcomm_schedule().  So
queue the confirmation instead of taking rfcomm_mutex from the HCI
event path, and let krfcommd apply it under rfcomm_mutex on its next
pass, ahead of session processing.  The queued entry carries the local
address and a reference on the connection, so the session lookup and
hci_conn_check_secure() stay valid without hdev->lock.  A confirmation
that cannot be allocated is dropped and the DLC closes on its auth
timeout.

Fixes: 759c185d0bbd ("Bluetooth: RFCOMM: serialize security confirmation handling")
Reported-by: Pauli Virtanen <pav@xxxxxx>
Closes: https://lore.kernel.org/linux-bluetooth/5e76a95e934e451e7006db28827c2d64af5a88be.camel@xxxxxx/
Reported-by: syzbot+74071deb72339c215b2e@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://lore.kernel.org/linux-bluetooth/6a92fadc.08e933ee.dbf97.008f.GAE@xxxxxxxxxx/
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@xxxxxxxxx>
---

The commit this fixes is in v7.3-rc1 and is marked for stable, so this
probably wants the bluetooth fixes tree rather than -next.

Tested on 7.3.0-rc1 with an MTK MT7921 controller (btusb) and a JBL
Tour Pro 3 headset.  Without this patch the steps above report the
inversion on every run.  With it applied the reproducer leaves the
validator armed and silent (debug_locks: 1), and a 5.5 hour session
with four headset connects, HFP/SCO audio and AVRCP produced no
lockdep report either.

The connect() side used for testing, so that it does not depend on which
end sets up the HFP session:

  #include <stdint.h>
  #include <string.h>
  #include <unistd.h>
  #include <sys/socket.h>

  #define BTPROTO_RFCOMM 3

  struct sockaddr_rc {
	unsigned short	rc_family;
	uint8_t		rc_bdaddr[6];	/* little endian */
	uint8_t		rc_channel;
  };

  int main(void)
  {
	struct sockaddr_rc addr = { .rc_family = AF_BLUETOOTH,
				    .rc_channel = 1 };
	int fd = socket(AF_BLUETOOTH, SOCK_STREAM, BTPROTO_RFCOMM);

	memcpy(addr.rc_bdaddr, "\x55\x44\x33\x22\x11\x00", 6);
	connect(fd, (struct sockaddr *)&addr, sizeof(addr));
	close(fd);
	return 0;
  }

 net/bluetooth/rfcomm/core.c | 139 ++++++++++++++++++++++++++----------
 1 file changed, 100 insertions(+), 39 deletions(-)

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index f7463f092283..728a6bd2986b 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -49,6 +49,18 @@ static DEFINE_MUTEX(rfcomm_mutex);
 
 static LIST_HEAD(session_list);
 
+/* Security confirmations handed over from the HCI event handler to krfcommd */
+struct rfcomm_sec_cfm {
+	struct list_head	list;
+	struct hci_conn		*conn;
+	bdaddr_t		src;
+	u8			status;
+	u8			encrypt;
+};
+
+static LIST_HEAD(security_cfm_list);
+static DEFINE_SPINLOCK(security_cfm_lock);
+
 static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
 static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
 static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
@@ -2122,6 +2134,73 @@ static void rfcomm_process_sessions(void)
 	rfcomm_unlock();
 }
 
+/* Must be called with rfcomm_mutex held */
+static void __rfcomm_security_cfm(struct rfcomm_sec_cfm *cfm)
+{
+	struct rfcomm_session *s;
+	struct rfcomm_dlc *d, *n;
+
+	s = rfcomm_session_get(&cfm->src, &cfm->conn->dst);
+	if (!s)
+		return;
+
+	list_for_each_entry_safe(d, n, &s->dlcs, list) {
+		if (test_and_clear_bit(RFCOMM_SEC_PENDING, &d->flags)) {
+			rfcomm_dlc_clear_timer(d);
+			if (cfm->status || cfm->encrypt == 0x00) {
+				set_bit(RFCOMM_ENC_DROP, &d->flags);
+				continue;
+			}
+		}
+
+		if (d->state == BT_CONNECTED && !cfm->status &&
+		    cfm->encrypt == 0x00) {
+			if (d->sec_level == BT_SECURITY_MEDIUM) {
+				set_bit(RFCOMM_SEC_PENDING, &d->flags);
+				rfcomm_dlc_set_timer(d, RFCOMM_AUTH_TIMEOUT);
+				continue;
+			} else if (d->sec_level == BT_SECURITY_HIGH ||
+				   d->sec_level == BT_SECURITY_FIPS) {
+				set_bit(RFCOMM_ENC_DROP, &d->flags);
+				continue;
+			}
+		}
+
+		if (!test_and_clear_bit(RFCOMM_AUTH_PENDING, &d->flags))
+			continue;
+
+		if (!cfm->status && hci_conn_check_secure(cfm->conn,
+							  d->sec_level))
+			set_bit(RFCOMM_AUTH_ACCEPT, &d->flags);
+		else
+			set_bit(RFCOMM_AUTH_REJECT, &d->flags);
+	}
+}
+
+static void rfcomm_process_security_cfm(void)
+{
+	struct rfcomm_sec_cfm *cfm, *n;
+	LIST_HEAD(cfm_list);
+
+	spin_lock(&security_cfm_lock);
+	list_splice_init(&security_cfm_list, &cfm_list);
+	spin_unlock(&security_cfm_lock);
+
+	if (list_empty(&cfm_list))
+		return;
+
+	rfcomm_lock();
+
+	list_for_each_entry_safe(cfm, n, &cfm_list, list) {
+		__rfcomm_security_cfm(cfm);
+		list_del(&cfm->list);
+		hci_conn_put(cfm->conn);
+		kfree(cfm);
+	}
+
+	rfcomm_unlock();
+}
+
 static int rfcomm_add_listener(bdaddr_t *ba)
 {
 	struct sockaddr_l2 addr;
@@ -2201,12 +2280,18 @@ static int rfcomm_run(void *unused)
 	while (!kthread_should_stop()) {
 
 		/* Process stuff */
+		rfcomm_process_security_cfm();
 		rfcomm_process_sessions();
 
 		wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 	}
 	remove_wait_queue(&rfcomm_wq, &wait);
 
+	/* rfcomm_exit() unregisters the HCI callback before stopping this
+	 * thread, so no further confirmation can be queued here.
+	 */
+	rfcomm_process_security_cfm();
+
 	rfcomm_kill_listener();
 
 	return 0;
@@ -2214,50 +2299,26 @@ static int rfcomm_run(void *unused)
 
 static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt)
 {
-	struct rfcomm_session *s;
-	struct rfcomm_dlc *d, *n;
+	struct rfcomm_sec_cfm *cfm;
 
 	BT_DBG("conn %p status 0x%02x encrypt 0x%02x", conn, status, encrypt);
 
-	rfcomm_lock();
-
-	s = rfcomm_session_get(&conn->hdev->bdaddr, &conn->dst);
-	if (!s) {
-		rfcomm_unlock();
+	cfm = kmalloc_obj(*cfm);
+	if (!cfm)
 		return;
-	}
-
-	list_for_each_entry_safe(d, n, &s->dlcs, list) {
-		if (test_and_clear_bit(RFCOMM_SEC_PENDING, &d->flags)) {
-			rfcomm_dlc_clear_timer(d);
-			if (status || encrypt == 0x00) {
-				set_bit(RFCOMM_ENC_DROP, &d->flags);
-				continue;
-			}
-		}
 
-		if (d->state == BT_CONNECTED && !status && encrypt == 0x00) {
-			if (d->sec_level == BT_SECURITY_MEDIUM) {
-				set_bit(RFCOMM_SEC_PENDING, &d->flags);
-				rfcomm_dlc_set_timer(d, RFCOMM_AUTH_TIMEOUT);
-				continue;
-			} else if (d->sec_level == BT_SECURITY_HIGH ||
-				   d->sec_level == BT_SECURITY_FIPS) {
-				set_bit(RFCOMM_ENC_DROP, &d->flags);
-				continue;
-			}
-		}
-
-		if (!test_and_clear_bit(RFCOMM_AUTH_PENDING, &d->flags))
-			continue;
-
-		if (!status && hci_conn_check_secure(conn, d->sec_level))
-			set_bit(RFCOMM_AUTH_ACCEPT, &d->flags);
-		else
-			set_bit(RFCOMM_AUTH_REJECT, &d->flags);
-	}
-
-	rfcomm_unlock();
+	/* The connection is pinned for hci_conn_check_secure(), but it drops
+	 * its reference on hdev once it is deleted, so take a copy of the
+	 * local address needed for the session lookup.
+	 */
+	cfm->conn = hci_conn_get(conn);
+	bacpy(&cfm->src, &conn->hdev->bdaddr);
+	cfm->status = status;
+	cfm->encrypt = encrypt;
+
+	spin_lock(&security_cfm_lock);
+	list_add_tail(&cfm->list, &security_cfm_list);
+	spin_unlock(&security_cfm_lock);
 
 	rfcomm_schedule();
 }
-- 
2.55.0