diff mbox series

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

Message ID 20210717000731.3836303-1-luiz.dentz@gmail.com (mailing list archive)
State New, archived
Headers show
Series [RFC] Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG | expand

Commit Message

Luiz Augusto von Dentz July 17, 2021, 12:07 a.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

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 <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Tested-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 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(-)

Comments

bluez.test.bot@gmail.com July 17, 2021, 2:04 a.m. UTC | #1 
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
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL - 0.12 seconds
Run gitlint with rule in .gitlint
Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG
14: B1 Line exceeds max length (85>80): "Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")"


##############################
Test: BuildKernel - PASS - 680.73 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 445.11 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 3.15 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 2.21 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 35.57 seconds
Run test-runner with mgmt-tester
Total: 446, Passed: 443 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.48 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.44 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.50 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.033 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 2.32 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth
Tetsuo Handa July 18, 2021, 2:22 a.m. UTC | #2 
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);
    
    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().
Luiz Augusto von Dentz July 18, 2021, 5:16 a.m. UTC | #3 
Hi Tetsuo,

On Sat, Jul 17, 2021 at 7:22 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> 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.

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.
Tetsuo Handa July 18, 2021, 6:22 a.m. UTC | #4 
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 July 18, 2021, 6:51 a.m. UTC | #5 
Hi Tetsuo,

On Sat, Jul 17, 2021 at 11:22 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> 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().

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.
Tetsuo Handa July 18, 2021, 2:56 p.m. UTC | #6 
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
diff mbox series

Patch

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;
 }