fs: rework listmount() implementation

Linus pointed out that there's error handling and naming issues in the
that we should rewrite:

* Perform the access checks for the buffer before actually doing any
  work instead of doing it during the iteration.
* Rename the arguments to listmount() and do_listmount() to clarify what
  the arguments are used for.
* Get rid of the pointless ctr variable and overflow checking.
* Get rid of the pointless speculation check.

Link: https://lore.kernel.org/r/CAHk-=wjh6Cypo8WC-McXgSzCaou3UXccxB+7PVeSuGR8AjCphg@mail.gmail.com
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Christian Brauner <brauner@kernel.org>

diff --git a/fs/namespace.c b/fs/namespace.c
index ef1fd6829814cdf9f97ddf10968a11fc7b91a3c9..437f60e96d405861683f7e3596063d26c0e55038 100644 (file)
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -5042,13 +5042,12 @@ static struct mount *listmnt_next(struct mount *curr)
        return node_to_mount(rb_next(&curr->mnt_node));
 }
 
-static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
-                           u64 __user *buf, size_t bufsize,
-                           const struct path *root)
+static ssize_t do_listmount(struct mount *first, struct path *orig,
+                           u64 mnt_parent_id, u64 __user *mnt_ids,
+                           size_t nr_mnt_ids, const struct path *root)
 {
        struct mount *r;
-       ssize_t ctr;
-       int err;
+       ssize_t ret;
 
        /*
         * Don't trigger audit denials. We just want to determine what
@@ -5058,50 +5057,57 @@ static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id,
            !ns_capable_noaudit(&init_user_ns, CAP_SYS_ADMIN))
                return -EPERM;
 
-       err = security_sb_statfs(orig->dentry);
-       if (err)
-               return err;
+       ret = security_sb_statfs(orig->dentry);
+       if (ret)
+               return ret;
 
-       for (ctr = 0, r = first; r && ctr < bufsize; r = listmnt_next(r)) {
-               if (r->mnt_id_unique == mnt_id)
+       for (ret = 0, r = first; r && nr_mnt_ids; r = listmnt_next(r)) {
+               if (r->mnt_id_unique == mnt_parent_id)
                        continue;
                if (!is_path_reachable(r, r->mnt.mnt_root, orig))
                        continue;
-               ctr = array_index_nospec(ctr, bufsize);
-               if (put_user(r->mnt_id_unique, buf + ctr))
+               if (put_user(r->mnt_id_unique, mnt_ids))
                        return -EFAULT;
-               if (check_add_overflow(ctr, 1, &ctr))
-                       return -ERANGE;
+               mnt_ids++;
+               nr_mnt_ids--;
+               ret++;
        }
-       return ctr;
+       return ret;
 }
 
-SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
-               u64 __user *, buf, size_t, bufsize, unsigned int, flags)
+SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, u64 __user *,
+               mnt_ids, size_t, nr_mnt_ids, unsigned int, flags)
 {
        struct mnt_namespace *ns = current->nsproxy->mnt_ns;
        struct mnt_id_req kreq;
        struct mount *first;
        struct path root, orig;
-       u64 mnt_id, last_mnt_id;
+       u64 mnt_parent_id, last_mnt_id;
+       const size_t maxcount = (size_t)-1 >> 3;
        ssize_t ret;
 
        if (flags)
                return -EINVAL;
 
+       if (unlikely(nr_mnt_ids > maxcount))
+               return -EFAULT;
+
+       if (!access_ok(mnt_ids, nr_mnt_ids * sizeof(*mnt_ids)))
+               return -EFAULT;
+
        ret = copy_mnt_id_req(req, &kreq);
        if (ret)
                return ret;
-       mnt_id = kreq.mnt_id;
+       mnt_parent_id = kreq.mnt_id;
        last_mnt_id = kreq.param;
 
        down_read(&namespace_sem);
        get_fs_root(current->fs, &root);
-       if (mnt_id == LSMT_ROOT) {
+       if (mnt_parent_id == LSMT_ROOT) {
                orig = root;
        } else {
                ret = -ENOENT;
-               orig.mnt  = lookup_mnt_in_ns(mnt_id, ns);
+               orig.mnt = lookup_mnt_in_ns(mnt_parent_id, ns);
                if (!orig.mnt)
                        goto err;
                orig.dentry = orig.mnt->mnt_root;
@@ -5111,7 +5117,7 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req,
        else
                first = mnt_find_id_at(ns, last_mnt_id + 1);
 
-       ret = do_listmount(first, &orig, mnt_id, buf, bufsize, &root);
+       ret = do_listmount(first, &orig, mnt_parent_id, mnt_ids, nr_mnt_ids, &root);
 err:
        path_put(&root);
        up_read(&namespace_sem);

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 5c0dbef55792f269b6faa1aa81c1684f35839fe7..cdba4d0c6d4a88dd19db34faa425addb8cd3f744 100644 (file)
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -414,7 +414,7 @@ asmlinkage long sys_statmount(const struct mnt_id_req __user *req,
                              struct statmount __user *buf, size_t bufsize,
                              unsigned int flags);
 asmlinkage long sys_listmount(const struct mnt_id_req __user *req,
-                             u64 __user *buf, size_t bufsize,
+                             u64 __user *mnt_ids, size_t nr_mnt_ids,
                              unsigned int flags);
 asmlinkage long sys_truncate(const char __user *path, long length);
 asmlinkage long sys_ftruncate(unsigned int fd, unsigned long length);