diff mbox series

[3/4] listmount: small changes in semantics

Message ID 20231128160337.29094-4-mszeredi@redhat.com (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series listmount changes | expand

Commit Message

Miklos Szeredi Nov. 28, 2023, 4:03 p.m. UTC 
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 <mszeredi@redhat.com>
---
 fs/namespace.c | 39 ++++++++++++++-------------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

Comments

Serge E. Hallyn Dec. 6, 2023, 7:58 p.m. UTC | #1 
On Tue, Nov 28, 2023 at 05:03:34PM +0100, Miklos Szeredi wrote:
> 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 <mszeredi@redhat.com>
> ---
>  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.

> +	    !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
>
Miklos Szeredi Dec. 6, 2023, 8:24 p.m. UTC | #2 
On Wed, 6 Dec 2023 at 20:58, Serge E. Hallyn <serge@hallyn.com> wrote:
>
> On Tue, Nov 28, 2023 at 05:03:34PM +0100, Miklos Szeredi wrote:

> > -     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.
>

I synchronized the permission checking with statmount() without
thinking about the order.   I guess we can change the order back in
both syscalls?

I also don't understand the reason behind the using the _noaudit()
variant.  Christian?

Thanks,
Miklos
Christian Brauner Dec. 8, 2023, 1:07 p.m. UTC | #3 
On Wed, Dec 06, 2023 at 09:24:45PM +0100, Miklos Szeredi wrote:
> On Wed, 6 Dec 2023 at 20:58, Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > On Tue, Nov 28, 2023 at 05:03:34PM +0100, Miklos Szeredi wrote:
> 
> > > -     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.
> >
> 
> I synchronized the permission checking with statmount() without
> thinking about the order.   I guess we can change the order back in
> both syscalls?

I can just change the order. It's mostly a question of what is more
expensive. If there's such unpleasant side-effects... then sure I'll
reorder.

> I also don't understand the reason behind the using the _noaudit()
> variant.  Christian?

The reasoning is similar to the change in commit e7eda157c407 ("fs:
don't audit the capability check in simple_xattr_list()").

    "The check being unconditional may lead to unwanted denials reported by
    LSMs when a process has the capability granted by DAC, but denied by an
    LSM. In the case of SELinux such denials are a problem, since they can't
    be effectively filtered out via the policy and when not silenced, they
    produce noise that may hide a true problem or an attack."

So for system calls like listmount() that we can expect to be called a
lot of times (findmnt etc at some point) this would needlessly spam
dmesg without any value. We can always change that to an explicit
capable() later.
diff mbox series

Patch

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) &&
+	    !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);