[PATCH] Bluetooth: ISO: Serialize concurrent connect calls

[PATCH] Bluetooth: ISO: Serialize concurrent connect calls

[Chengfeng Ye]

iso_sock_connect() checks the socket state before taking the socket lock
and drops the lock again before setting up a connection. Two callers can
both pass the admission check while the socket is open or bound.

Caller A can copy its destination for route selection, then caller B can
replace the socket destination before A binds or connects the CIS. A
then uses B's destination with the route selected for its own request.
B can also proceed after A attaches a connection and sets BT_CONNECT.
For deferred BIS setup, B can bind a second BIS and overwrite
iso_pi(sk)->conn, leaving the first connection's reference and conn->sk
back-pointer stranded. Concurrent CIS connects can likewise replace the
socket's connection. Later teardown only detaches the current connection,
so a callback on the old connection can race with socket release and
access a freed socket.

KASAN reported:

  BUG: KASAN: slab-use-after-free in iso_sock_hold+0xf7/0x1b0
  Call Trace:
   iso_sock_hold+0xf7/0x1b0
   iso_conn_del+0x7b/0x1d0
   iso_connect_cfm+0x186/0x16a0
   hci_conn_failed+0x154/0x280
   hci_abort_conn_sync+0x3dc/0x7d0
   hci_cmd_sync_work+0x173/0x300

  Allocated by task 121:
   sk_alloc+0x2b/0x6d0
   bt_sock_alloc+0x29/0x370
   iso_sock_alloc.constprop.0+0x19/0x300
   iso_sock_create+0x94/0x100

  Freed by task 121:
   kfree+0x121/0x3c0
   __sk_destruct+0x42b/0x540
   iso_sock_release+0x29d/0x340
   __sock_release+0xa1/0x260

Check admission under the socket lock and mark the connect operation in
progress before publishing its destination. Keep that flag set across
route lookup and connection setup, rejecting another connect with
-EBADFD before it can change the destination or attach a connection.
Clear the flag after either helper returns, including on failure, so a
failed setup can be retried.

A separate flag is needed because the socket lock must be released before
acquiring the HCI device lock, while BT_CONNECT requires an attached
connection. Keep the existing state transitions and the deferred CIS
completion through iso_sock_recvmsg().

Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
Cc: stable@vger.kernel.org
Assisted-by: GPT-6 Astra
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/bluetooth/iso.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 7657c2a0abbf..8113367c796e 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -61,6 +61,7 @@ enum {
 	BT_SK_BIG_SYNC,
 	BT_SK_PA_SYNC,
 	BT_SK_KILLED,
+	BT_SK_CONNECTING,
 };
 
 struct iso_pinfo {
@@ -1269,17 +1270,26 @@ static int iso_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 	    addr->sa_family != AF_BLUETOOTH)
 		return -EINVAL;
 
-	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
-		return -EBADFD;
+	lock_sock(sk);
 
-	if (sk->sk_type != SOCK_SEQPACKET)
-		return -EINVAL;
+	if ((sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) ||
+	    test_bit(BT_SK_CONNECTING, &iso_pi(sk)->flags)) {
+		err = -EBADFD;
+		goto done;
+	}
+
+	if (sk->sk_type != SOCK_SEQPACKET) {
+		err = -EINVAL;
+		goto done;
+	}
 
 	/* Check if the address type is of LE type */
-	if (!bdaddr_type_is_le(sa->iso_bdaddr_type))
-		return -EINVAL;
+	if (!bdaddr_type_is_le(sa->iso_bdaddr_type)) {
+		err = -EINVAL;
+		goto done;
+	}
 
-	lock_sock(sk);
+	set_bit(BT_SK_CONNECTING, &iso_pi(sk)->flags);
 
 	bacpy(&iso_pi(sk)->dst, &sa->iso_bdaddr);
 	iso_pi(sk)->dst_type = sa->iso_bdaddr_type;
@@ -1291,16 +1301,18 @@ static int iso_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 	else
 		err = iso_connect_bis(sk);
 
-	if (err)
-		return err;
-
 	lock_sock(sk);
 
+	clear_bit(BT_SK_CONNECTING, &iso_pi(sk)->flags);
+	if (err)
+		goto done;
+
 	if (!test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
 		err = bt_sock_wait_state(sk, BT_CONNECTED,
 					 sock_sndtimeo(sk, flags & O_NONBLOCK));
 	}
 
+done:
 	release_sock(sk);
 	return err;
 }
-- 
2.43.0

Re: [PATCH] Bluetooth: ISO: Serialize concurrent connect calls

[Pauli Virtanen]

Hi,

