linux-kernel - Re:Re: [PATCH v3] bluetooth: sco: Serialize state check in sco_sock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <534d8927.34d1.19a4047200e.Coremail.zzzccc427@163.com>
Date: Sun, 2 Nov 2025 00:36:34 +0800 (CST)
From: zzzccc427  <zzzccc427@....com>
To: "Pauli Virtanen" <pav@....fi>
Cc: "Luiz Augusto von Dentz" <luiz.dentz@...il.com>, johan.hedberg@...il.com,
	marcel@...tmann.org, linux-kernel@...r.kernel.org,
	linux-bluetooth@...r.kernel.org, baijiaju1990@...il.com,
	r33s3n6@...il.com, gality369@...il.com, zhenghaoran154@...il.com
Subject: Re:Re: [PATCH v3] bluetooth: sco: Serialize state check in
 sco_sock_connect to fix UAF

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