2021-07-20 10:57:24

by Tetsuo Handa

[permalink] [raw]
Subject: [PATCH] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()

Since userfaultfd mechanism allows sleeping with kernel lock held,
avoiding page fault with kernel lock held where possible will make
the module more robust. This patch just brings copy_{from,to}_user()
calls to out of hdev lock and sock lock.

Signed-off-by: Tetsuo Handa <[email protected]>
---
include/net/bluetooth/hci_core.h | 3 +-
net/bluetooth/hci_conn.c | 50 +---------
net/bluetooth/hci_sock.c | 165 ++++++++++++++++++++-----------
3 files changed, 108 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..ff78b79ee09d 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -892,82 +892,136 @@ 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 = 0;

- if (!hdev)
- return -EBADFD;
+ if (cmd == HCIBLOCKADDR || cmd == HCIUNBLOCKADDR) {
+ if (copy_from_user(&u.bdaddr, arg, sizeof(u.bdaddr)))
+ err = -EFAULT;
+ } else if (cmd == HCIGETCONNINFO) {
+ if (copy_from_user(&u.conn_req, arg, sizeof(u.conn_req)))
+ err = -EFAULT;
+ } else if (cmd == HCIGETAUTHINFO) {
+ if (copy_from_user(&u.auth_req, arg, sizeof(u.auth_req)))
+ err = -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);
-
+ if (!err)
+ err = hci_get_conn_info(hdev, &u.conn_req, &ci);
+ break;
case HCIGETAUTHINFO:
- return hci_get_auth_info(hdev, (void __user *)arg);
-
+ if (!err)
+ 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 if (!err)
+ 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 if (!err)
+ 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 +1029,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 +1108,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
--
2.18.4


2021-07-20 11:59:55

by bluez.test.bot

[permalink] [raw]
Subject: RE: Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()

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=518235

---Test result---

Test Summary:
CheckPatch PASS 0.84 seconds
GitLint PASS 0.09 seconds
BuildKernel PASS 507.20 seconds
TestRunner: Setup PASS 341.45 seconds
TestRunner: l2cap-tester PASS 2.57 seconds
TestRunner: bnep-tester PASS 1.87 seconds
TestRunner: mgmt-tester PASS 30.77 seconds
TestRunner: rfcomm-tester PASS 2.02 seconds
TestRunner: sco-tester PASS 2.00 seconds
TestRunner: smp-tester FAIL 2.02 seconds
TestRunner: userchan-tester PASS 1.90 seconds

Details
##############################
Test: CheckPatch - PASS - 0.84 seconds
Run checkpatch.pl script with rule in .checkpatch.conf


##############################
Test: GitLint - PASS - 0.09 seconds
Run gitlint with rule in .gitlint


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


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


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

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

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

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

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

##############################
Test: TestRunner: smp-tester - FAIL - 2.02 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.018 seconds

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



---
Regards,
Linux Bluetooth


Attachments:
l2cap-tester.log (43.31 kB)
bnep-tester.log (3.47 kB)
mgmt-tester.log (602.37 kB)
rfcomm-tester.log (11.40 kB)
sco-tester.log (9.68 kB)
smp-tester.log (11.43 kB)
userchan-tester.log (5.33 kB)
Download all attachments

2021-07-20 15:13:39

by Tetsuo Handa

[permalink] [raw]
Subject: [PATCH v2] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()

Since userfaultfd mechanism allows sleeping with kernel lock held,
avoiding page fault with kernel lock held where possible will make
the module more robust. This patch just brings copy_{from,to}_user()
calls to out of hdev lock and sock lock.

Signed-off-by: Tetsuo Handa <[email protected]>
---
Changes in v2:
Rename get_link_mode() to hci_get_link_mode() to avoid
symbol name collision.

include/net/bluetooth/hci_core.h | 3 +-
net/bluetooth/hci_conn.c | 52 +---------
net/bluetooth/hci_sock.c | 165 ++++++++++++++++++++-----------
3 files changed, 109 insertions(+), 111 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a53e94459ecd..654677f59887 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 hci_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..ea2b538537aa 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 hci_get_link_mode(struct hci_conn *conn)
{
u32 link_mode = 0;

@@ -1683,7 +1683,7 @@ int hci_get_conn_list(void __user *arg)
(ci + n)->type = c->type;
(ci + n)->out = c->out;
(ci + n)->state = c->state;
- (ci + n)->link_mode = get_link_mode(c);
+ (ci + n)->link_mode = hci_get_link_mode(c);
if (++n >= req.conn_num)
break;
}
@@ -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..ef7fc3e9d471 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -892,82 +892,136 @@ 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 = hci_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 = 0;

