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>
 <c998d16d-f45a-8be4-2898-9e94509cb2ea@i-love.sakura.ne.jp> <60f604f8-2a89-fd3f-996f-9d9e4a229427@i-love.sakura.ne.jp>
In-Reply-To: <60f604f8-2a89-fd3f-996f-9d9e4a229427@i-love.sakura.ne.jp>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 2 Sep 2021 20:48:35 -0700
Message-ID: <CABBYNZK-JvPcB_T39_NUE-O6ztE6crNEfmFxszEtAu3OkrKF2A@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: avoid page fault from sco_send_frame()
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc:     LinMa <linma@zju.edu.cn>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Linus Torvalds <torvalds@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Tetsuo,

On Thu, Sep 2, 2021 at 7:44 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> Since userfaultfd mechanism allows sleeping with kernel lock held,
> avoiding page fault with kernel lock held where possible will make
> the module more robust. This patch just brings memcpy_from_msg() calls
> to out of sock lock.
>
> This patch is an instant mitigation for CVE-2021-3640. To fully close
> the race window for this use-after-free problem, we need more changes.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  net/bluetooth/sco.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index d9a4e88dacbb..e4b079b31ce9 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -273,7 +273,7 @@ static int sco_connect(struct sock *sk)
>         return err;
>  }
>
> -static int sco_send_frame(struct sock *sk, struct msghdr *msg, int len)
> +static int sco_send_frame(struct sock *sk, const void *buf, int len, int flags)
>  {
>         struct sco_conn *conn = sco_pi(sk)->conn;
>         struct sk_buff *skb;
> @@ -285,14 +285,11 @@ static int sco_send_frame(struct sock *sk, struct msghdr *msg, int len)
>
>         BT_DBG("sk %p len %d", sk, len);
>
> -       skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
> +       skb = bt_skb_send_alloc(sk, len, flags & MSG_DONTWAIT, &err);
>         if (!skb)
>                 return err;
>
> -       if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
> -               kfree_skb(skb);
> -               return -EFAULT;
> -       }
> +       memcpy(skb_put(skb, len), buf, len);
>
>         hci_send_sco(conn->hcon, skb);
>
> @@ -714,6 +711,7 @@ static int sco_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>                             size_t len)
>  {
>         struct sock *sk = sock->sk;
> +       void *buf;
>         int err;
>
>         BT_DBG("sock %p, sk %p", sock, sk);
> @@ -725,14 +723,23 @@ static int sco_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>         if (msg->msg_flags & MSG_OOB)
>                 return -EOPNOTSUPP;
>
> +       buf = kmalloc(len, GFP_KERNEL | __GFP_NOWARN);
> +       if (!buf)
> +               return -ENOMEM;
> +       if (memcpy_from_msg(buf, msg, len)) {
> +               kfree(buf);
> +               return -EFAULT;
> +       }

There is a set already handing this sort of problem:

https://patchwork.kernel.org/project/bluetooth/patch/20210901002621.414016-3-luiz.dentz@gmail.com/

>         lock_sock(sk);
>
>         if (sk->sk_state == BT_CONNECTED)
> -               err = sco_send_frame(sk, msg, len);
> +               err = sco_send_frame(sk, buf, len, msg->msg_flags);
>         else
>                 err = -ENOTCONN;
>
>         release_sock(sk);
> +       kfree(buf);
>         return err;
>  }
>
> --
> 2.30.2
>
>


-- 
Luiz Augusto von Dentz