| Message ID | 60f604f8-2a89-fd3f-996f-9d9e4a229427@i-love.sakura.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: avoid page fault from sco_send_frame() | expand |
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 > >
On 2021/09/03 12:48, Luiz Augusto von Dentz wrote: > There is a set already handing this sort of problem: > > https://patchwork.kernel.org/project/bluetooth/patch/20210901002621.414016-3-luiz.dentz@gmail.com/ OK, I didn't know that. (I'm not subscribed to bluethooth ML.) But can we please keep the fix minimal? Multiple distributors are waiting for the fix (which can be backported) for more than one month. https://security-tracker.debian.org/tracker/CVE-2021-3640 https://access.redhat.com/security/cve/cve-2021-3640 And it looks to me that your "[3/4] Bluetooth: SCO: Replace use of memcpy_from_msg with bt_skb_sendmsg" contains a new use-after-free or memory corruption bug... :-(
Commit 99c23da0eed4fd20 ("Bluetooth: sco: Fix lock_sock() blockage by memcpy_from_msg()") in linux-next.git should be sent to linux.git now as a mitigation for CVE-2021-3640.
But I think "[PATCH v3 3/4] Bluetooth: SCO: Replace use of memcpy_from_msg with bt_skb_sendmsg" still contains bug.
Hi, On Sat, Sep 04, 2021 at 11:02:58AM +0900, Tetsuo Handa wrote: > Commit 99c23da0eed4fd20 ("Bluetooth: sco: Fix lock_sock() blockage > by memcpy_from_msg()") in linux-next.git should be sent to linux.git > now as a mitigation for CVE-2021-3640. > > But I think "[PATCH v3 3/4] Bluetooth: SCO: Replace use of > memcpy_from_msg with bt_skb_sendmsg" still contains bug. Did his one felt through the cracks? I'm confused about the statement in https://bugzilla.suse.com/show_bug.cgi?id=1188172#c8 so Cc'ing Takashi Iwai as well. Regards, Salvatore
On Mon, 11 Oct 2021 09:00:00 +0200, Salvatore Bonaccorso wrote: > > Hi, > > On Sat, Sep 04, 2021 at 11:02:58AM +0900, Tetsuo Handa wrote: > > Commit 99c23da0eed4fd20 ("Bluetooth: sco: Fix lock_sock() blockage > > by memcpy_from_msg()") in linux-next.git should be sent to linux.git > > now as a mitigation for CVE-2021-3640. > > > > But I think "[PATCH v3 3/4] Bluetooth: SCO: Replace use of > > memcpy_from_msg with bt_skb_sendmsg" still contains bug. > > Did his one felt through the cracks? I'm confused about the statement > in https://bugzilla.suse.com/show_bug.cgi?id=1188172#c8 so Cc'ing > Takashi Iwai as well. The quite similar fix has been already in the subsystem tree, commit 99c23da0eed4 ("Bluetooth: sco: Fix lock_sock() blockage by memcpy_from_msg()"). The particular CVE should be covered by that and prerequisite patches. Takashi
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; + } + 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; }
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(-)