- if (!hdev)
- return -EBADFD;
+ if (cmd == HCIBLOCKADDR || cmd == HCIUNBLOCKADDR) {
+ if (copy_from_user(&u.bdaddr, arg, sizeof(u.bdaddr)))
+ err = -EFAULT;
+ } else if (cmd == HCIGETCONNINFO) {
+ if (copy_from_user(&u.conn_req, arg, sizeof(u.conn_req)))
+ err = -EFAULT;
+ } else if (cmd == HCIGETAUTHINFO) {
+ if (copy_from_user(&u.auth_req, arg, sizeof(u.auth_req)))
+ err = -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);
-
+ if (!err)
+ err = hci_get_conn_info(hdev, &u.conn_req, &ci);
+ break;
case HCIGETAUTHINFO:
- return hci_get_auth_info(hdev, (void __user *)arg);
-
+ if (!err)
+ 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 if (!err)
+ 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 if (!err)
+ 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 +1029,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 +1108,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
--
2.18.4


2021-07-20 16:17:57

by bluez.test.bot

[permalink] [raw]
Subject: RE: [v2] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()

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=518371

---Test result---

Test Summary:
CheckPatch FAIL 0.82 seconds
GitLint PASS 0.10 seconds
BuildKernel PASS 509.45 seconds
TestRunner: Setup PASS 336.66 seconds
TestRunner: l2cap-tester PASS 2.55 seconds
TestRunner: bnep-tester PASS 1.88 seconds
TestRunner: mgmt-tester PASS 30.35 seconds
TestRunner: rfcomm-tester PASS 2.05 seconds
TestRunner: sco-tester PASS 1.99 seconds
TestRunner: smp-tester FAIL 2.03 seconds
TestRunner: userchan-tester PASS 1.92 seconds

Details
##############################
Test: CheckPatch - FAIL - 0.82 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()
WARNING: From:/Signed-off-by: email address mismatch: 'From: Tetsuo Handa <[email protected]>' != 'Signed-off-by: Tetsuo Handa <[email protected]>'

total: 0 errors, 1 warnings, 0 checks, 295 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: reorganize ioctls from hci_sock_bound_ioctl()" 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 - PASS - 0.10 seconds
Run gitlint with rule in .gitlint


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


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


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

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

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

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

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

##############################
Test: TestRunner: smp-tester - FAIL - 2.03 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.022 seconds

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



---
Regards,
Linux Bluetooth


Attachments:
l2cap-tester.log (43.31 kB)
bnep-tester.log (3.47 kB)
mgmt-tester.log (602.37 kB)
rfcomm-tester.log (11.40 kB)
sco-tester.log (9.68 kB)
smp-tester.log (11.43 kB)
userchan-tester.log (5.33 kB)
Download all attachments

2021-07-21 21:05:20

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()

Hi Tetsuo,

On Tue, Jul 20, 2021 at 3:49 AM Tetsuo Handa
<[email protected]> wrote:
>
> Since userfaultfd mechanism allows sleeping with kernel lock held,
> avoiding page fault with kernel lock held where possible will make
> the module more robust. This patch just brings copy_{from,to}_user()
> calls to out of hdev lock and sock lock.
>
> Signed-off-by: Tetsuo Handa <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 3 +-
> net/bluetooth/hci_conn.c | 50 +---------
> net/bluetooth/hci_sock.c | 165 ++++++++++++++++++++-----------
> 3 files changed, 108 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..ff78b79ee09d 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -892,82 +892,136 @@ 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 = 0;
>
> - if (!hdev)
> - return -EBADFD;
> + if (cmd == HCIBLOCKADDR || cmd == HCIUNBLOCKADDR) {
> + if (copy_from_user(&u.bdaddr, arg, sizeof(u.bdaddr)))
> + err = -EFAULT;
> + } else if (cmd == HCIGETCONNINFO) {
> + if (copy_from_user(&u.conn_req, arg, sizeof(u.conn_req)))
> + err = -EFAULT;
> + } else if (cmd == HCIGETAUTHINFO) {
> + if (copy_from_user(&u.auth_req, arg, sizeof(u.auth_req)))
> + err = -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);

I think it would have been cleaner if we have dedicated functions for
each command like I did in my patch:

https://patchwork.kernel.org/project/bluetooth/patch/[email protected]/

That way it is simpler to protect the likes of
copy_from_user/copy_to_user, etc, even if we have to some checks
duplicated on each function we can have a helper function to checks
the flags, etc.

