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

[zzzccc427 <zzzccc427@163.com>]

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