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
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
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
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
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
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.
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
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 v3:
Use helper function for each command to avoid re-evaluating
the command over and over.
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 | 179 +++++++++++++++++++++++--------
3 files changed, 139 insertions(+), 95 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..edda31556f19 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -892,37 +892,157 @@ 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 struct hci_dev *validate_hdev_from_sock(struct sock *sk)
{
- bdaddr_t bdaddr;
+ struct hci_dev *hdev = hci_pi(sk)->hdev;
+
+ if (!hdev)
+ return ERR_PTR(-EBADFD);
+ if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
+ return ERR_PTR(-EBUSY);
+ if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
+ return ERR_PTR(-EOPNOTSUPP);
+ if (hdev->dev_type != HCI_PRIMARY)
+ return ERR_PTR(-EOPNOTSUPP);
+ return hdev;
+}
+
+static int hci_set_raw(struct sock *sk)
+{
+ struct hci_dev *hdev;
int err;
- if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev))
+ err = PTR_ERR(hdev);
+ else if (!capable(CAP_NET_ADMIN))
+ err = -EPERM;
+ else
+ err = -EOPNOTSUPP;
+ release_sock(sk);
+ return err;
+}
+
+static int hci_get_conn_info(struct sock *sk, void __user *arg)
+{
+ struct hci_dev *hdev;
+ struct hci_conn_info_req req;
+ struct hci_conn_info ci;
+ struct hci_conn *conn;
+ int err = 0;
+ char __user *ptr = arg + sizeof(req);
+
+ if (copy_from_user(&req, arg, sizeof(req)))
return -EFAULT;
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev)) {
+ err = PTR_ERR(hdev);
+ goto out;
+ }
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 = hci_get_link_mode(conn);
+ } else {
+ err = -ENOENT;
+ }
+ hci_dev_unlock(hdev);
+ out:
+ release_sock(sk);
- err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+ if (!err)
+ err = copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
+ return err;
+}
+
+static int hci_get_auth_info(struct sock *sk, void __user *arg)
+{
+ struct hci_dev *hdev;
+ struct hci_auth_info_req req;
+ struct hci_conn *conn;
+ int err = 0;
+
+ if (copy_from_user(&req, arg, sizeof(req)))
+ return -EFAULT;
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev)) {
+ err = PTR_ERR(hdev);
+ goto out;
+ }
+ hci_dev_lock(hdev);
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
+ if (conn)
+ req.type = conn->auth_type;
+ else
+ err = -ENOENT;
hci_dev_unlock(hdev);
+ out:
+ release_sock(sk);
+ if (!err)
+ err = copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
return err;
}
-static int hci_sock_reject_list_del(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;
+ int err = 0;
if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
return -EFAULT;
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev)) {
+ err = PTR_ERR(hdev);
+ goto out;
+ } else if (!capable(CAP_NET_ADMIN)) {
+ err = -EPERM;
+ goto out;
+ }
hci_dev_lock(hdev);
+ err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+ hci_dev_unlock(hdev);
+ out:
+ release_sock(sk);
+ return err;
+}
- err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+static int hci_sock_reject_list_del(struct sock *sk, void __user *arg)
+{
+ struct hci_dev *hdev;
+ bdaddr_t bdaddr;
+ int err = 0;
- hci_dev_unlock(hdev);
+ if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
+ return -EFAULT;
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev)) {
+ err = PTR_ERR(hdev);
+ goto out;
+ } else if (!capable(CAP_NET_ADMIN)) {
+ err = -EPERM;
+ goto out;
+ }
+ hci_dev_lock(hdev);
+ err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+ hci_dev_unlock(hdev);
+ out:
+ release_sock(sk);
return err;
}
@@ -930,41 +1050,21 @@ static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
unsigned long arg)
{
- struct hci_dev *hdev = hci_pi(sk)->hdev;
-
- if (!hdev)
- return -EBADFD;
-
- if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
- return -EBUSY;
-
- if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
- return -EOPNOTSUPP;
-
- if (hdev->dev_type != HCI_PRIMARY)
- return -EOPNOTSUPP;
-
switch (cmd) {
case HCISETRAW:
- if (!capable(CAP_NET_ADMIN))
- return -EPERM;
- return -EOPNOTSUPP;
+ return hci_set_raw(sk);
case HCIGETCONNINFO:
- return hci_get_conn_info(hdev, (void __user *)arg);
+ return hci_get_conn_info(sk, (void __user *)arg);
case HCIGETAUTHINFO:
- return hci_get_auth_info(hdev, (void __user *)arg);
+ return hci_get_auth_info(sk, (void __user *)arg);
case HCIBLOCKADDR:
- if (!capable(CAP_NET_ADMIN))
- return -EPERM;
- return hci_sock_reject_list_add(hdev, (void __user *)arg);
+ return hci_sock_reject_list_add(sk, (void __user *)arg);
case HCIUNBLOCKADDR:
- if (!capable(CAP_NET_ADMIN))
- return -EPERM;
- return hci_sock_reject_list_del(hdev, (void __user *)arg);
+ return hci_sock_reject_list_del(sk, (void __user *)arg);
}
return -ENOIOCTLCMD;
@@ -975,15 +1075,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 +1154,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, arg);
}
#ifdef CONFIG_COMPAT
--
2.18.4
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=524307
---Test result---
Test Summary:
CheckPatch FAIL 1.01 seconds
GitLint PASS 0.12 seconds
BuildKernel PASS 606.09 seconds
TestRunner: Setup PASS 394.98 seconds
TestRunner: l2cap-tester PASS 2.81 seconds
TestRunner: bnep-tester PASS 2.11 seconds
TestRunner: mgmt-tester PASS 31.84 seconds
TestRunner: rfcomm-tester PASS 2.33 seconds
TestRunner: sco-tester PASS 2.22 seconds
TestRunner: smp-tester FAIL 2.27 seconds
TestRunner: userchan-tester PASS 2.11 seconds
Details
##############################
Test: CheckPatch - FAIL - 1.01 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, 321 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.12 seconds
Run gitlint with rule in .gitlint
##############################
Test: BuildKernel - PASS - 606.09 seconds
Build Kernel with minimal configuration supports Bluetooth
##############################
Test: TestRunner: Setup - PASS - 394.98 seconds
Setup environment for running Test Runner
##############################
Test: TestRunner: l2cap-tester - PASS - 2.81 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: bnep-tester - PASS - 2.11 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: mgmt-tester - PASS - 31.84 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3
##############################
Test: TestRunner: rfcomm-tester - PASS - 2.33 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: sco-tester - PASS - 2.22 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: smp-tester - FAIL - 2.27 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.024 seconds
##############################
Test: TestRunner: userchan-tester - PASS - 2.11 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
---
Regards,
Linux Bluetooth
Hi Tetsuo,
> 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 v3:
> Use helper function for each command to avoid re-evaluating
> the command over and over.
>
> 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 | 179 +++++++++++++++++++++++--------
> 3 files changed, 139 insertions(+), 95 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..edda31556f19 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -892,37 +892,157 @@ 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 struct hci_dev *validate_hdev_from_sock(struct sock *sk)
> {
> - bdaddr_t bdaddr;
> + struct hci_dev *hdev = hci_pi(sk)->hdev;
> +
> + if (!hdev)
> + return ERR_PTR(-EBADFD);
> + if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
> + return ERR_PTR(-EBUSY);
> + if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
> + return ERR_PTR(-EOPNOTSUPP);
> + if (hdev->dev_type != HCI_PRIMARY)
> + return ERR_PTR(-EOPNOTSUPP);
> + return hdev;
> +}
> +
> +static int hci_set_raw(struct sock *sk)
> +{
> + struct hci_dev *hdev;
> int err;
>
> - if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
> + lock_sock(sk);
> + hdev = validate_hdev_from_sock(sk);
> + if (IS_ERR(hdev))
> + err = PTR_ERR(hdev);
> + else if (!capable(CAP_NET_ADMIN))
> + err = -EPERM;
> + else
> + err = -EOPNOTSUPP;
> + release_sock(sk);
> + return err;
> +}
> +
> +static int hci_get_conn_info(struct sock *sk, void __user *arg)
> +{
> + struct hci_dev *hdev;
> + struct hci_conn_info_req req;
> + struct hci_conn_info ci;
> + struct hci_conn *conn;
> + int err = 0;
> + char __user *ptr = arg + sizeof(req);
> +
> + if (copy_from_user(&req, arg, sizeof(req)))
> return -EFAULT;
>
> + lock_sock(sk);
> + hdev = validate_hdev_from_sock(sk);
> + if (IS_ERR(hdev)) {
> + err = PTR_ERR(hdev);
> + goto out;
> + }
> 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 = hci_get_link_mode(conn);
> + } else {
> + err = -ENOENT;
> + }
> + hci_dev_unlock(hdev);
> + out:
> + release_sock(sk);
>
> - err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
> + if (!err)
> + err = copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
> + return err;
> +}
> +
> +static int hci_get_auth_info(struct sock *sk, void __user *arg)
> +{
> + struct hci_dev *hdev;
> + struct hci_auth_info_req req;
> + struct hci_conn *conn;
> + int err = 0;
> +
> + if (copy_from_user(&req, arg, sizeof(req)))
> + return -EFAULT;
>
> + lock_sock(sk);
> + hdev = validate_hdev_from_sock(sk);
> + if (IS_ERR(hdev)) {
> + err = PTR_ERR(hdev);
> + goto out;
> + }
> + hci_dev_lock(hdev);
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
> + if (conn)
> + req.type = conn->auth_type;
> + else
> + err = -ENOENT;
> hci_dev_unlock(hdev);
> + out:
> + release_sock(sk);
>
> + if (!err)
> + err = copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
> return err;
> }
>
> -static int hci_sock_reject_list_del(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;
> + int err = 0;
>
> if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
> return -EFAULT;
>
> + lock_sock(sk);
> + hdev = validate_hdev_from_sock(sk);
> + if (IS_ERR(hdev)) {
> + err = PTR_ERR(hdev);
> + goto out;
> + } else if (!capable(CAP_NET_ADMIN)) {
> + err = -EPERM;
> + goto out;
> + }
> hci_dev_lock(hdev);
> + err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
> + hci_dev_unlock(hdev);
> + out:
> + release_sock(sk);
> + return err;
> +}
>
> - err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
> +static int hci_sock_reject_list_del(struct sock *sk, void __user *arg)
> +{
> + struct hci_dev *hdev;
> + bdaddr_t bdaddr;
> + int err = 0;
>
> - hci_dev_unlock(hdev);
> + if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
> + return -EFAULT;
>
> + lock_sock(sk);
> + hdev = validate_hdev_from_sock(sk);
> + if (IS_ERR(hdev)) {
> + err = PTR_ERR(hdev);
> + goto out;
> + } else if (!capable(CAP_NET_ADMIN)) {
> + err = -EPERM;
> + goto out;
> + }
> + hci_dev_lock(hdev);
> + err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
> + hci_dev_unlock(hdev);
> + out:
> + release_sock(sk);
> return err;
> }
>
> @@ -930,41 +1050,21 @@ static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
> static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
> unsigned long arg)
> {
> - struct hci_dev *hdev = hci_pi(sk)->hdev;
> -
> - if (!hdev)
> - return -EBADFD;
> -
> - if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
> - return -EBUSY;
> -
> - if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
> - return -EOPNOTSUPP;
> -
> - if (hdev->dev_type != HCI_PRIMARY)
> - return -EOPNOTSUPP;
> -
what is the problem to just put this under lock_sock here globally. I am totally failing to see why you are moving all this code around.
> switch (cmd) {
> case HCISETRAW:
> - if (!capable(CAP_NET_ADMIN))
> - return -EPERM;
> - return -EOPNOTSUPP;
> + return hci_set_raw(sk);
>
> case HCIGETCONNINFO:
> - return hci_get_conn_info(hdev, (void __user *)arg);
> + return hci_get_conn_info(sk, (void __user *)arg);
>
> case HCIGETAUTHINFO:
> - return hci_get_auth_info(hdev, (void __user *)arg);
> + return hci_get_auth_info(sk, (void __user *)arg);
>
> case HCIBLOCKADDR:
> - if (!capable(CAP_NET_ADMIN))
> - return -EPERM;
> - return hci_sock_reject_list_add(hdev, (void __user *)arg);
> + return hci_sock_reject_list_add(sk, (void __user *)arg);
>
> case HCIUNBLOCKADDR:
> - if (!capable(CAP_NET_ADMIN))
> - return -EPERM;
I do not understand why are you moving the CAP_NET_ADMIN check. They are perfectly fine here. Moving these is just creating more complex if clauses in the functions. And that check most certainly doesn’t have to be done with lock_sock.
> - return hci_sock_reject_list_del(hdev, (void __user *)arg);
> + return hci_sock_reject_list_del(sk, (void __user *)arg);
> }
>
> return -ENOIOCTLCMD;
> @@ -975,15 +1075,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;
> }
So I don’t actually like the release_sock in an if clause.
>
> /* When calling an ioctl on an unbound raw socket, then ensure
> @@ -1055,13 +1154,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, arg);
> }
>
> #ifdef CONFIG_COMPAT
Regards
Marcel
On 2021/08/02 3:49, Marcel Holtmann wrote:
>> @@ -930,41 +1050,21 @@ static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
>> static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
>> unsigned long arg)
>> {
>> - struct hci_dev *hdev = hci_pi(sk)->hdev;
>> -
>> - if (!hdev)
>> - return -EBADFD;
>> -
>> - if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
>> - return -EBUSY;
>> -
>> - if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
>> - return -EOPNOTSUPP;
>> -
>> - if (hdev->dev_type != HCI_PRIMARY)
>> - return -EOPNOTSUPP;
>> -
>
> what is the problem to just put this under lock_sock here globally.
> I am totally failing to see why you are moving all this code around.
The intent of this patch is to avoid page fault with lock_sock and/or hci_dev_lock.
Since these checks are commonly done after lock_sock(), I moved these checks
to validate_hdev_from_sock() in order to make it possible to handle page fault
before calling lock_sock().
>
>> switch (cmd) {
>> case HCISETRAW:
>> - if (!capable(CAP_NET_ADMIN))
>> - return -EPERM;
>> - return -EOPNOTSUPP;
>> + return hci_set_raw(sk);
>>
>> case HCIGETCONNINFO:
>> - return hci_get_conn_info(hdev, (void __user *)arg);
>> + return hci_get_conn_info(sk, (void __user *)arg);
>>
>> case HCIGETAUTHINFO:
>> - return hci_get_auth_info(hdev, (void __user *)arg);
>> + return hci_get_auth_info(sk, (void __user *)arg);
>>
>> case HCIBLOCKADDR:
>> - if (!capable(CAP_NET_ADMIN))
>> - return -EPERM;
>> - return hci_sock_reject_list_add(hdev, (void __user *)arg);
>> + return hci_sock_reject_list_add(sk, (void __user *)arg);
>>
>> case HCIUNBLOCKADDR:
>> - if (!capable(CAP_NET_ADMIN))
>> - return -EPERM;
>
> I do not understand why are you moving the CAP_NET_ADMIN check.
> They are perfectly fine here. Moving these is just creating more
> complex if clauses in the functions. And that check most certainly
> doesn't have to be done with lock_sock.
Yes, capable() does not need to be done with lock_sock, but I just
wanted to preserve the ordering, for I considered that capable() is
expected to be checked after validate_hdev_from_sock() check.
I assumed that the ordering is important, for userspace might depend on
what error is returned by e.g. ioctl(HCISETRAW) which always returns an
error. If a userspace without CAP_NET_ADMIN capability were using e.g.
ioctl(HCISETRAW) for checking what validate_hdev_from_sock() checks, such
userspace will get confused by always getting -EPERM.
If userspace does not get confused by doing capable() before
validate_hdev_from_sock(), we can change the ordering (like a diff
shown bottom).
>
>> - return hci_sock_reject_list_del(hdev, (void __user *)arg);
>> + return hci_sock_reject_list_del(sk, (void __user *)arg);
>> }
>>
>> return -ENOIOCTLCMD;
>> @@ -975,15 +1075,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;
>> }
>
> So I don’t actually like the release_sock in an if clause.
OK, we can preserve "goto" if you like. But this is the only
location that will need to call release_sock(); use of "goto"
does not look smart to me.
Accepting your preferences, are you OK with below diff?
include/net/bluetooth/hci_core.h | 3 +-
net/bluetooth/hci_conn.c | 52 +---------
net/bluetooth/hci_sock.c | 167 ++++++++++++++++++++++++-------
3 files changed, 133 insertions(+), 89 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a7360c8c72f8..0e60aa193f19 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1275,8 +1275,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 55b0d177375b..68aff40f4e87 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -897,37 +897,149 @@ 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 struct hci_dev *validate_hdev_from_sock(struct sock *sk)
{
- bdaddr_t bdaddr;
+ struct hci_dev *hdev = hci_hdev_from_sock(sk);
+
+ if (IS_ERR(hdev))
+ return hdev;
+ if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
+ return ERR_PTR(-EBUSY);
+ if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
+ return ERR_PTR(-EOPNOTSUPP);
+ if (hdev->dev_type != HCI_PRIMARY)
+ return ERR_PTR(-EOPNOTSUPP);
+ return hdev;
+}
+
+static int hci_set_raw(struct sock *sk)
+{
+ struct hci_dev *hdev;
int err;
- if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev))
+ err = PTR_ERR(hdev);
+ else
+ err = -EOPNOTSUPP;
+ release_sock(sk);
+ return err;
+}
+
+static int hci_get_conn_info(struct sock *sk, void __user *arg)
+{
+ struct hci_dev *hdev;
+ struct hci_conn_info_req req;
+ struct hci_conn_info ci;
+ struct hci_conn *conn;
+ int err = 0;
+ char __user *ptr = arg + sizeof(req);
+
+ if (copy_from_user(&req, arg, sizeof(req)))
return -EFAULT;
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev)) {
+ err = PTR_ERR(hdev);
+ goto out;
+ }
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 = hci_get_link_mode(conn);
+ } else {
+ err = -ENOENT;
+ }
+ hci_dev_unlock(hdev);
+ out:
+ release_sock(sk);
- err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+ if (!err)
+ err = copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
+ return err;
+}
+static int hci_get_auth_info(struct sock *sk, void __user *arg)
+{
+ struct hci_dev *hdev;
+ struct hci_auth_info_req req;
+ struct hci_conn *conn;
+ int err = 0;
+
+ if (copy_from_user(&req, arg, sizeof(req)))
+ return -EFAULT;
+
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev)) {
+ err = PTR_ERR(hdev);
+ goto out;
+ }
+ hci_dev_lock(hdev);
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
+ if (conn)
+ req.type = conn->auth_type;
+ else
+ err = -ENOENT;
hci_dev_unlock(hdev);
+ out:
+ release_sock(sk);
+ if (!err)
+ err = copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
return err;
}
-static int hci_sock_reject_list_del(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;
+ int err = 0;
if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
return -EFAULT;
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev)) {
+ err = PTR_ERR(hdev);
+ goto out;
+ }
hci_dev_lock(hdev);
+ err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+ hci_dev_unlock(hdev);
+ out:
+ release_sock(sk);
+ return err;
+}
- err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+static int hci_sock_reject_list_del(struct sock *sk, void __user *arg)
+{
+ struct hci_dev *hdev;
+ bdaddr_t bdaddr;
+ int err = 0;
- hci_dev_unlock(hdev);
+ if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
+ return -EFAULT;
+ lock_sock(sk);
+ hdev = validate_hdev_from_sock(sk);
+ if (IS_ERR(hdev)) {
+ err = PTR_ERR(hdev);
+ goto out;
+ }
+ hci_dev_lock(hdev);
+ err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+ hci_dev_unlock(hdev);
+ out:
+ release_sock(sk);
return err;
}
@@ -935,41 +1047,27 @@ static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
unsigned long arg)
{
- struct hci_dev *hdev = hci_hdev_from_sock(sk);
-
- if (IS_ERR(hdev))
- return PTR_ERR(hdev);
-
- if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
- return -EBUSY;
-
- if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
- return -EOPNOTSUPP;
-
- if (hdev->dev_type != HCI_PRIMARY)
- return -EOPNOTSUPP;
-
switch (cmd) {
case HCISETRAW:
if (!capable(CAP_NET_ADMIN))
return -EPERM;
- return -EOPNOTSUPP;
+ return hci_set_raw(sk);
case HCIGETCONNINFO:
- return hci_get_conn_info(hdev, (void __user *)arg);
+ return hci_get_conn_info(sk, (void __user *)arg);
case HCIGETAUTHINFO:
- return hci_get_auth_info(hdev, (void __user *)arg);
+ return hci_get_auth_info(sk, (void __user *)arg);
case HCIBLOCKADDR:
if (!capable(CAP_NET_ADMIN))
return -EPERM;
- return hci_sock_reject_list_add(hdev, (void __user *)arg);
+ return hci_sock_reject_list_add(sk, (void __user *)arg);
case HCIUNBLOCKADDR:
if (!capable(CAP_NET_ADMIN))
return -EPERM;
- return hci_sock_reject_list_del(hdev, (void __user *)arg);
+ return hci_sock_reject_list_del(sk, (void __user *)arg);
}
return -ENOIOCTLCMD;
@@ -980,16 +1078,13 @@ 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;
- }
+ if (hci_pi(sk)->channel != HCI_CHANNEL_RAW)
+ goto out;
/* When calling an ioctl on an unbound raw socket, then ensure
* that the monitor gets informed. Ensure that the resulting event
@@ -1060,13 +1155,11 @@ 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);
+ return hci_sock_bound_ioctl(sk, cmd, arg);
-done:
+out:
release_sock(sk);
- return err;
+ return -EBADFD;
}
#ifdef CONFIG_COMPAT
--
2.18.4