| Message ID | CAJjojJsj9pzF4j2MVvsM-hCpvyR7OkZn232yt3MdOGnLxOiRRg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Another patch for CVE-2021-3573 without introducing lock bugs | expand |
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
--- 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); }