[PATCH v3] bluetooth: sco: Serialize state check in sco_sock_connect to fix UAF

[PATCH v3] bluetooth: sco: Serialize state check in sco_sock_connect to fix UAF

[Cen Zhang]

Concurrent sco_sock_connect() calls could race on the same socket since the
state checks (BT_OPEN/BT_BOUND) were done without holding the socket lock.
This allowed two parallel connects to proceed and end up binding two
separate sco_conn objects to the same sk. Later, when sk->conn had been
updated to point to the second conn, closing the socket could free the
second conn and the socket, while the first conn's connect confirm path
still referenced the stale sk/conn, triggering a KASAN use-after-free.

Fix by taking lock_sock(sk) before checking sk->sk_state and sk->sk_type,
performing the destination address assignment under the lock, and releasing
it before invoking sco_connect() (which will acquire the lock as needed).
This serializes concurrent connect attempts for the same sk and prevents the
interleaving that caused the double-attachment and subsequent UAF.

Thread 1:               Thread 2:               Thread3:
check sk_state          check sk_state
sco_sock_connect(sk)    sco_sock_connect(sk)    sco_connect_cfm(sk->conn)
conn1->sk = sk
                        conn2->sk = sk
sk->conn = conn1
                        sk->conn = conn2
                        sco_sock_release
                        free conn2 and sk
                                                sco_connect_cfm
                                                sco_conn_del
                                                sco_conn_free
                                                UAF on sk 

The representative KASAN report excerpt:

  BUG: KASAN: slab-use-after-free in sco_conn_free net/bluetooth/sco.c:94
  ...
  Write of size 8 at addr ffff88810d2be350 by task kworker/u25:1/88
  ...
  Call Trace:
  sco_conn_free net/bluetooth/sco.c:94 [inline]
  kref_put include/linux/kref.h:65 [inline]
  sco_conn_put+0x49d/0xfc0 net/bluetooth/sco.c:115
  sco_conn_del+0x46d/0x8d0 net/bluetooth/sco.c:280
  sco_connect_cfm+0x83d/0x1ee0 net/bluetooth/sco.c:1468
  hci_connect_cfm include/net/bluetooth/hci_core.h:2082 [inline]
  ...
  Allocated by task 294:
  ...
  sco_sock_create+0x22d/0xc00 net/bluetooth/sco.c:616
  ...
  Freed by task 295:
  __sk_destruct+0x4b0/0x630 net/core/sock.c:2373
  sock_put include/net/sock.h:1962 [inline]
  sco_sock_kill+0x64d/0x9b0 net/bluetooth/sco.c:526
  sco_sock_release+0x770/0xa50 net/bluetooth/sco.c:1359
  ...

Reported-by: Cen Zhang <zzzccc427@163.com>
Signed-off-by: Cen Zhang <zzzccc427@163.com>

---
v2 and v3:
 - Fix the patch format
---

---
 net/bluetooth/sco.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index ab0cf442d..0af266880 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -651,13 +651,18 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen
 	    addr->sa_family != AF_BLUETOOTH)
 		return -EINVAL;
 
-	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
+	lock_sock(sk);
+
+	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND){
+		release_sock(sk);
 		return -EBADFD;
+	}
 
-	if (sk->sk_type != SOCK_SEQPACKET)
-		err = -EINVAL;
+	if (sk->sk_type != SOCK_SEQPACKET){
+		release_sock(sk);
+		return -EINVAL;
+	}
 
-	lock_sock(sk);
 	/* Set destination address and psm */
 	bacpy(&sco_pi(sk)->dst, &sa->sco_bdaddr);
 	release_sock(sk);
-- 
2.34.1

Re: [PATCH v3] bluetooth: sco: Serialize state check in sco_sock_connect to fix UAF

[Pauli Virtanen]

Hi,

