[RFC] Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG

Started by Luiz Augusto von Dentz on Jul 17, 2021 · 7 messages from 3 authors · Last reply Jul 18, 2021

Subsystems (2)

Maintainers (3)

open the first message on its own page

Luiz Augusto von Dentz

Message details and related content
Open message details and related content
From: Luiz Augusto von Dentz <redacted>

This removes the reference of hci_dev to hci_pinfo since the reference
cannot prevent hdev to be freed hci_pinfo only keeps the index so in
case the device is unregistered and freed hci_dev_get will return NULL
thus prevent UAF.

On top of it commands cases where copy_from_user needs to be used are
now done without helding a reference to the hci_dev.

Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
Reported-by: syzbot <redacted>
Tested-by: syzbot <redacted>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
Signed-off-by: Luiz Augusto von Dentz <redacted>
---
 include/net/bluetooth/hci_core.h |   6 +-
 net/bluetooth/hci_conn.c         |  24 ++---
 net/bluetooth/hci_sock.c         | 162 +++++++++++++++++++++++--------
 3 files changed, 137 insertions(+), 55 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8b63ef2ec31a..ddac4be91525 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1266,8 +1266,10 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg);
 int hci_get_dev_list(void __user *arg);
 int hci_get_dev_info(void __user *arg);
 int hci_get_conn_list(void __user *arg);
-int hci_get_conn_info(struct hci_dev *hdev, void __user *arg);
-int hci_get_auth_info(struct hci_dev *hdev, void __user *arg);
+int hci_get_conn_info(struct hci_dev *hdev, void __user *arg,
+		      struct hci_conn_info_req *req);
+int hci_get_auth_info(struct hci_dev *hdev, void __user *arg,
+		      struct hci_auth_info_req *req);
 int hci_inquiry(void __user *arg);
 
 struct bdaddr_list *hci_bdaddr_list_lookup(struct list_head *list,
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9fbab563362..4345bcc05778 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1701,18 +1701,15 @@ int hci_get_conn_list(void __user *arg)
 	return err ? -EFAULT : 0;
 }
 
-int hci_get_conn_info(struct hci_dev *hdev, void __user *arg)
+int hci_get_conn_info(struct hci_dev *hdev, void __user *arg,
+		      struct hci_conn_info_req *req)
 {
-	struct hci_conn_info_req req;
 	struct hci_conn_info ci;
 	struct hci_conn *conn;
-	char __user *ptr = arg + sizeof(req);
-
-	if (copy_from_user(&req, arg, sizeof(req)))
-		return -EFAULT;
+	char __user *ptr = (char *)arg + sizeof(*req);
 
 	hci_dev_lock(hdev);
-	conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
+	conn = hci_conn_hash_lookup_ba(hdev, req->type, &req->bdaddr);
 	if (conn) {
 		bacpy(&ci.bdaddr, &conn->dst);
 		ci.handle = conn->handle;
@@ -1729,24 +1726,21 @@ int hci_get_conn_info(struct hci_dev *hdev, void __user *arg)
 	return copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
 }
 
-int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
+int hci_get_auth_info(struct hci_dev *hdev, void __user *arg,
+		      struct hci_auth_info_req *req)
 {
-	struct hci_auth_info_req req;
 	struct hci_conn *conn;
 
-	if (copy_from_user(&req, arg, sizeof(req)))
-		return -EFAULT;
-
 	hci_dev_lock(hdev);
-	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
+	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req->bdaddr);
 	if (conn)
-		req.type = conn->auth_type;
+		req->type = conn->auth_type;
 	hci_dev_unlock(hdev);
 
 	if (!conn)
 		return -ENOENT;
 
-	return copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
+	return copy_to_user(arg, req, sizeof(*req)) ? -EFAULT : 0;
 }
 
 struct hci_chan *hci_chan_create(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..3f104a3aca7e 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -50,7 +50,7 @@ static atomic_t monitor_promisc = ATOMIC_INIT(0);
 
 struct hci_pinfo {
 	struct bt_sock    bt;
-	struct hci_dev    *hdev;
+	int               dev;
 	struct hci_filter filter;
 	__u8              cmsg_mask;
 	unsigned short    channel;
@@ -200,7 +200,8 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
 	sk_for_each(sk, &hci_sk_list.head) {
 		struct sk_buff *nskb;
 
-		if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev)
+		if (sk->sk_state != BT_BOUND ||
+		    (hdev && hci_pi(sk)->dev != hdev->id))
 			continue;
 
 		/* Don't send frame to the socket it came from */
@@ -536,8 +537,8 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk)
 
 	hdr = skb_push(skb, HCI_MON_HDR_SIZE);
 	hdr->opcode = cpu_to_le16(HCI_MON_CTRL_OPEN);
-	if (hci_pi(sk)->hdev)
-		hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
+	if (hci_pi(sk)->dev >= 0)
+		hdr->index = cpu_to_le16(hci_pi(sk)->dev);
 	else
 		hdr->index = cpu_to_le16(HCI_DEV_NONE);
 	hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE);
