Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <15f5a46.b79d9.17ba6802ccd.Coremail.linma@zju.edu.cn>
In-Reply-To: <15f5a46.b79d9.17ba6802ccd.Coremail.linma@zju.edu.cn>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 2 Sep 2021 11:46:28 -0700
Message-ID: <CABBYNZKSJAHK-YPx=XAiq2FYLU-dOiNY9rYuOVG1a1V4pH2c-Q@mail.gmail.com>
Subject: Re: Help needed in patching CVE-2021-3640
To:     LinMa <linma@zju.edu.cn>
Cc:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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-secur=
ity/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() l=
ike 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_sen=
dmsg() function. However, this patch can incur WARNING report like below. (=
I don't really know if this report is correct).
>
> [   75.147515] =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> [   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_c=
onn_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 cleanu=
p 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_socke=
t);
>
>         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 =3D 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 =3D 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


--=20
Luiz Augusto von Dentz