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

[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@iki.fi>
Closes: https://lore.kernel.org/linux-bluetooth/5e76a95e934e451e7006db28827c2d64af5a88be.camel@iki.fi/
Reported-by: syzbot+74071deb72339c215b2e@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-bluetooth/6a92fadc.08e933ee.dbf97.008f.GAE@google.com/
Cc: stable@vger.kernel.org
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
---

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

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

[Pauli Virtanen]

Hi,

to, 2026-09-03 kello 04:51 +0500, Mikhail Gavrilov kirjoitti:
> 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.

Sashiko review comment that security_cfm() cleanup should be run in
rfcomm_init() after hci_unregister_cb(), appears correct.

AFAICS, none of the callsites of hci_auth_cfm(), which call the
security_cfm, require that it is synchronous under the lock. 

However, rfcomm_session_get() could return a different session if
processing is delayed. Is this a concern? ABA issue?

This patch introduces data race in read of conn->cfm->sec_level,
probably benign, but these are not marked with READ_ONCE/WRITE_ONCE.

I'd maybe take hdev_lock in rfcomm_process_security_cfm instead. This
requires struct hci_dev *hdev; added in rfcomm_sec_cfm and hci_dev
get/put, since hci_conn_get() does not guarantee hci_conn::hdev is
valid pointer.

I'd maybe also add Documentation/dev-tools/context-analysis.rst
annotations while at it, unless it requires extensive changes.

> Fixes: 759c185d0bbd ("Bluetooth: RFCOMM: serialize security confirmation handling")
> Reported-by: Pauli Virtanen <pav@iki.fi>
> Closes: https://lore.kernel.org/linux-bluetooth/5e76a95e934e451e7006db28827c2d64af5a88be.camel@iki.fi/
> Reported-by: syzbot+74071deb72339c215b2e@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-bluetooth/6a92fadc.08e933ee.dbf97.008f.GAE@google.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> ---
> 
> 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);

Context analysis annotations would be useful here:

static DEFINE_SPINLOCK(security_cfm_lock);
static __guarded_by(&security_cfm_lock) LIST_HEAD(security_cfm_list);

struct rfcomm_sec_cfm {
	struct list_head	list __guarded_by(&security_cfm_lock);
	struct hci_conn		*conn;
	bdaddr_t		src;
	u8			status;
	u8			encrypt;
};

Static checker can be run with Clang 23, 
make LLVM=1 net/bluetooth/

>  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)

__must_hold(&rfcomm_mutex) annotation is better than comment, although
would be needed also in the caller.

> +{
> +	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();
>  }

-- 
Pauli Virtanen

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

[Mikhail Gavrilov]

Hi Pauli,
thanks for the review.

> Sashiko review comment that security_cfm() cleanup should be run in
> rfcomm_init() after hci_unregister_cb(), appears correct.

Correct, and rfcomm_init() is exactly where it matters: the error path
runs kthread_stop() at the stop: label and hci_unregister_cb() only at
unregister:, so a confirmation queued in between would have outlived the
thread that frees it.  v2 has rfcomm_flush_security_cfm(), which just
drops the queued entries, called after hci_unregister_cb() in
rfcomm_init() and after kthread_stop() in rfcomm_exit().  The drain at
the end of rfcomm_run() is gone.

> However, rfcomm_session_get() could return a different session if
> processing is delayed. Is this a concern? ABA issue?

It is.  Nothing keeps a session alive while the confirmation waits:
__rfcomm_dlc_close() can drop the last DLC and rfcomm_dlc_open() can
create a new session to the same peer, both from a syscall, without
krfcommd running in between.  The lookup is by (src, dst), so the new
session matches and a stale RFCOMM_AUTH_ACCEPT can be set on a DLC that
asked for BT_SECURITY_HIGH while the current link is weaker.

v2 compares the session's hci_conn with the one the confirmation was
reported for and skips the session if they differ.  Comparing pointers
is safe because the entry holds a reference on the connection, so the
object cannot be freed and reused while it is queued.

> This patch introduces data race in read of conn->cfm->sec_level,
> probably benign, but these are not marked with READ_ONCE/WRITE_ONCE.
>
> I'd maybe take hdev_lock in rfcomm_process_security_cfm instead.