@@ -574,8 +575,8 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk)
 
 	hdr = skb_push(skb, HCI_MON_HDR_SIZE);
 	hdr->opcode = cpu_to_le16(HCI_MON_CTRL_CLOSE);
-	if (hci_pi(sk)->hdev)
-		hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
+	if (hci_pi(sk)->dev >= 0)
+		hdr->index = cpu_to_le16(hci_pi(sk)->dev);
 	else
 		hdr->index = cpu_to_le16(HCI_DEV_NONE);
 	hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE);
@@ -762,16 +763,13 @@ 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);
-			if (hci_pi(sk)->hdev == hdev) {
-				hci_pi(sk)->hdev = NULL;
+			bh_lock_sock_nested(sk);
+			if (hci_pi(sk)->dev == hdev->id) {
 				sk->sk_err = EPIPE;
 				sk->sk_state = BT_OPEN;
 				sk->sk_state_change(sk);
-
-				hci_dev_put(hdev);
 			}
-			release_sock(sk);
+			bh_unlock_sock(sk);
 		}
 		read_unlock(&hci_sk_list.lock);
 	}
@@ -861,7 +859,7 @@ static int hci_sock_release(struct socket *sock)
 
 	bt_sock_unlink(&hci_sk_list, sk);
 
-	hdev = hci_pi(sk)->hdev;
+	hdev = hci_dev_get(hci_pi(sk)->dev);
 	if (hdev) {
 		if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
 			/* When releasing a user channel exclusive access,
@@ -892,82 +890,161 @@ static int hci_sock_release(struct socket *sock)
 	return 0;
 }
 
-static int hci_sock_reject_list_add(struct hci_dev *hdev, void __user *arg)
+static int hci_sock_reject_list_add(struct sock *sk, void __user *arg)
 {
+	struct hci_dev *hdev;
 	bdaddr_t bdaddr;
 	int err;
 
 	if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
 		return -EFAULT;
 
+	hdev = hci_dev_get(hci_pi(sk)->dev);
+	if (!hdev)
+		return -EBADF;
+
 	hci_dev_lock(hdev);
 
 	err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
 
 	hci_dev_unlock(hdev);
+	hci_dev_put(hdev);
 
 	return err;
 }
 
-static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
+static int hci_sock_reject_list_del(struct sock *sk, void __user *arg)
 {
+	struct hci_dev *hdev;
 	bdaddr_t bdaddr;
 	int err;
 
 	if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
 		return -EFAULT;
 
+	hdev = hci_dev_get(hci_pi(sk)->dev);
+	if (!hdev)
+		return -EBADF;
+
 	hci_dev_lock(hdev);
 
 	err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
 
 	hci_dev_unlock(hdev);
+	hci_dev_put(hdev);
 
 	return err;
 }
 
+static int hci_sock_get_conn_info(struct sock *sk, void __user *arg)
+{
+	struct hci_dev *hdev;
+	struct hci_conn_info_req req;
+	int ret;
+
+	if (copy_from_user(&req, arg, sizeof(req)))
+		return -EFAULT;
+
+	hdev = hci_dev_get(hci_pi(sk)->dev);
+	if (!hdev)
+		return -EBADF;
+
+	ret = hci_get_conn_info(hdev, arg, &req);
+
+	hci_dev_put(hdev);
+
+	return ret;
+}
+
+static int hci_sock_get_auth_info(struct sock *sk, void __user *arg)
+{
+	struct hci_dev *hdev;
+	struct hci_auth_info_req req;
+	int ret;
+
+	if (copy_from_user(&req, arg, sizeof(req)))
+		return -EFAULT;
+
+	hdev = hci_dev_get(hci_pi(sk)->dev);
+	if (!hdev)
+		return -EBADF;
+
+	ret = hci_get_auth_info(hdev, arg, &req);
+
+	hci_dev_put(hdev);
+
+	return ret;
+}
+
 /* Ioctls that require bound socket */
 static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
 				unsigned long arg)
 {
-	struct hci_dev *hdev = hci_pi(sk)->hdev;
+	struct hci_dev *hdev = hci_dev_get(hci_pi(sk)->dev);
+	int ret;
 
 	if (!hdev)
 		return -EBADFD;
 
-	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
-		return -EBUSY;
+	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
+		ret = -EBUSY;
+		goto done;
+	}
 
