| Message ID | 20210720104905.6870-1-penguin-kernel@I-love.SAKURA.ne.jp |
|---|---|
| State | New |
| 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 > -- 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/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. -- Luiz Augusto von Dentz
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 -- 2.18.4
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(-)