Re: Help needed in patching CVE-2021-3640

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Li,

On Thu, Sep 2, 2021 at 5:40 AM LinMa <linma@zju.edu.cn> wrote:
>
> Hello there,
>
> There is one bug (CVE-2021-3640: https://www.openwall.com/lists/oss-security/2021/07/22/1) that is similar to the recently fixed CVE-2021-3573.
>
> The key point here is that the sco_conn_del() function can be called when syscalls like sco_sendmsg() is undergoing.
> I think the easiest fix is to hang the sco_conn_del() using lock_sock() like below.
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index d9a4e88dacbb..3da1ad441463 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -173,10 +173,10 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>
>         if (sk) {
>                 sock_hold(sk);
> -               bh_lock_sock(sk);
> +               lock_sock(sk);
>                 sco_sock_clear_timer(sk);
>                 sco_chan_del(sk, err);
> -               bh_unlock_sock(sk);
> +               release_sock(sk);
>                 sco_sock_kill(sk);
>                 sock_put(sk);
>         }
>
> This can make sure the kfree() will wait for the sock held by the sco_sendmsg() function. However, this patch can incur WARNING report like below. (I don't really know if this report is correct).
>
> [   75.147515] ======================================================
> [   75.149955] WARNING: possible circular locking dependency detected
> [   75.150546] 5.11.11+ #58 Not tainted
> [   75.150895] ------------------------------------------------------
> [   75.151485] poc.sco/127 is trying to acquire lock:
> [   75.151947] ffff888012212120 (sk_lock-AF_BLUETOOTH-BTPROTO_SCO){+.+.}-{0:0}, at: sco_conn_del+0xf6/0x0
> [   75.152863]
> [   75.152863] but task is already holding lock:
> [   75.153420] ffffffff85b43948 (hci_cb_list_lock){+.+.}-{3:3}, at: hci_conn_hash_flush+0xb3/0x1f0
> [   75.154256]
> [   75.154256] which lock already depends on the new lock.

Im not really sure what to make it out of this, they are not the same
lock so how does it establish the relationship of hci_cb_list_lock and
sock_lock? Anyway it seems pretty obvious that sock_lock must be used
to prevent concurrent operation like these to happen, but if we can't
use sock_lock then perhaps something needs to change in the way we
acquire hci_cb_list_lock.

> P.S. find the POC code in openwall report
>
> With the lesson I learnt in last bad patch e305509e678b ("Bluetooth: use correct lock to prevent UAF of hdev object"). I don't really expect this as the final correct patch.
>
> I then try to use the technique in e04480920d1e ("Bluetooth: defer cleanup of resources in hci_unregister_dev()"). I mean, I want to defer the kfree of sco_conn object. However, the sco connection/disconnection mechanism is somewhat weird and I didn't really understand it by now.
>
> Let's see this __sco_sock_close() function, which will be called from sco_sock_release().
>
> static void __sco_sock_close(struct sock *sk)
> {
>         BT_DBG("sk %p state %d socket %p", sk, sk->sk_state, sk->sk_socket);
>
>         switch (sk->sk_state) {
>         case BT_LISTEN:
>                 sco_sock_cleanup_listen(sk);
>                 break;
>
>         case BT_CONNECTED:
>         case BT_CONFIG:
>                 if (sco_pi(sk)->conn->hcon) {
>                         sk->sk_state = BT_DISCONN;
>                         sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
>                         sco_conn_lock(sco_pi(sk)->conn);
>                         hci_conn_drop(sco_pi(sk)->conn->hcon);
>                         sco_pi(sk)->conn->hcon = NULL;
>                         sco_conn_unlock(sco_pi(sk)->conn);
>                 } else
>                         sco_chan_del(sk, ECONNRESET);
>                 break;
>
>         case BT_CONNECT2:
>         case BT_CONNECT:
>         case BT_DISCONN:
>                 sco_chan_del(sk, ECONNRESET);
>                 break;
>
>         default:
>                 sock_set_flag(sk, SOCK_ZAPPED);
>                 break;
>         }
> }
>
> As you can see, though one socket is in BT_CONNECTED state, this function will just drop the kref of sco_pi(sk)->conn->hcon but do nothing with sco_pi(sk)->conn object. Then how this conn object is released? Where should I defer the deallocation function to?
>
> I think I need help and discussion to settle down the solution for this. T_T
>
> Best Wishes
> Lin Ma



-- 
Luiz Augusto von Dentz