-	if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
-		return -EOPNOTSUPP;
+	if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
+		ret = -EOPNOTSUPP;
+		goto done;
+	}
 
-	if (hdev->dev_type != HCI_PRIMARY)
-		return -EOPNOTSUPP;
+	if (hdev->dev_type != HCI_PRIMARY) {
+		ret = -EOPNOTSUPP;
+		goto done;
+	}
+
+	/* Commands may use copy_from_user which is unsafe while holding hdev as
+	 * it could be unregistered in the meantime.
+	 */
+	hci_dev_put(hdev);
+	hdev = NULL;
 
 	switch (cmd) {
 	case HCISETRAW:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return -EOPNOTSUPP;
+			ret = -EPERM;
+		else
+			ret = -EOPNOTSUPP;
+		break;
 
 	case HCIGETCONNINFO:
-		return hci_get_conn_info(hdev, (void __user *)arg);
+		ret = hci_sock_get_conn_info(sk, (void __user *)arg);
+		break;
 
 	case HCIGETAUTHINFO:
-		return hci_get_auth_info(hdev, (void __user *)arg);
+		ret = hci_sock_get_auth_info(sk, (void __user *)arg);
+		break;
 
 	case HCIBLOCKADDR:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return hci_sock_reject_list_add(hdev, (void __user *)arg);
+			ret = -EPERM;
+		else
+			ret = hci_sock_reject_list_add(sk, (void __user *)arg);
+		break;
 
 	case HCIUNBLOCKADDR:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return hci_sock_reject_list_del(hdev, (void __user *)arg);
+			ret = -EPERM;
+		else
+			ret = hci_sock_reject_list_del(sk, (void __user *)arg);
+		break;
+	default:
+		ret = -ENOIOCTLCMD;
 	}
 
-	return -ENOIOCTLCMD;
+done:
+	if (hdev)
+		hci_dev_put(hdev);
+
+	return ret;
 }
 
 static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