Done that way in v2: the entry pins the controller with hci_dev_hold()
and krfcommd takes hdev->lock around applying the confirmation, so
conn->sec_level is read in the same context as before the change and
nothing needs new READ_ONCE()/WRITE_ONCE() annotations.  rfcomm_mutex ->
hdev->lock is the order rfcomm_dlc_open() already uses, so this does not
bring the cycle back.

> Context analysis annotations would be useful here:

Added __guarded_by(&security_cfm_lock) on security_cfm_list and
__must_hold(&rfcomm_mutex) on __rfcomm_security_cfm().

I left the list_head member of struct rfcomm_sec_cfm unannotated:
rfcomm_process_security_cfm() moves the whole queue to a local list with
list_splice_init() and from then on the entries are private to that
thread, so the list_del() and the free happen without the spinlock and
__guarded_by(&security_cfm_lock) on the member would flag correct code.

I could not build with LLVM here - no clang 23 - so the annotations are
by inspection only, gcc W=1 and checkpatch --strict are clean.  If you
have the checker at hand a run on net/bluetooth/rfcomm/ would be
welcome.

-- 
Thanks,
Mikhail

[PATCH v2] 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 pins both the connection and the controller, and
krfcommd takes hdev->lock while applying it, so the lookup and
hci_conn_check_secure() run in the same context as before.  A session
that was torn down and set up again while the confirmation was queued
runs over a different hci_conn and is skipped.  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@iki.fi>
Closes: https://lore.kernel.org/linux-bluetooth/5e76a95e934e451e7006db28827c2d64af5a88be.camel@iki.fi/
Reported-by: syzbot+74071deb72339c215b2e@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-bluetooth/6a92fadc.08e933ee.dbf97.008f.GAE@google.com/
Cc: stable@vger.kernel.org
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
---

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.

v1: https://lore.kernel.org/linux-bluetooth/20260902235132.453044-1-mikhail.v.gavrilov@gmail.com/

v2:
 - free queued confirmations from rfcomm_init() and rfcomm_exit() after
   hci_unregister_cb(), instead of at the end of rfcomm_run(); the init
   error path stops the thread before unregistering the callback, so
   the old placement leaked there
 - pin the controller too and take hdev->lock while the confirmation is
   applied, so conn->sec_level is read in the same context as before
 - skip a session that runs over a different hci_conn than the one the
   confirmation was reported for
 - context-analysis annotations for security_cfm_list and
   __rfcomm_security_cfm(); not verified with clang, done by inspection
 - the reproducer below now uses spaces, gitlint tripped over the tabs

Tested on 7.3.0-rc1 with an MT7922 controller (btusb).  Without the
patch the steps above report the inversion on every run.  With v2
applied the reproducer leaves the validator armed and silent
(debug_locks: 1), and a 2.5 hour session with three BR/EDR headsets
(soundcore Liberty 5, FIIO UTWS17, JBL Tour Pro 3), HFP/SCO audio and
AVRCP produced no lockdep report.

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 | 178 ++++++++++++++++++++++++++++--------
 1 file changed, 140 insertions(+), 38 deletions(-)

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index f7463f092283..246c811dfca1 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_dev		*hdev;
+	struct hci_conn		*conn;
+	u8			status;
+	u8			encrypt;
+};
+
+static DEFINE_SPINLOCK(security_cfm_lock);
+static __guarded_by(&security_cfm_lock) LIST_HEAD(security_cfm_list);
+
 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,117 @@ static void rfcomm_process_sessions(void)
 	rfcomm_unlock();
 }
 