> 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);
> -
> + if (!err)
> + err = hci_get_conn_info(hdev, &u.conn_req, &ci);
> + break;
> case HCIGETAUTHINFO:
> - return hci_get_auth_info(hdev, (void __user *)arg);
> -
> + if (!err)
> + 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 if (!err)
> + 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 if (!err)
> + 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 +1029,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 +1108,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
> --
> 2.18.4
>


--
Luiz Augusto von Dentz

2021-07-21 23:45:06

by Tetsuo Handa

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()

On 2021/07/22 3:17, Luiz Augusto von Dentz wrote:
> I think it would have been cleaner if we have dedicated functions for
> each command like I did in my patch:
>
> https://patchwork.kernel.org/project/bluetooth/patch/[email protected]/

But your patch was proven to be unsafe. There is a use-after-unregister
race window which would require at least 1000 lines of modification and
a lot of careful review if we try to manage without my patch.
Such all-in-one-step change is too much for "sleep in atomic context"
regression fix which is preventing syzbot from testing Bluetooth module
and is preventing Linux distributors from fixing CVE-2021-3573.

As far as I can see, it is lock_sock() (not bh_lock_sock_nested() in your
patch) that is needed for protecting

sk->sk_err = EPIPE;
sk->sk_state = BT_OPEN;
sk->sk_state_change(sk);

in hci_sock_dev_event(HCI_DEV_UNREG) from concurrent modification

lock_sock(sk);

if (sk->sk_state == BT_BOUND) {
err = -EALREADY;
goto done;
}

(...snipped...)

- hci_pi(sk)->hdev = hdev;
+ if (hdev) {
+ hci_pi(sk)->dev = hdev->id;
+ hci_dev_put(hdev);
+ }

(...snipped...)
/* Race window is here. */
(...snipped...)

sk->sk_state = BT_BOUND;
done:
release_sock(sk);

in hci_sock_bind().

>
> That way it is simpler to protect the likes of
> copy_from_user/copy_to_user, etc, even if we have to some checks
> duplicated on each function we can have a helper function to checks
> the flags, etc.

My patch calls copy_from_user()/copy_to_user() without lock_sock()
which works nicely with "[PATCH v3] Bluetooth: call lock_sock() outside
of spinlock section". I'd like to backport "[PATCH v2] Bluetooth:
reorganize ioctls from hci_sock_bound_ioctl()" together.

2021-07-23 21:31:15

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()

Hi Tetsuo,

On Wed, Jul 21, 2021 at 4:42 PM Tetsuo Handa
<[email protected]> wrote:
>
> On 2021/07/22 3:17, Luiz Augusto von Dentz wrote:
> > I think it would have been cleaner if we have dedicated functions for
> > each command like I did in my patch:
> >
> > https://patchwork.kernel.org/project/bluetooth/patch/[email protected]/
>
> But your patch was proven to be unsafe. There is a use-after-unregister
> race window which would require at least 1000 lines of modification and
> a lot of careful review if we try to manage without my patch.
> Such all-in-one-step change is too much for "sleep in atomic context"
> regression fix which is preventing syzbot from testing Bluetooth module
> and is preventing Linux distributors from fixing CVE-2021-3573.

Im not saying you should adopt my solution, the locking etc stay the
same as in this set but each command should have a helper function to
make it clearer that way we don't have to re-evaluate the command over
and over.

> As far as I can see, it is lock_sock() (not bh_lock_sock_nested() in your
> patch) that is needed for protecting
>
> sk->sk_err = EPIPE;
> sk->sk_state = BT_OPEN;
> sk->sk_state_change(sk);
>
> in hci_sock_dev_event(HCI_DEV_UNREG) from concurrent modification
>
> lock_sock(sk);
>
> if (sk->sk_state == BT_BOUND) {
> err = -EALREADY;
> goto done;
> }
>
> (...snipped...)
>
> - hci_pi(sk)->hdev = hdev;
> + if (hdev) {
> + hci_pi(sk)->dev = hdev->id;
> + hci_dev_put(hdev);
> + }
>
> (...snipped...)
> /* Race window is here. */
> (...snipped...)
>
> sk->sk_state = BT_BOUND;
> done:
> release_sock(sk);
>
> in hci_sock_bind().
>
> >
> > That way it is simpler to protect the likes of
> > copy_from_user/copy_to_user, etc, even if we have to some checks
> > duplicated on each function we can have a helper function to checks
> > the flags, etc.
>
> My patch calls copy_from_user()/copy_to_user() without lock_sock()
> which works nicely with "[PATCH v3] Bluetooth: call lock_sock() outside
> of spinlock section". I'd like to backport "[PATCH v2] Bluetooth:
> reorganize ioctls from hci_sock_bound_ioctl()" together.

Yep, Im not asking you to change any of that.


--
Luiz Augusto von Dentz