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

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