Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210720104905.6870-1-penguin-kernel@I-love.SAKURA.ne.jp>
In-Reply-To: <20210720104905.6870-1-penguin-kernel@I-love.SAKURA.ne.jp>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 21 Jul 2021 11:17:52 -0700
Message-ID: <CABBYNZLt=fJTtdj9qvC22GkF_uYFe59D2bS3u61K14=Gq43Qbw@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: reorganize ioctls from hci_sock_bound_ioctl()
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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