| Message ID | 20210720104905.6870-1-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | 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=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
Hi Tetsuo, On Tue, Jul 20, 2021 at 3:49 AM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> 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 <penguin-kernel@I-love.SAKURA.ne.jp> > --- > 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/20210717000731.3836303-1-luiz.dentz@gmail.com/ 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 >
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/20210717000731.3836303-1-luiz.dentz@gmail.com/ 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 <penguin-kernel@i-love.sakura.ne.jp> 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/20210717000731.3836303-1-luiz.dentz@gmail.com/ > > 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.
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
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> --- 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(-)