Another patch for CVE-2021-3573 without introducing lock bugs

Another patch for CVE-2021-3573 without introducing lock bugs

[Lin Horse]

[-- Attachment #1: Type: text/plain, Size: 3969 bytes --]

Hello there

I need to say sorry about the erroneous patch I provided recently to
fix the CVE-2021-3573
(https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=e305509e678b3a4af2b3cfd410f409f7cdaabb52),
which will throw a locking bug when the CONFIG_DEBUG_ATOMIC_SLEEP
option is enabled.

[    8.234583] BUG: sleeping function called from invalid context at
net/core/sock.c:3048
[    8.235336] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid:
125, name: exp
[    8.236038] CPU: 0 PID: 125 Comm: exp Not tainted 5.11.11+ #13
[    8.236542] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.10.2-1ubuntu1 04/01/2014
[    8.237330] Call Trace:
[    8.237605]  dump_stack+0x1b9/0x22e
[    8.237946]  ? log_buf_vmcoreinfo_setup+0x45d/0x45d
[    8.238453]  ? tty_ldisc_hangup+0x4d7/0x6d0
[    8.238912]  ? show_regs_print_info+0x12/0x12
[    8.239383]  ? task_work_run+0x16c/0x210
[    8.239807]  ? syscall_exit_to_user_mode+0x20/0x40
[    8.240324]  ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    8.240897]  ? _raw_spin_lock+0xa1/0x170
[    8.241326]  ___might_sleep+0x32d/0x420
[    8.241749]  ? stack_trace_snprint+0xe0/0xe0
[    8.242204]  ? __might_sleep+0x100/0x100
[    8.242636]  ? deactivate_slab+0x1ca/0x560
[    8.243080]  lock_sock_nested+0x96/0x360
[    8.243523]  ? hci_sock_dev_event+0xfe/0x5b0
[    8.244007]  ? sock_def_destruct+0x10/0x10
[    8.244372]  ? kasan_set_free_info+0x1f/0x40
[    8.244738]  ? kmem_cache_free+0xca/0x220
[    8.245093]  hci_sock_dev_event+0x2fa/0x5b0
[    8.245454]  hci_unregister_dev+0x3fa/0x1700
[    8.245820]  ? rcu_sync_exit+0xe0/0x1e0

Recently I received several emails informing this. Thank Anand for his
detailed analysis, you can find the relevant discussion either in the
linux-kernel@vger.kernel.org mail list or the stable@vger.kernel.org
mail list. (https://www.spinics.net/lists/stable/msg476038.html for
example).