+static void rfcomm_sec_cfm_free(struct rfcomm_sec_cfm *cfm)
+{
+	hci_conn_put(cfm->conn);
+	hci_dev_put(cfm->hdev);
+	kfree(cfm);
+}
+
+static struct hci_conn *rfcomm_session_hcon(struct rfcomm_session *s)
+{
+	struct l2cap_conn *conn = l2cap_pi(s->sock->sk)->chan->conn;
+
+	return conn ? conn->hcon : NULL;
+}
+
+static void __rfcomm_security_cfm(struct rfcomm_sec_cfm *cfm)
+	__must_hold(&rfcomm_mutex)
+{
+	struct rfcomm_session *s;
+	struct rfcomm_dlc *d, *n;
+
+	s = rfcomm_session_get(&cfm->hdev->bdaddr, &cfm->conn->dst);
+	if (!s)
+		return;
+
+	/* The confirmation belongs to the link it was reported for.  A
+	 * session that was torn down and set up again in the meantime runs
+	 * over a different connection and must not be judged by it.
+	 */
+	if (rfcomm_session_hcon(s) != cfm->conn)
+		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) {
+		/* Restores the context the callback used to run in, so that
+		 * hci_conn_check_secure() sees a stable sec_level.
+		 */
+		hci_dev_lock(cfm->hdev);
+		__rfcomm_security_cfm(cfm);
+		hci_dev_unlock(cfm->hdev);
+
+		list_del(&cfm->list);
+		rfcomm_sec_cfm_free(cfm);
+	}
+
+	rfcomm_unlock();
+}
+
+/* Drops confirmations that krfcommd will not get to any more.  Called once
+ * the HCI callback is unregistered and the thread is gone.
+ */
+static void rfcomm_flush_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);
+
+	list_for_each_entry_safe(cfm, n, &cfm_list, list) {
+		list_del(&cfm->list);
+		rfcomm_sec_cfm_free(cfm);
+	}
+}
+
 static int rfcomm_add_listener(bdaddr_t *ba)
 {
 	struct sockaddr_l2 addr;
@@ -2201,6 +2324,7 @@ 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);
@@ -2214,50 +2338,25 @@ 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);
-	}
+	/* hci_conn drops its own reference on hdev once it is deleted, so
+	 * both objects are pinned until krfcommd is done with them.
+	 */
+	cfm->hdev = hci_dev_hold(conn->hdev);
+	cfm->conn = hci_conn_get(conn);
+	cfm->status = status;
+	cfm->encrypt = encrypt;
 
-	rfcomm_unlock();
+	spin_lock(&security_cfm_lock);
+	list_add_tail(&cfm->list, &security_cfm_list);
+	spin_unlock(&security_cfm_lock);
 
 	rfcomm_schedule();
 }
@@ -2333,6 +2432,7 @@ static int __init rfcomm_init(void)
 
 unregister:
 	hci_unregister_cb(&rfcomm_cb);
+	rfcomm_flush_security_cfm();
 
 	return err;
 }
@@ -2345,6 +2445,8 @@ static void __exit rfcomm_exit(void)
 
 	kthread_stop(rfcomm_thread);
 
+	rfcomm_flush_security_cfm();
+
 	rfcomm_cleanup_ttys();
 
 	rfcomm_cleanup_sockets();
-- 
2.55.0

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

[Pauli Virtanen]

Hi,

pe, 2026-09-04 kello 06:20 +0500, Mikhail Gavrilov kirjoitti:
> 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.

The delayed processing of the security confirmations is somewhat hard
for me to reason about.

This v2 checks session hci_conn is the same as original, however it is
unclear if delayed processing of confirmation on the same hci_conn, can
result to wrong outcomes.

What makes it not introduce new race conditions?

My best guess is that synchronization between DLC events and security
CFM was questionable also before, so this would only widen existing
race windows. GPT-5.6 produced report of pre-existing race condition
where security_cfm() races with DLC open and results to intermittent
wrong security level, but I didn't verify this was not nonsense.

I wonder if the kernel_connect() could be moved out from under
rfcomm_mutex, since the RFCOMM channels should already have to handle
transition to CONNECTED state and possible failures there, and the lock
cycle solved from the other side.

LLVM is happy with the context analysis annotations in v2.

