Re: [PATCH 3/4] listmount: small changes in semantics
From: "Serge E. Hallyn" <serge@hallyn.com>
Date: 2023-12-06 19:58:09
Also in:
linux-fsdevel, linux-man, linux-security-module
On Tue, Nov 28, 2023 at 05:03:34PM +0100, Miklos Szeredi wrote:
quoted hunk ↗ jump to hunk
1) Make permission checking consistent with statmount(2): fail if mount is unreachable from current root. Previously it failed if mount was unreachable from root->mnt->mnt_root. 2) List all submounts, even if unreachable from current root. This is safe, since 1) will prevent listing unreachable mounts for unprivileged users. 3) LSMT_ROOT is unchaged, it lists mounts under current root. Signed-off-by: Miklos Szeredi <redacted> --- fs/namespace.c | 39 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 25 deletions(-)diff --git a/fs/namespace.c b/fs/namespace.c index ad62cf7ee334..10cd651175b5 100644 --- a/fs/namespace.c +++ b/fs/namespace.c@@ -5004,37 +5004,26 @@ 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 vfsmount *mnt, +static ssize_t do_listmount(struct mount *first, struct path *orig, u64 mnt_id, u64 __user *buf, size_t bufsize, const struct path *root) { - struct mount *r, *m = real_mount(mnt); - struct path rootmnt = { - .mnt = root->mnt, - .dentry = root->mnt->mnt_root - }; - struct path orig; + struct mount *r; ssize_t ctr; int err; - if (!is_path_reachable(m, mnt->mnt_root, &rootmnt)) - return capable(CAP_SYS_ADMIN) ? 0 : -EPERM; + if (!capable(CAP_SYS_ADMIN) &&
Was there a reason to do the capable check first? In general, checking capable() when not needed is frowned upon, as it will set the PF_SUPERPRIV flag.
quoted hunk ↗ jump to hunk
+ !is_path_reachable(real_mount(orig->mnt), orig->dentry, root)) + return -EPERM; - err = security_sb_statfs(mnt->mnt_root); + err = security_sb_statfs(orig->dentry); if (err) return err; - if (root->mnt == mnt) { - orig = *root; - } else { - orig.mnt = mnt; - orig.dentry = mnt->mnt_root; - } - for (ctr = 0, r = first; r; r = listmnt_next(r)) { - if (r == m) + if (r->mnt_id_unique == mnt_id) continue; - if (!is_path_reachable(r, r->mnt.mnt_root, &orig)) + if (!is_path_reachable(r, r->mnt.mnt_root, orig)) continue; if (ctr >= bufsize)@@ -5053,9 +5042,8 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, { struct mnt_namespace *ns = current->nsproxy->mnt_ns; struct mnt_id_req kreq; - struct vfsmount *mnt; struct mount *first; - struct path root; + struct path root, orig; u64 mnt_id; ssize_t ret;@@ -5071,16 +5059,17 @@ SYSCALL_DEFINE4(listmount, const struct mnt_id_req __user *, req, down_read(&namespace_sem); get_fs_root(current->fs, &root); if (mnt_id == LSMT_ROOT) { - mnt = root.mnt; + orig = root; } else { ret = -ENOENT; - mnt = lookup_mnt_in_ns(mnt_id, ns); - if (!mnt) + orig.mnt = lookup_mnt_in_ns(mnt_id, ns); + if (!orig.mnt) goto err; + orig.dentry = orig.mnt->mnt_root; } first = node_to_mount(rb_first(&ns->mounts)); - ret = do_listmount(first, mnt, buf, bufsize, &root); + ret = do_listmount(first, &orig, mnt_id, buf, bufsize, &root); err: path_put(&root); up_read(&namespace_sem);-- 2.41.0