ma, 2026-10-05 kello 00:24 +0800, Chengfeng Ye kirjoitti:
> iso_sock_connect() checks the socket state before taking the socket lock
> and drops the lock again before setting up a connection. Two callers can
> both pass the admission check while the socket is open or bound.
> 
> Caller A can copy its destination for route selection, then caller B can
> replace the socket destination before A binds or connects the CIS. A
> then uses B's destination with the route selected for its own request.
> B can also proceed after A attaches a connection and sets BT_CONNECT.
> For deferred BIS setup, B can bind a second BIS and overwrite
> iso_pi(sk)->conn, leaving the first connection's reference and conn->sk
> back-pointer stranded. Concurrent CIS connects can likewise replace the

Memory safety probably should be enforced somewhat down the calls,
likely __iso_chan_add() should reject adding a different iso_conn to sk
if it already has one, since that looks like it leaks the
iso_conn_hold() reference and the back pointer association.

> socket's connection. Later teardown only detaches the current connection,
> so a callback on the old connection can race with socket release and
> access a freed socket.
> 
> KASAN reported:
> 
>   BUG: KASAN: slab-use-after-free in iso_sock_hold+0xf7/0x1b0
>   Call Trace:
>    iso_sock_hold+0xf7/0x1b0
>    iso_conn_del+0x7b/0x1d0
>    iso_connect_cfm+0x186/0x16a0
>    hci_conn_failed+0x154/0x280
>    hci_abort_conn_sync+0x3dc/0x7d0
>    hci_cmd_sync_work+0x173/0x300
> 
>   Allocated by task 121:
>    sk_alloc+0x2b/0x6d0
>    bt_sock_alloc+0x29/0x370
>    iso_sock_alloc.constprop.0+0x19/0x300
>    iso_sock_create+0x94/0x100
> 
>   Freed by task 121:
>    kfree+0x121/0x3c0
>    __sk_destruct+0x42b/0x540
>    iso_sock_release+0x29d/0x340
>    __sock_release+0xa1/0x260
> 
> Check admission under the socket lock and mark the connect operation in
> progress before publishing its destination. Keep that flag set across
> route lookup and connection setup, rejecting another connect with
> -EBADFD before it can change the destination or attach a connection.
> Clear the flag after either helper returns, including on failure, so a
> failed setup can be retried.
> 
> A separate flag is needed because the socket lock must be released before
> acquiring the HCI device lock, while BT_CONNECT requires an attached
> connection. Keep the existing state transitions and the deferred CIS
> completion through iso_sock_recvmsg().
> 
> Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> Cc: stable@vger.kernel.org
> Assisted-by: GPT-6 Astra
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
>  net/bluetooth/iso.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 7657c2a0abbf..8113367c796e 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -61,6 +61,7 @@ enum {
>  	BT_SK_BIG_SYNC,
>  	BT_SK_PA_SYNC,
>  	BT_SK_KILLED,
> +	BT_SK_CONNECTING,
>  };
>  
>  struct iso_pinfo {
> @@ -1269,17 +1270,26 @@ static int iso_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  	    addr->sa_family != AF_BLUETOOTH)
>  		return -EINVAL;
>  
> -	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
> -		return -EBADFD;
> +	lock_sock(sk);
>  
> -	if (sk->sk_type != SOCK_SEQPACKET)
> -		return -EINVAL;
> +	if ((sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) ||
> +	    test_bit(BT_SK_CONNECTING, &iso_pi(sk)->flags)) {
> +		err = -EBADFD;
> +		goto done;
> +	}
> +
> +	if (sk->sk_type != SOCK_SEQPACKET) {
> +		err = -EINVAL;
> +		goto done;
> +	}
>  
>  	/* Check if the address type is of LE type */
> -	if (!bdaddr_type_is_le(sa->iso_bdaddr_type))
> -		return -EINVAL;
> +	if (!bdaddr_type_is_le(sa->iso_bdaddr_type)) {
> +		err = -EINVAL;
> +		goto done;
> +	}
>  
> -	lock_sock(sk);
> +	set_bit(BT_SK_CONNECTING, &iso_pi(sk)->flags);
>  
>  	bacpy(&iso_pi(sk)->dst, &sa->iso_bdaddr);
>  	iso_pi(sk)->dst_type = sa->iso_bdaddr_type;
> @@ -1291,16 +1301,18 @@ static int iso_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  	else
>  		err = iso_connect_bis(sk);
>  
> -	if (err)
> -		return err;
> -
>  	lock_sock(sk);
>  
> +	clear_bit(BT_SK_CONNECTING, &iso_pi(sk)->flags);
> +	if (err)
> +		goto done;
> +
>  	if (!test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
>  		err = bt_sock_wait_state(sk, BT_CONNECTED,
>  					 sock_sndtimeo(sk, flags & O_NONBLOCK));
>  	}
>  
> +done:
>  	release_sock(sk);
>  	return err;
>  }

-- 
Pauli Virtanen