| Message ID | e4277f3d-e725-273a-9537-f4fdc9539981@i-love.sakura.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl() | expand |
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 <penguin-kernel@i-love.sakura.ne.jp>' != 'Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>'
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 <penguin-kernel@I-love.SAKURA.ne.jp> > --- > 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
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
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 <penguin-kernel@I-love.SAKURA.ne.jp> --- 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(-)