LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] proc: prevent mount proc on same mountpoint in one pid namespace
@ 2021-08-21  8:31 cgel.zte
  2021-08-21  8:58 ` Alexey Gladkov
  2021-08-21 14:10 ` Al Viro
  0 siblings, 2 replies; 4+ messages in thread
From: cgel.zte @ 2021-08-21  8:31 UTC (permalink / raw)
  To: viro, christian.brauner, jamorris, gladkov.alexey
  Cc: yang.yang29, tj, paul.gortmaker, linux-fsdevel, linux-kernel, Zeal Robot

From: Yang Yang <yang.yang29@zte.com.cn>

Patch "proc: allow to mount many instances of proc in one pid namespace"
aims to mount many instances of proc on different mountpoint, see
tools/testing/selftests/proc/proc-multiple-procfs.c.

But there is a side-effects, user can mount many instances of proc on
the same mountpoint in one pid namespace, which is not allowed before.
This duplicate mount makes no sense but wastes memory and CPU, and user
may be confused why kernel allows it.

The logic of this patch is: when try to mount proc on /mnt, check if
there is a proc instance mount on /mnt in the same pid namespace. If
answer is yes, return -EBUSY.

Since this check can't be done in proc_get_tree(), which call
get_tree_nodev() and will create new super_block unconditionally.
And other nodev fs may faces the same case, so add a new hook in
fs_context_operations.

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
---
 fs/namespace.c             |  9 +++++++++
 fs/proc/root.c             | 15 +++++++++++++++
 include/linux/fs_context.h |  1 +
 3 files changed, 25 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index f79d9471cb76..84da649a70c5 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2878,6 +2878,7 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
 static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
 			int mnt_flags, const char *name, void *data)
 {
+	int (*check_mntpoint)(struct fs_context *fc, struct path *path);
 	struct file_system_type *type;
 	struct fs_context *fc;
 	const char *subtype = NULL;
@@ -2906,6 +2907,13 @@ static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
 	if (IS_ERR(fc))
 		return PTR_ERR(fc);
 
+	/* check if there is a same super_block mount on path*/
+	check_mntpoint = fc->ops->check_mntpoint;
+	if (check_mntpoint)
+		err = check_mntpoint(fc, path);
+	if (err < 0)
+		goto err_fc;
+
 	if (subtype)
 		err = vfs_parse_fs_string(fc, "subtype",
 					  subtype, strlen(subtype));
@@ -2920,6 +2928,7 @@ static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
 	if (!err)
 		err = do_new_mount_fc(fc, path, mnt_flags);
 
+err_fc:
 	put_fs_context(fc);
 	return err;
 }
diff --git a/fs/proc/root.c b/fs/proc/root.c
index c7e3b1350ef8..0971d6b0bec2 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -237,11 +237,26 @@ static void proc_fs_context_free(struct fs_context *fc)
 	kfree(ctx);
 }
 
+static int proc_check_mntpoint(struct fs_context *fc, struct path *path)
+{
+	struct super_block *mnt_sb = path->mnt->mnt_sb;
+	struct proc_fs_info *fs_info;
+
+	if (strcmp(mnt_sb->s_type->name, "proc") == 0) {
+		fs_info = mnt_sb->s_fs_info;
+		if (fs_info->pid_ns == task_active_pid_ns(current) &&
+		    path->mnt->mnt_root == path->dentry)
+			return -EBUSY;
+	}
+	return 0;
+}
+
 static const struct fs_context_operations proc_fs_context_ops = {
 	.free		= proc_fs_context_free,
 	.parse_param	= proc_parse_param,
 	.get_tree	= proc_get_tree,
 	.reconfigure	= proc_reconfigure,
+	.check_mntpoint	= proc_check_mntpoint,
 };
 
 static int proc_init_fs_context(struct fs_context *fc)
diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
index 6b54982fc5f3..090a05fb2d7d 100644
--- a/include/linux/fs_context.h
+++ b/include/linux/fs_context.h
@@ -119,6 +119,7 @@ struct fs_context_operations {
 	int (*parse_monolithic)(struct fs_context *fc, void *data);
 	int (*get_tree)(struct fs_context *fc);
 	int (*reconfigure)(struct fs_context *fc);
+	int (*check_mntpoint)(struct fs_context *fc, struct path *path);
 };
 
 /*
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] proc: prevent mount proc on same mountpoint in one pid namespace
  2021-08-21  8:31 [PATCH] proc: prevent mount proc on same mountpoint in one pid namespace cgel.zte
@ 2021-08-21  8:58 ` Alexey Gladkov
  2021-08-21 14:10 ` Al Viro
  1 sibling, 0 replies; 4+ messages in thread
From: Alexey Gladkov @ 2021-08-21  8:58 UTC (permalink / raw)
  To: cgel.zte
  Cc: viro, christian.brauner, jamorris, yang.yang29, tj,
	paul.gortmaker, linux-fsdevel, linux-kernel, Zeal Robot

On Sat, Aug 21, 2021 at 01:31:05AM -0700, cgel.zte@gmail.com wrote:
> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> Patch "proc: allow to mount many instances of proc in one pid namespace"
> aims to mount many instances of proc on different mountpoint, see
> tools/testing/selftests/proc/proc-multiple-procfs.c.
> 
> But there is a side-effects, user can mount many instances of proc on
> the same mountpoint in one pid namespace, which is not allowed before.

It is not true. Even before this patch, it was possible to mount procfs
multiple times. The instance was the same, but the opportunity was
_always_ there.

If you forbid procfs from mounting multiple times, you get a huge
regression.

> This duplicate mount makes no sense but wastes memory and CPU, and user
> may be confused why kernel allows it.

You may need to mount procfs in one mount namespace if you are doing
chroot and want to grant him access to not the entire pid space.

$ mount -o hidpid=2 -t proc proc /path/to/chroot/proc
$ chroot /path/to/chroot
<switch to user>

> The logic of this patch is: when try to mount proc on /mnt, check if
> there is a proc instance mount on /mnt in the same pid namespace. If
> answer is yes, return -EBUSY.
> 
> Since this check can't be done in proc_get_tree(), which call
> get_tree_nodev() and will create new super_block unconditionally.
> And other nodev fs may faces the same case, so add a new hook in
> fs_context_operations.
> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> ---
>  fs/namespace.c             |  9 +++++++++
>  fs/proc/root.c             | 15 +++++++++++++++
>  include/linux/fs_context.h |  1 +
>  3 files changed, 25 insertions(+)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index f79d9471cb76..84da649a70c5 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2878,6 +2878,7 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
>  static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
>  			int mnt_flags, const char *name, void *data)
>  {
> +	int (*check_mntpoint)(struct fs_context *fc, struct path *path);
>  	struct file_system_type *type;
>  	struct fs_context *fc;
>  	const char *subtype = NULL;
> @@ -2906,6 +2907,13 @@ static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
>  	if (IS_ERR(fc))
>  		return PTR_ERR(fc);
>  
> +	/* check if there is a same super_block mount on path*/
> +	check_mntpoint = fc->ops->check_mntpoint;
> +	if (check_mntpoint)
> +		err = check_mntpoint(fc, path);
> +	if (err < 0)
> +		goto err_fc;
> +
>  	if (subtype)
>  		err = vfs_parse_fs_string(fc, "subtype",
>  					  subtype, strlen(subtype));
> @@ -2920,6 +2928,7 @@ static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
>  	if (!err)
>  		err = do_new_mount_fc(fc, path, mnt_flags);
>  
> +err_fc:
>  	put_fs_context(fc);
>  	return err;
>  }
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index c7e3b1350ef8..0971d6b0bec2 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -237,11 +237,26 @@ static void proc_fs_context_free(struct fs_context *fc)
>  	kfree(ctx);
>  }
>  
> +static int proc_check_mntpoint(struct fs_context *fc, struct path *path)
> +{
> +	struct super_block *mnt_sb = path->mnt->mnt_sb;
> +	struct proc_fs_info *fs_info;
> +
> +	if (strcmp(mnt_sb->s_type->name, "proc") == 0) {
> +		fs_info = mnt_sb->s_fs_info;
> +		if (fs_info->pid_ns == task_active_pid_ns(current) &&
> +		    path->mnt->mnt_root == path->dentry)
> +			return -EBUSY;
> +	}
> +	return 0;
> +}
> +
>  static const struct fs_context_operations proc_fs_context_ops = {
>  	.free		= proc_fs_context_free,
>  	.parse_param	= proc_parse_param,
>  	.get_tree	= proc_get_tree,
>  	.reconfigure	= proc_reconfigure,
> +	.check_mntpoint	= proc_check_mntpoint,
>  };
>  
>  static int proc_init_fs_context(struct fs_context *fc)
> diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
> index 6b54982fc5f3..090a05fb2d7d 100644
> --- a/include/linux/fs_context.h
> +++ b/include/linux/fs_context.h
> @@ -119,6 +119,7 @@ struct fs_context_operations {
>  	int (*parse_monolithic)(struct fs_context *fc, void *data);
>  	int (*get_tree)(struct fs_context *fc);
>  	int (*reconfigure)(struct fs_context *fc);
> +	int (*check_mntpoint)(struct fs_context *fc, struct path *path);
>  };
>  
>  /*
> -- 
> 2.25.1
> 

-- 
Rgrds, legion


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] proc: prevent mount proc on same mountpoint in one pid namespace
  2021-08-21  8:31 [PATCH] proc: prevent mount proc on same mountpoint in one pid namespace cgel.zte
  2021-08-21  8:58 ` Alexey Gladkov
@ 2021-08-21 14:10 ` Al Viro
  2021-08-22 13:06   ` CGEL
  1 sibling, 1 reply; 4+ messages in thread
From: Al Viro @ 2021-08-21 14:10 UTC (permalink / raw)
  To: cgel.zte
  Cc: christian.brauner, jamorris, gladkov.alexey, yang.yang29, tj,
	paul.gortmaker, linux-fsdevel, linux-kernel, Zeal Robot

On Sat, Aug 21, 2021 at 01:31:05AM -0700, cgel.zte@gmail.com wrote:
> From: Yang Yang <yang.yang29@zte.com.cn>
> 
> Patch "proc: allow to mount many instances of proc in one pid namespace"
> aims to mount many instances of proc on different mountpoint, see
> tools/testing/selftests/proc/proc-multiple-procfs.c.
> 
> But there is a side-effects, user can mount many instances of proc on
> the same mountpoint in one pid namespace, which is not allowed before.
> This duplicate mount makes no sense but wastes memory and CPU, and user
> may be confused why kernel allows it.
> 
> The logic of this patch is: when try to mount proc on /mnt, check if
> there is a proc instance mount on /mnt in the same pid namespace. If
> answer is yes, return -EBUSY.
> 
> Since this check can't be done in proc_get_tree(), which call
> get_tree_nodev() and will create new super_block unconditionally.
> And other nodev fs may faces the same case, so add a new hook in
> fs_context_operations.

NAK.  As attack prevention it's worthless (you can just bind-mount
a tmpfs directory between them).  Besides, filesystem does *not*
get to decide where it would be mounted.  Especially since it couldn't
rely upon that, anyway, what with mount --bind possible *after* it had
been initially mounted.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] proc: prevent mount proc on same mountpoint in one pid namespace
  2021-08-21 14:10 ` Al Viro
@ 2021-08-22 13:06   ` CGEL
  0 siblings, 0 replies; 4+ messages in thread
From: CGEL @ 2021-08-22 13:06 UTC (permalink / raw)
  To: Al Viro
  Cc: christian.brauner, jamorris, gladkov.alexey, yang.yang29, tj,
	paul.gortmaker, linux-fsdevel, linux-kernel, Zeal Robot

On Sat, Aug 21, 2021 at 02:10:16PM +0000, Al Viro wrote:
> On Sat, Aug 21, 2021 at 01:31:05AM -0700, cgel.zte@gmail.com wrote:
> > From: Yang Yang <yang.yang29@zte.com.cn>
> > 
> > Patch "proc: allow to mount many instances of proc in one pid namespace"
> > aims to mount many instances of proc on different mountpoint, see
> > tools/testing/selftests/proc/proc-multiple-procfs.c.
> > 
> > But there is a side-effects, user can mount many instances of proc on
> > the same mountpoint in one pid namespace, which is not allowed before.
> > This duplicate mount makes no sense but wastes memory and CPU, and user
> > may be confused why kernel allows it.
> > 
> > The logic of this patch is: when try to mount proc on /mnt, check if
> > there is a proc instance mount on /mnt in the same pid namespace. If
> > answer is yes, return -EBUSY.
> > 
> > Since this check can't be done in proc_get_tree(), which call
> > get_tree_nodev() and will create new super_block unconditionally.
> > And other nodev fs may faces the same case, so add a new hook in
> > fs_context_operations.
> 
> NAK.  As attack prevention it's worthless (you can just bind-mount
> a tmpfs directory between them).  Besides, filesystem does *not*
> get to decide where it would be mounted.  Especially since it couldn't
> rely upon that, anyway, what with mount --bind possible *after* it had
> been initially mounted.

Thanks for your relpy! No doubt anymore.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-08-22 13:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-21  8:31 [PATCH] proc: prevent mount proc on same mountpoint in one pid namespace cgel.zte
2021-08-21  8:58 ` Alexey Gladkov
2021-08-21 14:10 ` Al Viro
2021-08-22 13:06   ` CGEL

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).