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

[Pauli Virtanen <pav@iki.fi>]

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