> 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 pins both the connection and the controller, and
> krfcommd takes hdev->lock while applying it, so the lookup and
> hci_conn_check_secure() run in the same context as before.  A session
> that was torn down and set up again while the confirmation was queued
> runs over a different hci_conn and is skipped.  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@iki.fi>
> Closes: https://lore.kernel.org/linux-bluetooth/5e76a95e934e451e7006db28827c2d64af5a88be.camel@iki.fi/
> Reported-by: syzbot+74071deb72339c215b2e@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-bluetooth/6a92fadc.08e933ee.dbf97.008f.GAE@google.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> ---
> 
> 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.
> 
> v1: https://lore.kernel.org/linux-bluetooth/20260902235132.453044-1-mikhail.v.gavrilov@gmail.com/
> 
> v2:
>  - free queued confirmations from rfcomm_init() and rfcomm_exit() after
>    hci_unregister_cb(), instead of at the end of rfcomm_run(); the init
>    error path stops the thread before unregistering the callback, so
>    the old placement leaked there
>  - pin the controller too and take hdev->lock while the confirmation is
>    applied, so conn->sec_level is read in the same context as before
>  - skip a session that runs over a different hci_conn than the one the
>    confirmation was reported for
>  - context-analysis annotations for security_cfm_list and
>    __rfcomm_security_cfm(); not verified with clang, done by inspection
>  - the reproducer below now uses spaces, gitlint tripped over the tabs
> 
> Tested on 7.3.0-rc1 with an MT7922 controller (btusb).  Without the
> patch the steps above report the inversion on every run.  With v2
> applied the reproducer leaves the validator armed and silent
> (debug_locks: 1), and a 2.5 hour session with three BR/EDR headsets
> (soundcore Liberty 5, FIIO UTWS17, JBL Tour Pro 3), HFP/SCO audio and
> AVRCP produced no lockdep report.
> 
> 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 | 178 ++++++++++++++++++++++++++++--------
>  1 file changed, 140 insertions(+), 38 deletions(-)
> 
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index f7463f092283..246c811dfca1 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_dev		*hdev;
> +	struct hci_conn		*conn;
> +	u8			status;
> +	u8			encrypt;
> +};
> +
> +static DEFINE_SPINLOCK(security_cfm_lock);
> +static __guarded_by(&security_cfm_lock) LIST_HEAD(security_cfm_list);
> +
>  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,117 @@ static void rfcomm_process_sessions(void)
>  	rfcomm_unlock();
>  }
>  
> +static void rfcomm_sec_cfm_free(struct rfcomm_sec_cfm *cfm)
> +{
> +	hci_conn_put(cfm->conn);
> +	hci_dev_put(cfm->hdev);
> +	kfree(cfm);
> +}
> +
> +static struct hci_conn *rfcomm_session_hcon(struct rfcomm_session *s)
> +{
> +	struct l2cap_conn *conn = l2cap_pi(s->sock->sk)->chan->conn;
> +
> +	return conn ? conn->hcon : NULL;
> +}
> +
> +static void __rfcomm_security_cfm(struct rfcomm_sec_cfm *cfm)
> +	__must_hold(&rfcomm_mutex)
> +{
> +	struct rfcomm_session *s;
> +	struct rfcomm_dlc *d, *n;
> +
> +	s = rfcomm_session_get(&cfm->hdev->bdaddr, &cfm->conn->dst);
> +	if (!s)
> +		return;
> +
> +	/* The confirmation belongs to the link it was reported for.  A
> +	 * session that was torn down and set up again in the meantime runs
> +	 * over a different connection and must not be judged by it.
> +	 */
> +	if (rfcomm_session_hcon(s) != cfm->conn)
> +		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) {
> +		/* Restores the context the callback used to run in, so that
> +		 * hci_conn_check_secure() sees a stable sec_level.
> +		 */
> +		hci_dev_lock(cfm->hdev);
> +		__rfcomm_security_cfm(cfm);
> +		hci_dev_unlock(cfm->hdev);
> +
> +		list_del(&cfm->list);
> +		rfcomm_sec_cfm_free(cfm);
> +	}
> +
> +	rfcomm_unlock();
> +}
> +
> +/* Drops confirmations that krfcommd will not get to any more.  Called once
> + * the HCI callback is unregistered and the thread is gone.
> + */
> +static void rfcomm_flush_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);
> +
> +	list_for_each_entry_safe(cfm, n, &cfm_list, list) {
> +		list_del(&cfm->list);
> +		rfcomm_sec_cfm_free(cfm);
> +	}
> +}
> +
>  static int rfcomm_add_listener(bdaddr_t *ba)
>  {
>  	struct sockaddr_l2 addr;
> @@ -2201,6 +2324,7 @@ 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);
> @@ -2214,50 +2338,25 @@ 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);
> -	}
> +	/* hci_conn drops its own reference on hdev once it is deleted, so
> +	 * both objects are pinned until krfcommd is done with them.
> +	 */
> +	cfm->hdev = hci_dev_hold(conn->hdev);
> +	cfm->conn = hci_conn_get(conn);
> +	cfm->status = status;
> +	cfm->encrypt = encrypt;
>  
> -	rfcomm_unlock();
> +	spin_lock(&security_cfm_lock);
> +	list_add_tail(&cfm->list, &security_cfm_list);
> +	spin_unlock(&security_cfm_lock);
>  
>  	rfcomm_schedule();
>  }
> @@ -2333,6 +2432,7 @@ static int __init rfcomm_init(void)
>  
>  unregister:
>  	hci_unregister_cb(&rfcomm_cb);
> +	rfcomm_flush_security_cfm();
>  
>  	return err;
>  }
> @@ -2345,6 +2445,8 @@ static void __exit rfcomm_exit(void)
>  
>  	kthread_stop(rfcomm_thread);
>  
> +	rfcomm_flush_security_cfm();
> +
>  	rfcomm_cleanup_ttys();
>  
>  	rfcomm_cleanup_sockets();

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

