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

[Lin Horse <kylin.formalin@gmail.com>]

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