la, 2025-11-01 kello 10:45 +0000, Cen Zhang kirjoitti:
> Concurrent sco_sock_connect() calls could race on the same socket since the
> state checks (BT_OPEN/BT_BOUND) were done without holding the socket lock.
> This allowed two parallel connects to proceed and end up binding two
> separate sco_conn objects to the same sk. Later, when sk->conn had been
> updated to point to the second conn, closing the socket could free the
> second conn and the socket, while the first conn's connect confirm path
> still referenced the stale sk/conn, triggering a KASAN use-after-free.
> 
> Fix by taking lock_sock(sk) before checking sk->sk_state and sk->sk_type,
> performing the destination address assignment under the lock, and releasing
> it before invoking sco_connect() (which will acquire the lock as needed).
> This serializes concurrent connect attempts for the same sk and prevents the
> interleaving that caused the double-attachment and subsequent UAF.
> 
> Thread 1:               Thread 2:               Thread3:
> check sk_state          check sk_state
> sco_sock_connect(sk)    sco_sock_connect(sk)    sco_connect_cfm(sk->conn)
> conn1->sk = sk
>                         conn2->sk = sk
> sk->conn = conn1
>                         sk->conn = conn2
>                         sco_sock_release
>                         free conn2 and sk
>                                                 sco_connect_cfm
>                                                 sco_conn_del
>                                                 sco_conn_free
>                                                 UAF on sk 
> 
> The representative KASAN report excerpt:
> 
>   BUG: KASAN: slab-use-after-free in sco_conn_free net/bluetooth/sco.c:94
>   ...
>   Write of size 8 at addr ffff88810d2be350 by task kworker/u25:1/88
>   ...
>   Call Trace:
>   sco_conn_free net/bluetooth/sco.c:94 [inline]
>   kref_put include/linux/kref.h:65 [inline]
>   sco_conn_put+0x49d/0xfc0 net/bluetooth/sco.c:115
>   sco_conn_del+0x46d/0x8d0 net/bluetooth/sco.c:280
>   sco_connect_cfm+0x83d/0x1ee0 net/bluetooth/sco.c:1468
>   hci_connect_cfm include/net/bluetooth/hci_core.h:2082 [inline]
>   ...
>   Allocated by task 294:
>   ...
>   sco_sock_create+0x22d/0xc00 net/bluetooth/sco.c:616
>   ...
>   Freed by task 295:
>   __sk_destruct+0x4b0/0x630 net/core/sock.c:2373
>   sock_put include/net/sock.h:1962 [inline]
>   sco_sock_kill+0x64d/0x9b0 net/bluetooth/sco.c:526
>   sco_sock_release+0x770/0xa50 net/bluetooth/sco.c:1359
>   ...
> 
> Reported-by: Cen Zhang <zzzccc427@163.com>
> Signed-off-by: Cen Zhang <zzzccc427@163.com>
> 
> ---
> v2 and v3:
>  - Fix the patch format
> ---
> 
> ---
>  net/bluetooth/sco.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index ab0cf442d..0af266880 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -651,13 +651,18 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen
>  	    addr->sa_family != AF_BLUETOOTH)
>  		return -EINVAL;
>  
> -	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
> +	lock_sock(sk);
> +
> +	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND){
> +		release_sock(sk);
>  		return -EBADFD;
> +	}
>  
> -	if (sk->sk_type != SOCK_SEQPACKET)
> -		err = -EINVAL;
> +	if (sk->sk_type != SOCK_SEQPACKET){
> +		release_sock(sk);
> +		return -EINVAL;
> +	}
>  
> -	lock_sock(sk);
>  	/* Set destination address and psm */
>  	bacpy(&sco_pi(sk)->dst, &sa->sco_bdaddr);
>  	release_sock(sk);

This looks correct in principle, sk_state shall be accessed only under
lock.

However, the lock is released before sco_connect, so looks like two
connect calls can still be at this point at the same time, so AFAICS
the above only restricts the time window for the race.