--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -755,7 +755,7 @@ void hci_sock_dev_event(struct hci_dev *
    /* Detach sockets from device */
    read_lock(&hci_sk_list.lock);
    sk_for_each(sk, &hci_sk_list.head) {
- bh_lock_sock_nested(sk);
+ lock_sock(sk);
    if (hci_pi(sk)->hdev == hdev) {
    hci_pi(sk)->hdev = NULL;
    sk->sk_err = EPIPE;
@@ -764,7 +764,7 @@ void hci_sock_dev_event(struct hci_dev *

    hci_dev_put(hdev);
    }
- bh_unlock_sock(sk);
+ release_sock(sk);
    }
    read_unlock(&hci_sk_list.lock);
    }

The core insight for the bug in the previous commit is that: the
lock_sock() I replaced can sleep while the
read_lock(&hci_sk_list.lock) two lines before is not going to allow
the sleep.

Okay, the reason I changed the bh_lock_sock_nested() to lock_sock() is
that I want this hci_sock_dev_event() function hangs out when other
functions, like hci_sock_bound_ioctl() holds the lock. Otherwise bad
UAF can happen. Check the OSS mail list for details:
https://www.openwall.com/lists/oss-security/2021/06/08/2

However, from the lock principle in the Linux kernel, this lock
replacement is not appropriate. I take a lot of time to try with other
lock combinations for this case but failed. For example, I tried to
replace the rwlock_t in the hci_sk_list with a sleep-able mutex lock.
But the CONFIG_DEBUG_ATOMIC_SLEEP tells me that this mutex lock is not
expected in the system call context.

In short, I have no idea if there is any lock replacing solution for
this bug. I need help and suggestions because the lock mechanism is
just so difficult.

Think about the UAF root cause, I find out another possible patch for
this, which is also provided as the attachment. That is, maybe we can
use a ref-count method as well as additional checks to prevent the UAF
of hdev object.

I mainly concentrated on the hci_sock_bound_ioctl() and
hci_sock_sendmsg() functions, which are the main targets for me to
write the exploit. For now I think these checks are enough and welcome
more advice.

Thanks
Lin Ma

[-- Attachment #2: demo.patch --]
[-- Type: application/octet-stream, Size: 5600 bytes --]

From 4482177818ca24cb8c6aa35007573be3b69e0316 Mon Sep 17 00:00:00 2001
From: Lin Ma <linma@zju.edu.cn>
Date: Wed, 23 Jun 2021 09:09:02 +0800
Subject: [PATCH] Bluetooth: fix the UAF without introducing the lock bug

This patch revert the previous one (e305509e678b3a4af2b3cfd410f409f7cdaabb52)
which will trigger a lock bug when enabling CONFIG_DEBUG_ATOMIC_SLEEP.

At the same time, to make sure that the hdev object will not be released when
hci_sock_bound_ioctl() and hci_sock_sendmsg() functions still hold its reference.
This patch utilizes refcount methods in these two functions.

Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 net/bluetooth/hci_conn.c |  6 ++++
 net/bluetooth/hci_sock.c | 76 ++++++++++++++++++++++++++++------------
 2 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 88ec08978ff4..2787da8fe14a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1711,6 +1711,9 @@ int hci_get_conn_info(struct hci_dev *hdev, void __user *arg)
 	if (copy_from_user(&req, arg, sizeof(req)))
 		return -EFAULT;
 
+	if (!test_bit(HCI_UP, &hdev->flags))
+		return -ENETDOWN;
+
 	hci_dev_lock(hdev);
 	conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
 	if (conn) {
@@ -1737,6 +1740,9 @@ int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
 	if (copy_from_user(&req, arg, sizeof(req)))
 		return -EFAULT;
 
+	if (!test_bit(HCI_UP, &hdev->flags))
+		return -ENETDOWN;
+
 	hci_dev_lock(hdev);
 	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
 	if (conn)
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index eed0dd066e12..a7a9e5c55ffd 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -762,7 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 		/* Detach sockets from device */
 		read_lock(&hci_sk_list.lock);
 		sk_for_each(sk, &hci_sk_list.head) {
-			lock_sock(sk);
+			bh_lock_sock_nested(sk);
 			if (hci_pi(sk)->hdev == hdev) {
 				hci_pi(sk)->hdev = NULL;
 				sk->sk_err = EPIPE;
@@ -771,7 +771,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
 
 				hci_dev_put(hdev);
 			}
-			release_sock(sk);
+			bh_unlock_sock(sk);
 		}
 		read_unlock(&hci_sk_list.lock);
 	}
@@ -900,6 +900,9 @@ static int hci_sock_blacklist_add(struct hci_dev *hdev, void __user *arg)
 	if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
 		return -EFAULT;
 
+	if (!test_bit(HCI_UP, &hdev->flags))
+		return -ENETDOWN;
+
 	hci_dev_lock(hdev);
 
 	err = hci_bdaddr_list_add(&hdev->blacklist, &bdaddr, BDADDR_BREDR);
@@ -917,6 +920,9 @@ static int hci_sock_blacklist_del(struct hci_dev *hdev, void __user *arg)
 	if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
 		return -EFAULT;
 
+	if (!test_bit(HCI_UP, &hdev->flags))
+		return -ENETDOWN;
+
 	hci_dev_lock(hdev);
 
 	err = hci_bdaddr_list_del(&hdev->blacklist, &bdaddr, BDADDR_BREDR);
@@ -931,43 +937,64 @@ static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
 				unsigned long arg)
 {
 	struct hci_dev *hdev = hci_pi(sk)->hdev;
+	int err;
 
 	if (!hdev)
 		return -EBADFD;
 
-	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
-		return -EBUSY;
+	hdev = hci_dev_hold(hdev);
 
-	if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
-		return -EOPNOTSUPP;
+	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
+		err = -EBUSY;
+		goto done;
+	}
 
-	if (hdev->dev_type != HCI_PRIMARY)
-		return -EOPNOTSUPP;
+	if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
+		err = -EOPNOTSUPP;
+		goto done;
+	}
+
+	if (hdev->dev_type != HCI_PRIMARY) {
+		err = -EOPNOTSUPP;
+		goto done;
+	}
 
 	switch (cmd) {
 	case HCISETRAW:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return -EOPNOTSUPP;
+			err = -EPERM;
+		else
+			err = -EOPNOTSUPP;
+		break;
 
 	case HCIGETCONNINFO:
-		return hci_get_conn_info(hdev, (void __user *)arg);
+		err = hci_get_conn_info(hdev, (void __user *)arg);
+		break;
 
 	case HCIGETAUTHINFO:
-		return hci_get_auth_info(hdev, (void __user *)arg);
+		err = hci_get_auth_info(hdev, (void __user *)arg);
+		break;
 
 	case HCIBLOCKADDR:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return hci_sock_blacklist_add(hdev, (void __user *)arg);
+			err = -EPERM;
+		else
+			err = hci_sock_blacklist_add(hdev, (void __user *)arg);
+		break;
 
 	case HCIUNBLOCKADDR:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return hci_sock_blacklist_del(hdev, (void __user *)arg);
+			err = -EPERM;
+		else
+			err = hci_sock_blacklist_del(hdev, (void __user *)arg);
+		break;
+	default:
+		err = -ENOIOCTLCMD;
 	}
 
-	return -ENOIOCTLCMD;
+done:
+	hci_dev_put(hdev);
+	return err;
 }
 
 static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
