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