Probably something along the following is more sure. It's important the
check is under same lock_sock() in sco_connect where sk_state is
modified; in addition sk_state check could be added (or maybe moved)
also there.

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index ab0cf442d57b..06c20d99521d 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -298,7 +298,7 @@ static int sco_chan_add(struct sco_conn *conn,
struct sock *sk,
        int err = 0;
 
        sco_conn_lock(conn);
-       if (conn->sk)
+       if (conn->sk || sco_pi(sk)->conn)
                err = -EBUSY;
        else
                __sco_chan_add(conn, sk, parent);
@@ -356,6 +356,7 @@ static int sco_connect(struct sock *sk)
        err = sco_chan_add(conn, sk, NULL);
        if (err) {
                release_sock(sk);
+               hci_conn_drop(hcon);
                goto unlock;
        }

The test bot also says:

"Bluetooth: " prefix is not specified in the subject

The patch subject should start "Bluetooth: SCO:" not "bluetooth: sco:".

The following errors in test bot afaik are known pre-existing failures,
and can be ignored here:

Failed Test Cases
Read Exp Feature - Success                           Failed       0.102
seconds
LL Privacy - Add Device 3 (AL is full)               Failed       0.196
seconds
Mesh - Send cancel - 1                               Timed out    2.022
seconds
Mesh - Send cancel - 2                               Timed out    1.996
seconds

-- 
Pauli Virtanen

Re:Re: [PATCH v3] bluetooth: sco: Serialize state check in sco_sock_connect to fix UAF

[zzzccc427]

Hi Pauli,
thanks for the review and the detailed explanation.
>This looks correct in principle, sk_state shall be accessed only under
>lock.
>
>However, the lock is released before sco_connect, so looks like two
>connect calls can still be at this point at the same time, so AFAICS
>the above only restricts the time window for the race.
>
>Probably something along the following is more sure. It's important the
>check is under same lock_sock() in sco_connect where sk_state is
>modified; in addition sk_state check could be added (or maybe moved)
>also there.
>
>diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
>index ab0cf442d57b..06c20d99521d 100644
>--- a/net/bluetooth/sco.c
>+++ b/net/bluetooth/sco.c
>@@ -298,7 +298,7 @@ static int sco_chan_add(struct sco_conn *conn,
>struct sock *sk,
> int err = 0;
>
> sco_conn_lock(conn);
>- if (conn->sk)
>+ if (conn->sk || sco_pi(sk)->conn)
> err = -EBUSY;
> else
> __sco_chan_add(conn, sk, parent);
>@@ -356,6 +356,7 @@ static int sco_connect(struct sock *sk)
> err = sco_chan_add(conn, sk, NULL);
> if (err) {
> release_sock(sk);
>+ hci_conn_drop(hcon);
> goto unlock;
> }
>
You're right — my v3 only reduced the race window because I was releasing
the socket lock before calling sco_connect(), so two concurrent connect()
calls could still reach sco_connect() at roughly the same time.
>The test bot also says:
>
>"Bluetooth: " prefix is not specified in the subject
>
>The patch subject should start "Bluetooth: SCO:" not "bluetooth: sco:".
>
>The following errors in test bot afaik are known pre-existing failures,
>and can be ignored here:
>
>Failed Test Cases
>Read Exp Feature - Success Failed 0.102
>seconds
>LL Privacy - Add Device 3 (AL is full) Failed 0.196
>seconds
>Mesh - Send cancel - 1 Timed out 2.022
>seconds
>Mesh - Send cancel - 2 Timed out 1.996
>seconds
Thanks again for the guidance,this helps me a lot!
I'll send a v4 that does the following:
  - fix the subject prefix to "Bluetooth: SCO: ..."
  - in sco_chan_add(), also check sco_pi(sk)->conn and return -EBUSY if the
    socket is already attached, as you suggested:
        if (conn->sk || sco_pi(sk)->conn)
                err = -EBUSY;
  - in sco_connect(), if sco_chan_add() fails, drop the hci_conn ref
    (hci_conn_drop(hcon)) before returning

Best regards,
Cen Zhang