@@ -1745,14 +1772,11 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	hdev = hci_pi(sk)->hdev;
 	if (!hdev) {
-		err = -EBADFD;
-		goto done;
+		release_sock(sk);
+		return -EBADFD;
 	}
 
-	if (!test_bit(HCI_UP, &hdev->flags)) {
-		err = -ENETDOWN;
-		goto done;
-	}
+	hdev = hci_dev_hold(hdev);
 
 	skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
@@ -1763,6 +1787,11 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 		goto drop;
 	}
 
+        if (!test_bit(HCI_UP, &hdev->flags)) {
+                err = -ENETDOWN;
+                goto drop;
+        }
+
 	hci_skb_pkt_type(skb) = skb->data[0];
 	skb_pull(skb, 1);
 
@@ -1832,6 +1861,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	err = len;
 
 done:
+	hci_dev_put(hdev);
 	release_sock(sk);
 	return err;
 
-- 
2.32.0

Re: Another patch for CVE-2021-3573 without introducing lock bugs

[Lin Horse]

Hi there

>
> It is good to put your patch in the mail message instead of attachment.
>

Hi Danton, thanks for the advice. I will present the patch and give
the description in the message.

>
> Yes the uaf can be fixed by taking another grab to hci dev, see below
> diff.
> >

--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -762,7 +762,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
         /* Detach sockets from device */
         read_lock(&hci_sk_list.lock);
         sk_for_each(sk, &hci_sk_list.head) {
-            lock_sock(sk);
+            bh_lock_sock_nested(sk);
             if (hci_pi(sk)->hdev == hdev) {
                 hci_pi(sk)->hdev = NULL;
                 sk->sk_err = EPIPE;
@@ -771,7 +771,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)

                 hci_dev_put(hdev);
             }
-            release_sock(sk);
+            bh_unlock_sock(sk);
         }
         read_unlock(&hci_sk_list.lock);
     }

The first thing is to revert the lock replacement. By the way, I
wonder if we need to change the read_lock() here to write_lock(), as
the code between the lock indeed changes something related to the
hci_sk_list.

For the patch code in hci_sock_bound_ioctl() function, I prefer to
leave the hci_sock_ioctl() function alone. Danton changes the
lock_sock() from hci_sock_ioctl() to hci_sock_bound_ioctl() for the
serialise stuff, which I don't really get the point.

> +       /* serialise with read_lock in hci_sock_dev_event */
> +       write_lock(&hci_sk_list.lock);
> +       bh_lock_sock_nested(sk);
> +       hdev = hci_pi(sk)->hdev;
> +       if (hdev)
> +               hci_dev_hold(hdev);
> +       bh_unlock_sock(sk);
> +       write_unlock(&hci_sk_list.lock);

Even the read of hci_pi(sk)->hdev is protected like above, the
attacker can still play userfaultfd tricks to abuse this hdev object.
Check the attacker scripts in the OSS list.

Hence, I think the important thing is to add proper flag check after
the dangerous copy_from_user() functions like below.

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 88ec08978ff4..2787da8fe14a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1711,6 +1711,9 @@ int hci_get_conn_info(struct hci_dev *hdev, void
__user *arg)
        if (copy_from_user(&req, arg, sizeof(req)))
                return -EFAULT;

+       if (!test_bit(HCI_UP, &hdev->flags))
+               return -ENETDOWN;
+
        hci_dev_lock(hdev);
        conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
        if (conn) {
@@ -1737,6 +1740,9 @@ int hci_get_auth_info(struct hci_dev *hdev, void
__user *arg)
        if (copy_from_user(&req, arg, sizeof(req)))
                return -EFAULT;

+       if (!test_bit(HCI_UP, &hdev->flags))
+               return -ENETDOWN;
+
        hci_dev_lock(hdev);
        conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
        if (conn)

@@ -900,6 +900,9 @@ static int hci_sock_blacklist_add(struct hci_dev
*hdev, void __user *arg)
        if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
                return -EFAULT;

+       if (!test_bit(HCI_UP, &hdev->flags))
+               return -ENETDOWN;
+
        hci_dev_lock(hdev);

        err = hci_bdaddr_list_add(&hdev->blacklist, &bdaddr, BDADDR_BREDR);
@@ -917,6 +920,9 @@ static int hci_sock_blacklist_del(struct hci_dev
*hdev, void __user *arg)
        if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
                return -EFAULT;

+       if (!test_bit(HCI_UP, &hdev->flags))
+               return -ENETDOWN;
+
        hci_dev_lock(hdev);

        err = hci_bdaddr_list_del(&hdev->blacklist, &bdaddr, BDADDR_BREDR);

And last but not least, we patch the hci_sock_bound_ioctl() and
hci_sock_sendmsg() function to take an extra hold of the hdev. That
part is almost like Danton's code and you can check the previous added
attachment.

There are some other readings of hci_pi(sk)->hdev. One is in
hci_sock_release() and another is in hci_sock_getname(). However, I
don't think these two can be easily abused because no copy_from_user()
is called. Do we need to add hold for these functions too?

Regards
Lin Ma