linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	"linux-bluetooth@vger.kernel.org"
	<linux-bluetooth@vger.kernel.org>, LinMa <linma@zju.edu.cn>
Subject: Re: [PATCH] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()
Date: Wed, 21 Jul 2021 11:17:52 -0700	[thread overview]
Message-ID: <CABBYNZLt=fJTtdj9qvC22GkF_uYFe59D2bS3u61K14=Gq43Qbw@mail.gmail.com> (raw)
In-Reply-To: <20210720104905.6870-1-penguin-kernel@I-love.SAKURA.ne.jp>

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

  parent reply	other threads:[~2021-07-21 18:18 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-20 10:49 Tetsuo Handa
2021-07-20 11:58 ` bluez.test.bot
2021-07-20 15:02 ` [PATCH v2] " Tetsuo Handa
2021-07-20 16:09   ` [v2] " bluez.test.bot
2021-07-21 18:17 ` Luiz Augusto von Dentz [this message]
2021-07-21 23:42   ` [PATCH] " Tetsuo Handa
2021-07-23 21:28     ` Luiz Augusto von Dentz
2021-07-31  2:40       ` [PATCH v3] " Tetsuo Handa
2021-07-31  3:15         ` [v3] " bluez.test.bot
2021-08-01 18:49         ` [PATCH v3] " Marcel Holtmann
2021-08-24 14:59           ` Tetsuo Handa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABBYNZLt=fJTtdj9qvC22GkF_uYFe59D2bS3u61K14=Gq43Qbw@mail.gmail.com' \
    --to=luiz.dentz@gmail.com \
    --cc=johan.hedberg@gmail.com \
    --cc=linma@zju.edu.cn \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=marcel@holtmann.org \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --subject='Re: [PATCH] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).