@@ -1110,7 +1187,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 
 	switch (haddr.hci_channel) {
 	case HCI_CHANNEL_RAW:
-		if (hci_pi(sk)->hdev) {
+		if (hci_pi(sk)->dev >= 0) {
 			err = -EALREADY;
 			goto done;
 		}
@@ -1145,7 +1222,10 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 		if (capable(CAP_NET_ADMIN))
 			hci_sock_set_flag(sk, HCI_SOCK_TRUSTED);
 
-		hci_pi(sk)->hdev = hdev;
+		if (hdev) {
+			hci_pi(sk)->dev = hdev->id;
+			hci_dev_put(hdev);
+		}
 
 		/* Send event to monitor */
 		skb = create_monitor_ctrl_open(sk);
@@ -1157,7 +1237,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 		break;
 
 	case HCI_CHANNEL_USER:
-		if (hci_pi(sk)->hdev) {
+		if (hci_pi(sk)->dev >= 0) {
 			err = -EALREADY;
 			goto done;
 		}
@@ -1236,7 +1316,8 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 		 */
 		hci_sock_set_flag(sk, HCI_SOCK_TRUSTED);
 
-		hci_pi(sk)->hdev = hdev;
+		hci_pi(sk)->dev = hdev->id;
+		hci_dev_put(hdev);
 
 		/* Send event to monitor */
 		skb = create_monitor_ctrl_open(sk);
@@ -1379,7 +1460,7 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr,
 
 	lock_sock(sk);
 
-	hdev = hci_pi(sk)->hdev;
+	hdev = hci_dev_get(hci_pi(sk)->dev);
 	if (!hdev) {
 		err = -EBADFD;
 		goto done;
@@ -1389,6 +1470,7 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr,
 	haddr->hci_dev    = hdev->id;
 	haddr->hci_channel= hci_pi(sk)->channel;
 	err = sizeof(*haddr);
+	hci_dev_put(hdev);
 
 done:
 	release_sock(sk);
@@ -1703,7 +1785,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 {
 	struct sock *sk = sock->sk;
 	struct hci_mgmt_chan *chan;
-	struct hci_dev *hdev;
+	struct hci_dev *hdev = NULL;
 	struct sk_buff *skb;
 	int err;
 
@@ -1743,7 +1825,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 		goto done;
 	}
 
-	hdev = hci_pi(sk)->hdev;
+	hdev = hci_dev_get(hci_pi(sk)->dev);
 	if (!hdev) {
 		err = -EBADFD;
 		goto done;
@@ -1832,6 +1914,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
 	err = len;
 
 done:
+	if (hdev)
+		hci_dev_put(hdev);
+
 	release_sock(sk);
 	return err;
 
@@ -2049,6 +2134,7 @@ static int hci_sock_create(struct net *net, struct socket *sock, int protocol,
 	sock->state = SS_UNCONNECTED;
 	sk->sk_state = BT_OPEN;
 
+	hci_pi(sk)->dev = -1;
 	bt_sock_link(&hci_sk_list, sk);
 	return 0;
 }
-- 
2.31.1

unknown sender replied to Luiz Augusto von Dentz

Message details and related content
Open message details and related content
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=516913

---Test result---

Test Summary:
CheckPatch                    FAIL      1.11 seconds
GitLint                       FAIL      0.12 seconds
BuildKernel                   PASS      680.73 seconds
TestRunner: Setup             PASS      445.11 seconds
TestRunner: l2cap-tester      PASS      3.15 seconds
TestRunner: bnep-tester       PASS      2.21 seconds
TestRunner: mgmt-tester       PASS      35.57 seconds
TestRunner: rfcomm-tester     PASS      2.48 seconds
TestRunner: sco-tester        PASS      2.44 seconds
TestRunner: smp-tester        FAIL      2.50 seconds
TestRunner: userchan-tester   PASS      2.32 seconds

Details
##############################
Test: CheckPatch - FAIL - 1.11 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG
WARNING: Unknown commit id 'e305509e678b3a4a', maybe rebased or not pulled?
#18: 
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")

total: 0 errors, 1 warnings, 0 checks, 389 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: hci_sock: Fix calling lock_sock when handling" has style problems, please review.

NOTE: If any of the errors are false positives, please report

64 more lines

Tetsuo Handa replied to Luiz Augusto von Dentz

Message details and related content
Open message details and related content
Luiz Augusto von Dentz wrote:
This removes the reference of hci_dev to hci_pinfo since the reference
cannot prevent hdev to be freed hci_pinfo only keeps the index so in
case the device is unregistered and freed hci_dev_get will return NULL
thus prevent UAF.
I'm not convinced that this change is safe.

vhci_release() (which will call hci_unregister_dev(hdev)) is called when
refcount to /dev/vchi dropped to 0. That is, vhci_release() might be called
while e.g. hci_sock_bound_ioctl() is in progress.

Since hci_unregister_dev(hdev) calls list_del(&hdev->list) with hci_dev_list_lock
held for write, hci_dev_get(hci_pi(sk)->dev) which scans hci_dev_list with
hci_dev_list_lock held for read will start returning NULL. But I think that
this change introduces two race windows.

Since hci_unregister_dev(hdev) then calls hci_sock_dev_event(hdev, HCI_DEV_UNREG)
and finally calls ida_simple_remove(&hci_index_ida, id), subsequent
hci_register_dev(struct hci_dev *hdev) call can re-assign the id which
hci_pi(sk)->dev is tracking, by calling ida_simple_get() and finally calling
list_add(&hdev->list, &hci_dev_list) with hci_dev_list_lock held for write.

Therefore, I think that first race window is that

+	/* Commands may use copy_from_user which is unsafe while holding hdev as
+	 * it could be unregistered in the meantime.
+	 */
+	hci_dev_put(hdev);
+	hdev = NULL;

causes hci_sock_bound_ioctl() to check flags on an intended hdev and
e.g. hci_sock_reject_list_add() to operate on an unintended hdev.

Also, even if hci_sock_bound_ioctl() and hci_sock_reject_list_add() reached
the same hdev, I think that there still is second race window that

  hci_unregister_dev() {                       hci_sock_reject_list_add() {
                                                 hdev = hci_dev_get(hci_pi(sk)->dev);
    write_lock(&hci_dev_list_lock);
    list_del(&hdev->list);
    write_unlock(&hci_dev_list_lock);
    
    hci_sock_dev_event(hdev, HCI_DEV_UNREG);

13 more lines

Luiz Augusto von Dentz replied to Tetsuo Handa

Message details and related content
Open message details and related content
Hi Tetsuo,

On Sat, Jul 17, 2021 at 7:22 PM Tetsuo Handa
[off-list ref] wrote:
Luiz Augusto von Dentz wrote:
quoted
This removes the reference of hci_dev to hci_pinfo since the reference
cannot prevent hdev to be freed hci_pinfo only keeps the index so in
case the device is unregistered and freed hci_dev_get will return NULL
thus prevent UAF.
I'm not convinced that this change is safe.

vhci_release() (which will call hci_unregister_dev(hdev)) is called when
refcount to /dev/vchi dropped to 0. That is, vhci_release() might be called
while e.g. hci_sock_bound_ioctl() is in progress.

Since hci_unregister_dev(hdev) calls list_del(&hdev->list) with hci_dev_list_lock
held for write, hci_dev_get(hci_pi(sk)->dev) which scans hci_dev_list with
hci_dev_list_lock held for read will start returning NULL. But I think that
this change introduces two race windows.

Since hci_unregister_dev(hdev) then calls hci_sock_dev_event(hdev, HCI_DEV_UNREG)
and finally calls ida_simple_remove(&hci_index_ida, id), subsequent
hci_register_dev(struct hci_dev *hdev) call can re-assign the id which
hci_pi(sk)->dev is tracking, by calling ida_simple_get() and finally calling
list_add(&hdev->list, &hci_dev_list) with hci_dev_list_lock held for write.
We can perform a pointer comparison if that makes you happy. Anyway I
don't know if you guys have realized already but this is probably
impossible to reproduce with real hardware since the
registration/unregistration comes for other buses no memfault would
hold the thread that long for unplugging and replugging a device on
the bus, vhci is usually only used for emulation/testing/CI, not sure
who got the idea that finding vulnerabilities on our emulator would be
a great feat.

Therefore, I think that first race window is that

+       /* Commands may use copy_from_user which is unsafe while holding hdev as
+        * it could be unregistered in the meantime.
+        */
+       hci_dev_put(hdev);
+       hdev = NULL;

causes hci_sock_bound_ioctl() to check flags on an intended hdev and
e.g. hci_sock_reject_list_add() to operate on an unintended hdev.

Also, even if hci_sock_bound_ioctl() and hci_sock_reject_list_add() reached
the same hdev, I think that there still is second race window that

  hci_unregister_dev() {                       hci_sock_reject_list_add() {
                                                 hdev = hci_dev_get(hci_pi(sk)->dev);
    write_lock(&hci_dev_list_lock);
    list_del(&hdev->list);
    write_unlock(&hci_dev_list_lock);

    hci_sock_dev_event(hdev, HCI_DEV_UNREG);

    hci_dev_lock(hdev);
    hci_bdaddr_list_clear(&hdev->reject_list);
    hci_dev_unlock(hdev);
                                                 hci_dev_lock(hdev);
                                                 err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR); // <= Adding after clear all; at least memory leak.
                                                 hci_dev_unlock(hdev);
                                                 hci_dev_put(hdev);
  }

. That is, an attempt to replace pointer reference with index number is racy.

After all, hci_sock_dev_event(hdev, HCI_DEV_UNREG) is responsible for
waiting for already started e.g. hci_sock_reject_list_add().
Both blocks do require hci_dev_lock, so if the second block had
acquired the lock isn't it obvious that the first won't execute until
hci_dev_unlock is complete? Anyway after all these discussion Im even
more convinced that the real problem lies in hci_dev_get/hold, after
all references are usually used to prevent the objects to be freed but
in this case it doesn't and no locking will gonna fix that.

-- 
Luiz Augusto von Dentz

Tetsuo Handa replied to Luiz Augusto von Dentz

Message details and related content
Open message details and related content
On 2021/07/18 14:16, Luiz Augusto von Dentz wrote:
Anyway after all these discussion Im even
more convinced that the real problem lies in hci_dev_get/hold, after
all references are usually used to prevent the objects to be freed but
in this case it doesn't and no locking will gonna fix that.
If hci_dev_hold() calls atomic_long_add_unless(&file->f_count, 1, 0) under RCU,
vhci_release(file) would not be called until all sockets using that hdev drops
the reference, and hci_sock_dev_event(hdev, HCI_DEV_UNREG) no longer needs to
traverse sockets on hci_sk_list.head list. This requires adding "struct file *" to
"struct hci_dev". My patch keeps changes be confined to only hci_sock_dev_event().

Luiz Augusto von Dentz replied to Tetsuo Handa

Message details and related content
Open message details and related content
Hi Tetsuo,

On Sat, Jul 17, 2021 at 11:22 PM Tetsuo Handa
[off-list ref] wrote:
On 2021/07/18 14:16, Luiz Augusto von Dentz wrote:
quoted
Anyway after all these discussion Im even
more convinced that the real problem lies in hci_dev_get/hold, after
all references are usually used to prevent the objects to be freed but
in this case it doesn't and no locking will gonna fix that.
If hci_dev_hold() calls atomic_long_add_unless(&file->f_count, 1, 0) under RCU,
vhci_release(file) would not be called until all sockets using that hdev drops
the reference, and hci_sock_dev_event(hdev, HCI_DEV_UNREG) no longer needs to
traverse sockets on hci_sk_list.head list. This requires adding "struct file *" to
"struct hci_dev". My patch keeps changes be confined to only hci_sock_dev_event().
Being confined doesn't mean that it simple, your changes are doing a
loop locking, and I also didn't touch hci_dev_hold because it would
affect all drivers but if there is a way to do it by all means we
should do it, but notice that we do need a way to cleanup if the
device is unregistered so I don't think holding the file directly
would be a good idea since it prevents release but it would also
prevent cleanup, in other words if the process which open the vhci
terminates or close, all bluetooth sockets should receive a proper
error so we cannot really change this behavior. From the brief look at
it I think we should remove the function hci_dev_free and leave
hci_dev_unregister to cleanup everything, but I'm afraid there could
be extra references that are not being cleanup properly and finding
out where could take a lot more time, well even with your suggestion
that could be a problem since we also would need to inspect every time
we hold a reference in the same manner.

-- 
Luiz Augusto von Dentz

Tetsuo Handa replied to Tetsuo Handa

Message details and related content
Open message details and related content
Apart from my "[PATCH v3] Bluetooth: call lock_sock() outside of spinlock section",
I propose below change in order to make sure that hci_sock_bound_ioctl() will not
be blocked with sock lock held due to userfaultfd mechanism. This is a portion of
improvement I commented

  After lock_sock() became free from delay caused by pagefault handling

at https://lore.kernel.org/linux-bluetooth/9771b40f-b544-a2a7-04e1-eddb38a4aae7@i-love.sakura.ne.jp/ .
 include/net/bluetooth/hci_core.h |    3 
 net/bluetooth/hci_conn.c         |   50 -----------
 net/bluetooth/hci_sock.c         |  163 ++++++++++++++++++++++++---------------
 3 files changed, 106 insertions(+), 110 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a53e94459ecd..d9e55682b908 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1261,8 +1261,7 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg);
 int hci_get_dev_list(void __user *arg);
 int hci_get_dev_info(void __user *arg);
 int hci_get_conn_list(void __user *arg);
-int hci_get_conn_info(struct hci_dev *hdev, void __user *arg);
-int hci_get_auth_info(struct hci_dev *hdev, void __user *arg);
+u32 get_link_mode(struct hci_conn *conn);
 int hci_inquiry(void __user *arg);
 
 struct bdaddr_list *hci_bdaddr_list_lookup(struct list_head *list,
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 2b5059a56cda..41af11fadb74 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1626,7 +1626,7 @@ void hci_conn_check_pending(struct hci_dev *hdev)
 	hci_dev_unlock(hdev);
 }
 
-static u32 get_link_mode(struct hci_conn *conn)
+u32 get_link_mode(struct hci_conn *conn)
 {
 	u32 link_mode = 0;
 
@@ -1701,54 +1701,6 @@ int hci_get_conn_list(void __user *arg)
 	return err ? -EFAULT : 0;
 }
 
-int hci_get_conn_info(struct hci_dev *hdev, void __user *arg)
-{
-	struct hci_conn_info_req req;
-	struct hci_conn_info ci;
-	struct hci_conn *conn;
-	char __user *ptr = arg + sizeof(req);
-
-	if (copy_from_user(&req, arg, sizeof(req)))
-		return -EFAULT;
-
-	hci_dev_lock(hdev);
-	conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
-	if (conn) {
-		bacpy(&ci.bdaddr, &conn->dst);
-		ci.handle = conn->handle;
-		ci.type  = conn->type;
-		ci.out   = conn->out;
-		ci.state = conn->state;
-		ci.link_mode = get_link_mode(conn);
-	}
-	hci_dev_unlock(hdev);
-
-	if (!conn)
-		return -ENOENT;
-
-	return copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
-}
-
-int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
-{
-	struct hci_auth_info_req req;
-	struct hci_conn *conn;
-
-	if (copy_from_user(&req, arg, sizeof(req)))
-		return -EFAULT;
-
-	hci_dev_lock(hdev);
-	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
-	if (conn)
-		req.type = conn->auth_type;
-	hci_dev_unlock(hdev);
-
-	if (!conn)
-		return -ENOENT;
-
-	return copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
-}
-
 struct hci_chan *hci_chan_create(struct hci_conn *conn)
 {
 	struct hci_dev *hdev = conn->hdev;
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..2b166a269712 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -892,82 +892,134 @@ static int hci_sock_release(struct socket *sock)
 	return 0;
 }
 
-static int hci_sock_reject_list_add(struct hci_dev *hdev, void __user *arg)
+static int hci_sock_reject_list_add(struct hci_dev *hdev, bdaddr_t *bdaddr)
 {
-	bdaddr_t bdaddr;
-	int err;
-
-	if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
-		return -EFAULT;
-
-	hci_dev_lock(hdev);
-
-	err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
-
-	hci_dev_unlock(hdev);
-
-	return err;
+	return hci_bdaddr_list_add(&hdev->reject_list, bdaddr, BDADDR_BREDR);
 }
 
-static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
+static int hci_sock_reject_list_del(struct hci_dev *hdev, bdaddr_t *bdaddr)
 {
-	bdaddr_t bdaddr;
-	int err;
-
-	if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
-		return -EFAULT;
-
-	hci_dev_lock(hdev);
+	return hci_bdaddr_list_del(&hdev->reject_list, bdaddr, BDADDR_BREDR);
+}
 
-	err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+static int hci_get_conn_info(struct hci_dev *hdev, struct hci_conn_info_req *req,
+			     struct hci_conn_info *ci)
+{
+	struct hci_conn *conn;
+
+	conn = hci_conn_hash_lookup_ba(hdev, req->type, &req->bdaddr);
+	if (!conn)
+		return -ENOENT;
+	bacpy(&ci->bdaddr, &conn->dst);
+	ci->handle = conn->handle;
+	ci->type  = conn->type;
+	ci->out   = conn->out;
+	ci->state = conn->state;
+	ci->link_mode = get_link_mode(conn);
+	return 0;
+}
 
-	hci_dev_unlock(hdev);
+static int hci_get_auth_info(struct hci_dev *hdev, struct hci_auth_info_req *req)
+{
+	struct hci_conn *conn;
 
-	return err;
+	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req->bdaddr);
+	if (!conn)
+		return -ENOENT;
+	req->type = conn->auth_type;
+	return 0;
 }
 
 /* Ioctls that require bound socket */
-static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
-				unsigned long arg)
+static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 {
-	struct hci_dev *hdev = hci_pi(sk)->hdev;
+	struct hci_dev *hdev;
+	union {
+		bdaddr_t bdaddr;
+		struct hci_conn_info_req conn_req;
+		struct hci_auth_info_req auth_req;
+	} u;
+	struct hci_conn_info ci;
+	int err;
 
-	if (!hdev)
-		return -EBADFD;
+	if (cmd == HCIBLOCKADDR || cmd == HCIUNBLOCKADDR) {
+		if (copy_from_user(&u.bdaddr, arg, sizeof(u.bdaddr)))
+			return -EFAULT;
+	} else if (cmd == HCIGETCONNINFO) {
+		if (copy_from_user(&u.conn_req, arg, sizeof(u.conn_req)))
+			return -EFAULT;
+	} else if (cmd == HCIGETAUTHINFO) {
+		if (copy_from_user(&u.auth_req, arg, sizeof(u.auth_req)))
+			return -EFAULT;
+	}
 
-	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
-		return -EBUSY;
+	lock_sock(sk);
 
-	if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
-		return -EOPNOTSUPP;
+	hdev = hci_pi(sk)->hdev;
+	if (!hdev) {
+		err = -EBADFD;
+		goto out;
+	}
 
-	if (hdev->dev_type != HCI_PRIMARY)
-		return -EOPNOTSUPP;
+	if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
+		err = -EBUSY;
+		goto out;
+	}
+
+	if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (hdev->dev_type != HCI_PRIMARY) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
 
+	hci_dev_lock(hdev);
 	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, &u.conn_req, &ci);
+		break;
 	case HCIGETAUTHINFO:
-		return hci_get_auth_info(hdev, (void __user *)arg);
-
+		err = hci_get_auth_info(hdev, &u.auth_req);
+		break;
 	case HCIBLOCKADDR:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return hci_sock_reject_list_add(hdev, (void __user *)arg);
-
+			err = -EPERM;
+		else
+			err = hci_sock_reject_list_add(hdev, &u.bdaddr);
+		break;
 	case HCIUNBLOCKADDR:
 		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return hci_sock_reject_list_del(hdev, (void __user *)arg);
+			err = -EPERM;
+		else
+			err = hci_sock_reject_list_del(hdev, &u.bdaddr);
+		break;
+	default:
+		err = -ENOIOCTLCMD;
 	}
+	hci_dev_unlock(hdev);
+
+ out:
+	release_sock(sk);
 
-	return -ENOIOCTLCMD;
+	if (!err) {
+		if (cmd == HCIGETCONNINFO) {
+			if (copy_to_user(arg + sizeof(u.conn_req), &ci, sizeof(ci)))
+				err = -EFAULT;
+		} else if (cmd == HCIGETAUTHINFO) {
+			if (copy_to_user(arg, &u.auth_req, sizeof(u.auth_req)))
+				err = -EFAULT;
+		}
+	}
+	return err;
 }
 
 static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
@@ -975,15 +1027,14 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
 {
 	void __user *argp = (void __user *)arg;
 	struct sock *sk = sock->sk;
-	int err;
 
 	BT_DBG("cmd %x arg %lx", cmd, arg);
 
 	lock_sock(sk);
 
 	if (hci_pi(sk)->channel != HCI_CHANNEL_RAW) {
-		err = -EBADFD;
-		goto done;
+		release_sock(sk);
+		return -EBADFD;
 	}
 
 	/* When calling an ioctl on an unbound raw socket, then ensure
@@ -1055,13 +1106,7 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
 		return hci_inquiry(argp);
 	}
 
-	lock_sock(sk);
-
-	err = hci_sock_bound_ioctl(sk, cmd, arg);
-
-done:
-	release_sock(sk);
-	return err;
+	return hci_sock_bound_ioctl(sk, cmd, (void __user *)arg);
 }
 
 #ifdef CONFIG_COMPAT
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help