[mikhail.v.gavrilov]

Hi Pauli,

thanks for running the checker, and for looking at this again.

> This v2 checks session hci_conn is the same as original, however it
is
> unclear if delayed processing of confirmation on the same hci_conn,
can
> result to wrong outcomes.
>
> What makes it not introduce new race conditions?

Only two values cross the window: status and encrypt.  Everything else
is read when the confirmation is applied - the session lookup, the DLC
list, d->state, d->sec_level, and conn->sec_level in
hci_conn_check_secure(), the last one under hdev->lock as before.

Confirmations are queued and drained FIFO, and krfcommd drains the
whole queue in one pass before rfcomm_process_sessions(), so none is
dropped or reordered.  Two encryption changes in the same window are
applied in the order they arrived, and the last one wins, same as
without the queue.

That leaves stale status/encrypt applied to a DLC list that may have
changed meanwhile, and every path they feed fails closed:

  - RFCOMM_SEC_PENDING with stale status or encrypt == 0 sets
    RFCOMM_ENC_DROP, which drops the DLC;
  - a stale success does not clear anything the next confirmation would
    have acted on: if encryption really went away, that event is queued
    too and reaches step two, which re-arms SEC_PENDING for
    BT_SECURITY_MEDIUM and sets ENC_DROP for HIGH/FIPS;
  - RFCOMM_AUTH_PENDING is answered with
    !status && hci_conn_check_secure(conn, d->sec_level), and that
    function reads the live conn->sec_level.  A stale failure can only
    reject.  An accept still requires the link to be secure enough at
    the time the decision is made.

So a stale confirmation can close a DLC that would have survived; it
cannot accept one that the current state of the link does not justify.
A confirmation that cannot be allocated has the same effect - the DLC
closes on its auth timeout.

> GPT-5.6 produced report of pre-existing race condition where
> security_cfm() races with DLC open and results to intermittent wrong
> security level, but I didn't verify this was not nonsense.

There is such a window and it is older than this patch.
__rfcomm_dlc_open() sets RFCOMM_AUTH_PENDING on a new DLC when
rfcomm_check_security() finds the request still in flight, and the next
confirmation for that link clears the bit and answers with the status
of whatever event it came from, which is not necessarily the request
that DLC is waiting for.  Before this patch the callback took
rfcomm_mutex and hit exactly the same DLC as soon as the opener
released it.  The queue makes the window longer, it does not add a case
that was not reachable.  Untangling that needs per-request state on the
DLC and looks like separate work to me.

> I wonder if the kernel_connect() could be moved out from under
> rfcomm_mutex, since the RFCOMM channels should already have to handle
> transition to CONNECTED state and possible failures there, and the
lock
> cycle solved from the other side.

I agree that is the better place to fix it - it removes the inversion
for any future callback that needs rfcomm_mutex, and it leaves the
security confirmation synchronous, so none of the above has to be
reasoned about at all.  Luiz suggested the same direction on the
original thread.

It is a rework of the connect path rather than a regression fix,
though: rfcomm_session_create() would have to build and connect the
socket outside the lock, and __rfcomm_dlc_open() would have to look the
session up again after re-acquiring rfcomm_mutex and drop the socket it
just made if another thread won the race.

Luiz, which one do you want?  759c185d0bbd is in v7.3-rc1 and marked
for stable, so the inversion is in a released tree and heading for the
stable trees.  If you would rather have the small fix now and the
connect-side rework in -next, v2 is here; if you want the connect side
instead, I will write it - I would just rather not write it twice.

-- 
Thanks,
Mikhail