[patch 00/10] (resend) mount ownership and unprivileged mount syscall

[patch 00/10] (resend) mount ownership and unprivileged mount syscall

[Miklos Szeredi]

This patchset adds support for keeping mount ownership information in
the kernel, and allow unprivileged mount(2) and umount(2) in certain
cases.

This can be useful for the following reasons:

- mount(8) can store ownership ("user=XY" option) in the kernel
  instead, or in addition to storing it in /etc/mtab.  For example if
  private namespaces are used with mount propagations /etc/mtab
  becomes unworkable, but using /proc/mounts works fine

- fuse won't need a special suid-root mount/umount utility.  Plain
  umount(8) can easily be made to work with unprivileged fuse mounts

- users can use bind mounts without having to pre-configure them in
  /etc/fstab

Unprivileged mounts are restricted to private namespaces created with
a special clone flag.

Changes from the previous submission:

 - add namespace flag for allowing user mounts
 - add clone flag to set above namespace flag
 - make max number of user mounts default to 1024, since now the
   namespace flag will prevent user mounts by default

--

[patch 01/10] add user mounts to the kernel

[Miklos Szeredi]

[-- Attachment #1: mount_owner.patch --]
[-- Type: text/plain, Size: 6258 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Add ownership information to mounts.

A new mount flag, MS_SETUSER is used to make a mount owned by a user.
If this flag is specified, then the owner will be set to the current
real user id and the mount will be marked with the MNT_USER flag.  On
remount don't preserve previous owner, and treat MS_SETUSER as for a
new mount.  The MS_SETUSER flag is ignored on mount move.

The MNT_USER flag is not copied on any kind of mount cloning:
namespace creation, binding or propagation.  For bind mounts the
cloned mount(s) are set to MNT_USER depending on the MS_SETUSER mount
flag.  In all the other cases MNT_USER is always cleared.

For MNT_USER mounts a "user=UID" option is added to /proc/PID/mounts.
This is compatible with how mount ownership is stored in /etc/mtab.

It is expected, that in the future mount(8) will use MS_SETUSER to
store mount ownership within the kernel.  This would help in
situations, where /etc/mtab is difficult or impossible to work with,
e.g. when using mount propagation.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2007-04-11 18:27:46.000000000 +0200
+++ linux/fs/namespace.c	2007-04-11 20:07:51.000000000 +0200
@@ -227,6 +227,13 @@ static struct vfsmount *skip_mnt_tree(st
 	return p;
 }
 
+static void set_mnt_user(struct vfsmount *mnt)
+{
+	BUG_ON(mnt->mnt_flags & MNT_USER);
+	mnt->mnt_uid = current->uid;
+	mnt->mnt_flags |= MNT_USER;
+}
+
 static struct vfsmount *clone_mnt(struct vfsmount *old, struct dentry *root,
 					int flag)
 {
@@ -241,6 +248,11 @@ static struct vfsmount *clone_mnt(struct
 		mnt->mnt_mountpoint = mnt->mnt_root;
 		mnt->mnt_parent = mnt;
 
+		/* don't copy the MNT_USER flag */
+		mnt->mnt_flags &= ~MNT_USER;
+		if (flag & CL_SETUSER)
+			set_mnt_user(mnt);
+
 		if (flag & CL_SLAVE) {
 			list_add(&mnt->mnt_slave, &old->mnt_slave_list);
 			mnt->mnt_master = old;
@@ -403,6 +415,8 @@ static int show_vfsmnt(struct seq_file *
 		if (mnt->mnt_flags & fs_infop->flag)
 			seq_puts(m, fs_infop->str);
 	}
+	if (mnt->mnt_flags & MNT_USER)
+		seq_printf(m, ",user=%i", mnt->mnt_uid);
 	if (mnt->mnt_sb->s_op->show_options)
 		err = mnt->mnt_sb->s_op->show_options(m, mnt);
 	seq_puts(m, " 0 0\n");
@@ -920,8 +934,9 @@ static int do_change_type(struct nameida
 /*
  * do loopback mount.
  */
-static int do_loopback(struct nameidata *nd, char *old_name, int recurse)
+static int do_loopback(struct nameidata *nd, char *old_name, int flags)
 {
+	int clone_flags;
 	struct nameidata old_nd;
 	struct vfsmount *mnt = NULL;
 	int err = mount_is_safe(nd);
@@ -941,11 +956,12 @@ static int do_loopback(struct nameidata 
 	if (!check_mnt(nd->mnt) || !check_mnt(old_nd.mnt))
 		goto out;
 
+	clone_flags = (flags & MS_SETUSER) ? CL_SETUSER : 0;
 	err = -ENOMEM;
-	if (recurse)
-		mnt = copy_tree(old_nd.mnt, old_nd.dentry, 0);
+	if (flags & MS_REC)
+		mnt = copy_tree(old_nd.mnt, old_nd.dentry, clone_flags);
 	else
-		mnt = clone_mnt(old_nd.mnt, old_nd.dentry, 0);
+		mnt = clone_mnt(old_nd.mnt, old_nd.dentry, clone_flags);
 
 	if (!mnt)
 		goto out;
@@ -987,8 +1003,11 @@ static int do_remount(struct nameidata *
 
 	down_write(&sb->s_umount);
 	err = do_remount_sb(sb, flags, data, 0);
-	if (!err)
+	if (!err) {
 		nd->mnt->mnt_flags = mnt_flags;
+		if (flags & MS_SETUSER)
+			set_mnt_user(nd->mnt);
+	}
 	up_write(&sb->s_umount);
 	if (!err)
 		security_sb_post_remount(nd->mnt, flags, data);
@@ -1093,10 +1112,13 @@ static int do_new_mount(struct nameidata
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	mnt = do_kern_mount(type, flags, name, data);
+	mnt = do_kern_mount(type, flags & ~MS_SETUSER, name, data);
 	if (IS_ERR(mnt))
 		return PTR_ERR(mnt);
 
+	if (flags & MS_SETUSER)
+		set_mnt_user(mnt);
+
 	return do_add_mount(mnt, nd, mnt_flags, NULL);
 }
 
@@ -1127,7 +1149,8 @@ int do_add_mount(struct vfsmount *newmnt
 	if (S_ISLNK(newmnt->mnt_root->d_inode->i_mode))
 		goto unlock;
 
-	newmnt->mnt_flags = mnt_flags;
+	/* MNT_USER was set earlier */
+	newmnt->mnt_flags |= mnt_flags;
 	if ((err = graft_tree(newmnt, nd)))
 		goto unlock;
 
@@ -1447,7 +1470,7 @@ long do_mount(char *dev_name, char *dir_
 		retval = do_remount(&nd, flags & ~MS_REMOUNT, mnt_flags,
 				    data_page);
 	else if (flags & MS_BIND)
-		retval = do_loopback(&nd, dev_name, flags & MS_REC);
+		retval = do_loopback(&nd, dev_name, flags);
 	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
 		retval = do_change_type(&nd, flags);
 	else if (flags & MS_MOVE)
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h	2007-04-11 18:27:46.000000000 +0200
+++ linux/include/linux/fs.h	2007-04-11 20:07:51.000000000 +0200
@@ -123,6 +123,7 @@ extern int dir_notify_enable;
 #define MS_SLAVE	(1<<19)	/* change to slave */
 #define MS_SHARED	(1<<20)	/* change to shared */
 #define MS_RELATIME	(1<<21)	/* Update atime relative to mtime/ctime. */
+#define MS_SETUSER	(1<<22) /* set mnt_uid to current user */
 #define MS_ACTIVE	(1<<30)
 #define MS_NOUSER	(1<<31)
 
Index: linux/include/linux/mount.h
===================================================================
--- linux.orig/include/linux/mount.h	2007-04-11 18:27:33.000000000 +0200
+++ linux/include/linux/mount.h	2007-04-11 20:07:51.000000000 +0200
@@ -28,6 +28,7 @@ struct mnt_namespace;
 #define MNT_NOATIME	0x08
 #define MNT_NODIRATIME	0x10
 #define MNT_RELATIME	0x20
+#define MNT_USER	0x40
 
 #define MNT_SHRINKABLE	0x100
 
@@ -61,6 +62,8 @@ struct vfsmount {
 	atomic_t mnt_count;
 	int mnt_expiry_mark;		/* true if marked for expiry */
 	int mnt_pinned;
+
+	uid_t mnt_uid;			/* owner of the mount */
 };
 
 static inline struct vfsmount *mntget(struct vfsmount *mnt)
Index: linux/fs/pnode.h
===================================================================
--- linux.orig/fs/pnode.h	2007-02-04 19:44:54.000000000 +0100
+++ linux/fs/pnode.h	2007-04-11 20:07:51.000000000 +0200
@@ -22,6 +22,7 @@
 #define CL_COPY_ALL 		0x04
 #define CL_MAKE_SHARED 		0x08
 #define CL_PROPAGATION 		0x10
+#define CL_SETUSER		0x20
 
 static inline void set_mnt_shared(struct vfsmount *mnt)
 {

--

[patch 02/10] allow unprivileged umount

[Miklos Szeredi]

[-- Attachment #1: unprivileged_umount.patch --]
[-- Type: text/plain, Size: 1373 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

The owner doesn't need sysadmin capabilities to call umount().

Similar behavior as umount(8) on mounts having "user=UID" option in
/etc/mtab.  The difference is that umount also checks /etc/fstab,
presumably to exclude another mount on the same mountpoint.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2007-04-11 20:07:51.000000000 +0200
+++ linux/fs/namespace.c	2007-04-11 20:08:05.000000000 +0200
@@ -659,6 +659,25 @@ static int do_umount(struct vfsmount *mn
 }
 
 /*
+ * umount is permitted for
+ *  - sysadmin
+ *  - mount owner, if not forced umount
+ */
+static bool permit_umount(struct vfsmount *mnt, int flags)
+{
+	if (capable(CAP_SYS_ADMIN))
+		return true;
+
+	if (!(mnt->mnt_flags & MNT_USER))
+		return false;
+
+	if (flags & MNT_FORCE)
+		return false;
+
+	return mnt->mnt_uid == current->uid;
+}
+
+/*
  * Now umount can handle mount points as well as block devices.
  * This is important for filesystems which use unnamed block devices.
  *
@@ -681,7 +700,7 @@ asmlinkage long sys_umount(char __user *
 		goto dput_and_out;
 
 	retval = -EPERM;
-	if (!capable(CAP_SYS_ADMIN))
+	if (!permit_umount(nd.mnt, flags))
 		goto dput_and_out;
 
 	retval = do_umount(nd.mnt, flags);

--

[patch 03/10] account user mounts

[Miklos Szeredi]

[-- Attachment #1: account_user_mounts.patch --]
[-- Type: text/plain, Size: 4798 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Add sysctl variables for accounting and limiting the number of user
mounts.

The maximum number of user mounts is set to 1024 by default.  This
won't in itself enable user mounts, setting the "permit user mount in
namespace" flag will also be needed.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/include/linux/sysctl.h
===================================================================
--- linux.orig/include/linux/sysctl.h	2007-04-11 18:27:46.000000000 +0200
+++ linux/include/linux/sysctl.h	2007-04-11 20:08:16.000000000 +0200
@@ -818,6 +818,8 @@ enum
 	FS_AIO_NR=18,	/* current system-wide number of aio requests */
 	FS_AIO_MAX_NR=19,	/* system-wide maximum number of aio requests */
 	FS_INOTIFY=20,	/* inotify submenu */
+	FS_NR_USER_MOUNTS=21,	/* int:current number of user mounts */
+	FS_MAX_USER_MOUNTS=22,	/* int:maximum number of user mounts */
 	FS_OCFS2=988,	/* ocfs2 */
 };
 
Index: linux/kernel/sysctl.c
===================================================================
--- linux.orig/kernel/sysctl.c	2007-04-11 18:27:46.000000000 +0200
+++ linux/kernel/sysctl.c	2007-04-11 20:08:16.000000000 +0200
@@ -1063,6 +1063,22 @@ static ctl_table fs_table[] = {
 #endif	
 #endif
 	{
+		.ctl_name	= FS_NR_USER_MOUNTS,
+		.procname	= "nr_user_mounts",
+		.data		= &nr_user_mounts,
+		.maxlen		= sizeof(int),
+		.mode		= 0444,
+		.proc_handler	= &proc_dointvec,
+	},
+	{
+		.ctl_name	= FS_MAX_USER_MOUNTS,
+		.procname	= "max_user_mounts",
+		.data		= &max_user_mounts,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec,
+	},
+	{
 		.ctl_name	= KERN_SETUID_DUMPABLE,
 		.procname	= "suid_dumpable",
 		.data		= &suid_dumpable,
Index: linux/Documentation/filesystems/proc.txt
===================================================================
--- linux.orig/Documentation/filesystems/proc.txt	2007-04-11 18:27:44.000000000 +0200
+++ linux/Documentation/filesystems/proc.txt	2007-04-12 13:32:14.000000000 +0200
@@ -923,6 +923,15 @@ reaches aio-max-nr then io_setup will fa
 raising aio-max-nr does not result in the pre-allocation or re-sizing
 of any kernel data structures.
 
+nr_user_mounts and max_user_mounts
+----------------------------------
+
+These represent the number of "user" mounts and the maximum number of
+"user" mounts respectively.  User mounts may be created by
+unprivileged users.  User mounts may also be created with sysadmin
+privileges on behalf of a user, in which case nr_user_mounts may
+exceed max_user_mounts.
+
 2.2 /proc/sys/fs/binfmt_misc - Miscellaneous binary formats
 -----------------------------------------------------------
 
Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2007-04-11 20:08:05.000000000 +0200
+++ linux/fs/namespace.c	2007-04-12 13:29:05.000000000 +0200
@@ -39,6 +39,9 @@ static int hash_mask __read_mostly, hash
 static struct kmem_cache *mnt_cache __read_mostly;
 static struct rw_semaphore namespace_sem;
 
+int nr_user_mounts;
+int max_user_mounts = 1024;
+
 /* /sys/fs */
 decl_subsys(fs, NULL, NULL);
 EXPORT_SYMBOL_GPL(fs_subsys);
@@ -227,11 +230,30 @@ static struct vfsmount *skip_mnt_tree(st
 	return p;
 }
 
+static void dec_nr_user_mounts(void)
+{
+	spin_lock(&vfsmount_lock);
+	nr_user_mounts--;
+	spin_unlock(&vfsmount_lock);
+}
+
 static void set_mnt_user(struct vfsmount *mnt)
 {
 	BUG_ON(mnt->mnt_flags & MNT_USER);
 	mnt->mnt_uid = current->uid;
 	mnt->mnt_flags |= MNT_USER;
+	spin_lock(&vfsmount_lock);
+	nr_user_mounts++;
+	spin_unlock(&vfsmount_lock);
+}
+
+static void clear_mnt_user(struct vfsmount *mnt)
+{
+	if (mnt->mnt_flags & MNT_USER) {
+		mnt->mnt_uid = 0;
+		mnt->mnt_flags &= ~MNT_USER;
+		dec_nr_user_mounts();
+	}
 }
 
 static struct vfsmount *clone_mnt(struct vfsmount *old, struct dentry *root,
@@ -283,6 +305,7 @@ static inline void __mntput(struct vfsmo
 {
 	struct super_block *sb = mnt->mnt_sb;
 	dput(mnt->mnt_root);
+	clear_mnt_user(mnt);
 	free_vfsmnt(mnt);
 	deactivate_super(sb);
 }
@@ -1023,6 +1046,7 @@ static int do_remount(struct nameidata *
 	down_write(&sb->s_umount);
 	err = do_remount_sb(sb, flags, data, 0);
 	if (!err) {
+		clear_mnt_user(nd->mnt);
 		nd->mnt->mnt_flags = mnt_flags;
 		if (flags & MS_SETUSER)
 			set_mnt_user(nd->mnt);
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h	2007-04-11 20:07:51.000000000 +0200
+++ linux/include/linux/fs.h	2007-04-12 13:26:42.000000000 +0200
@@ -50,6 +50,9 @@ extern struct inodes_stat_t inodes_stat;
 
 extern int leases_enable, lease_break_time;
 
+extern int nr_user_mounts;
+extern int max_user_mounts;
+
 #ifdef CONFIG_DNOTIFY
 extern int dir_notify_enable;
 #endif

--

[patch 04/10] add "permit user mounts" flag to namespaces

[Miklos Szeredi]

[-- Attachment #1: namespace_flag.patch --]
[-- Type: text/plain, Size: 2601 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

If MNT_NS_PERMIT_USERMOUNTS flag is not set for the current namespace,
then unprivileged mounts will be denied.

By default this flag is cleared in all namespaces.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2007-04-12 16:50:16.000000000 +0200
+++ linux/fs/namespace.c	2007-04-12 16:50:17.000000000 +0200
@@ -1526,6 +1526,19 @@ dput_out:
 	return retval;
 }
 
+static struct mnt_namespace *alloc_ns(void)
+{
+	struct mnt_namespace *ns;
+
+	ns = kzalloc(sizeof(struct mnt_namespace), GFP_KERNEL);
+	if (ns) {
+		atomic_set(&ns->count, 1);
+		INIT_LIST_HEAD(&ns->list);
+		init_waitqueue_head(&ns->poll);
+	}
+	return ns;
+}
+
 /*
  * Allocate a new namespace structure and populate it with contents
  * copied from the namespace of the passed in task structure.
@@ -1537,15 +1550,10 @@ static struct mnt_namespace *dup_mnt_ns(
 	struct vfsmount *rootmnt = NULL, *pwdmnt = NULL, *altrootmnt = NULL;
 	struct vfsmount *p, *q;
 
-	new_ns = kmalloc(sizeof(struct mnt_namespace), GFP_KERNEL);
+	new_ns = alloc_ns();
 	if (!new_ns)
 		return NULL;
 
-	atomic_set(&new_ns->count, 1);
-	INIT_LIST_HEAD(&new_ns->list);
-	init_waitqueue_head(&new_ns->poll);
-	new_ns->event = 0;
-
 	down_write(&namespace_sem);
 	/* First pass: copy the tree topology */
 	new_ns->root = copy_tree(mnt_ns->root, mnt_ns->root->mnt_root,
@@ -1860,13 +1868,10 @@ static void __init init_mount_tree(void)
 	mnt = do_kern_mount("rootfs", 0, "rootfs", NULL);
 	if (IS_ERR(mnt))
 		panic("Can't create rootfs");
-	ns = kmalloc(sizeof(*ns), GFP_KERNEL);
+	ns = alloc_ns();
 	if (!ns)
 		panic("Can't allocate initial namespace");
-	atomic_set(&ns->count, 1);
-	INIT_LIST_HEAD(&ns->list);
-	init_waitqueue_head(&ns->poll);
-	ns->event = 0;
+
 	list_add(&mnt->mnt_list, &ns->list);
 	ns->root = mnt;
 	mnt->mnt_ns = ns;
Index: linux/include/linux/mnt_namespace.h
===================================================================
--- linux.orig/include/linux/mnt_namespace.h	2007-04-12 16:50:02.000000000 +0200
+++ linux/include/linux/mnt_namespace.h	2007-04-12 16:50:17.000000000 +0200
@@ -6,12 +6,16 @@
 #include <linux/sched.h>
 #include <linux/nsproxy.h>
 
+/* mnt_namespace flags */
+#define MNT_NS_PERMIT_USERMOUNTS	(1 << 0)
+
 struct mnt_namespace {
 	atomic_t		count;
 	struct vfsmount *	root;
 	struct list_head	list;
 	wait_queue_head_t poll;
 	int event;
+	int flags;
 };
 
 extern struct mnt_namespace *copy_mnt_ns(int, struct mnt_namespace *,

--

[patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

[-- Attachment #1: mm-clone_allow_usermnt.patch --]
[-- Type: text/plain, Size: 2034 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

If CLONE_NEWNS and CLONE_NEWNS_USERMNT are given to clone(2) or
unshare(2), then allow user mounts within the new namespace.

This is not flexible enough, because user mounts can't be enabled for
the initial namespace.

The remaining clone bits also getting dangerously few...

Alternatives are:

  - prctl() flag
  - setting through the containers filesystem

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2007-04-12 13:46:19.000000000 +0200
+++ linux/fs/namespace.c	2007-04-12 13:54:36.000000000 +0200
@@ -1617,6 +1617,8 @@ struct mnt_namespace *copy_mnt_ns(int fl
 		return ns;
 
 	new_ns = dup_mnt_ns(ns, new_fs);
+	if (new_ns && (flags & CLONE_NEWNS_USERMNT))
+		new_ns->flags |= MNT_NS_PERMIT_USERMOUNTS;
 
 	put_mnt_ns(ns);
 	return new_ns;
Index: linux/include/linux/sched.h
===================================================================
--- linux.orig/include/linux/sched.h	2007-04-12 13:26:48.000000000 +0200
+++ linux/include/linux/sched.h	2007-04-12 13:54:36.000000000 +0200
@@ -26,6 +26,7 @@
 #define CLONE_STOPPED		0x02000000	/* Start in stopped state */
 #define CLONE_NEWUTS		0x04000000	/* New utsname group? */
 #define CLONE_NEWIPC		0x08000000	/* New ipcs */
+#define CLONE_NEWNS_USERMNT	0x10000000	/* Allow user mounts in ns? */
 
 /*
  * Scheduling policies
Index: linux/kernel/fork.c
===================================================================
--- linux.orig/kernel/fork.c	2007-04-11 18:27:46.000000000 +0200
+++ linux/kernel/fork.c	2007-04-12 13:59:10.000000000 +0200
@@ -1586,7 +1586,7 @@ asmlinkage long sys_unshare(unsigned lon
 	err = -EINVAL;
 	if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
 				CLONE_VM|CLONE_FILES|CLONE_SYSVSEM|
-				CLONE_NEWUTS|CLONE_NEWIPC))
+				CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWNS_USERMNT))
 		goto bad_unshare_out;
 
 	if ((err = unshare_thread(unshare_flags)))

--

Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Serge E. Hallyn]

Quoting Miklos Szeredi (miklos@szeredi.hu):
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> If CLONE_NEWNS and CLONE_NEWNS_USERMNT are given to clone(2) or
> unshare(2), then allow user mounts within the new namespace.
> 
> This is not flexible enough, because user mounts can't be enabled for
> the initial namespace.
> 
> The remaining clone bits also getting dangerously few...
> 
> Alternatives are:
> 
>   - prctl() flag
>   - setting through the containers filesystem

Sorry, I know I had mentioned it, but this is definately my least
favorite approach.

Curious whether are any other suggestions/opinions from the containers
list?

thanks,
-serge

> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> ---
> 
> Index: linux/fs/namespace.c
> ===================================================================
> --- linux.orig/fs/namespace.c	2007-04-12 13:46:19.000000000 +0200
> +++ linux/fs/namespace.c	2007-04-12 13:54:36.000000000 +0200
> @@ -1617,6 +1617,8 @@ struct mnt_namespace *copy_mnt_ns(int fl
>  		return ns;
> 
>  	new_ns = dup_mnt_ns(ns, new_fs);
> +	if (new_ns && (flags & CLONE_NEWNS_USERMNT))
> +		new_ns->flags |= MNT_NS_PERMIT_USERMOUNTS;
> 
>  	put_mnt_ns(ns);
>  	return new_ns;
> Index: linux/include/linux/sched.h
> ===================================================================
> --- linux.orig/include/linux/sched.h	2007-04-12 13:26:48.000000000 +0200
> +++ linux/include/linux/sched.h	2007-04-12 13:54:36.000000000 +0200
> @@ -26,6 +26,7 @@
>  #define CLONE_STOPPED		0x02000000	/* Start in stopped state */
>  #define CLONE_NEWUTS		0x04000000	/* New utsname group? */
>  #define CLONE_NEWIPC		0x08000000	/* New ipcs */
> +#define CLONE_NEWNS_USERMNT	0x10000000	/* Allow user mounts in ns? */
> 
>  /*
>   * Scheduling policies
> Index: linux/kernel/fork.c
> ===================================================================
> --- linux.orig/kernel/fork.c	2007-04-11 18:27:46.000000000 +0200
> +++ linux/kernel/fork.c	2007-04-12 13:59:10.000000000 +0200
> @@ -1586,7 +1586,7 @@ asmlinkage long sys_unshare(unsigned lon
>  	err = -EINVAL;
>  	if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
>  				CLONE_VM|CLONE_FILES|CLONE_SYSVSEM|
> -				CLONE_NEWUTS|CLONE_NEWIPC))
> +				CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWNS_USERMNT))
>  		goto bad_unshare_out;
> 
>  	if ((err = unshare_thread(unshare_flags)))
> 
> --
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Herbert Poetzl]

On Thu, Apr 12, 2007 at 03:32:08PM -0500, Serge E. Hallyn wrote:
> Quoting Miklos Szeredi (miklos@szeredi.hu):
> > From: Miklos Szeredi <mszeredi@suse.cz>
> > 
> > If CLONE_NEWNS and CLONE_NEWNS_USERMNT are given to clone(2) or
> > unshare(2), then allow user mounts within the new namespace.

> > This is not flexible enough, because user mounts can't be enabled for
> > the initial namespace.
> > 
> > The remaining clone bits also getting dangerously few...

ATM I think we do not have that many CLONE flags
available, so that this feature will have to wait
for a clone2/64 or similar ...

> > Alternatives are:
> > 
> >   - prctl() flag
> >   - setting through the containers filesystem

> Sorry, I know I had mentioned it, but this is definately my least
> favorite approach.
> 
> Curious whether are any other suggestions/opinions from the containers
> list?

question: how is mounting filesystems (loopback,
fuse, etc) secured in such way that the user
cannot 'create' device nodes with 'unfortunate'
permissions?

TIA,
Herbert

> thanks,
> -serge
> 
> > Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> > ---
> > 
> > Index: linux/fs/namespace.c
> > ===================================================================
> > --- linux.orig/fs/namespace.c	2007-04-12 13:46:19.000000000 +0200
> > +++ linux/fs/namespace.c	2007-04-12 13:54:36.000000000 +0200
> > @@ -1617,6 +1617,8 @@ struct mnt_namespace *copy_mnt_ns(int fl
> >  		return ns;
> > 
> >  	new_ns = dup_mnt_ns(ns, new_fs);
> > +	if (new_ns && (flags & CLONE_NEWNS_USERMNT))
> > +		new_ns->flags |= MNT_NS_PERMIT_USERMOUNTS;
> > 
> >  	put_mnt_ns(ns);
> >  	return new_ns;
> > Index: linux/include/linux/sched.h
> > ===================================================================
> > --- linux.orig/include/linux/sched.h	2007-04-12 13:26:48.000000000 +0200
> > +++ linux/include/linux/sched.h	2007-04-12 13:54:36.000000000 +0200
> > @@ -26,6 +26,7 @@
> >  #define CLONE_STOPPED		0x02000000	/* Start in stopped state */
> >  #define CLONE_NEWUTS		0x04000000	/* New utsname group? */
> >  #define CLONE_NEWIPC		0x08000000	/* New ipcs */
> > +#define CLONE_NEWNS_USERMNT	0x10000000	/* Allow user mounts in ns? */
> > 
> >  /*
> >   * Scheduling policies
> > Index: linux/kernel/fork.c
> > ===================================================================
> > --- linux.orig/kernel/fork.c	2007-04-11 18:27:46.000000000 +0200
> > +++ linux/kernel/fork.c	2007-04-12 13:59:10.000000000 +0200
> > @@ -1586,7 +1586,7 @@ asmlinkage long sys_unshare(unsigned lon
> >  	err = -EINVAL;
> >  	if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
> >  				CLONE_VM|CLONE_FILES|CLONE_SYSVSEM|
> > -				CLONE_NEWUTS|CLONE_NEWIPC))
> > +				CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWNS_USERMNT))
> >  		goto bad_unshare_out;
> > 
> >  	if ((err = unshare_thread(unshare_flags)))
> > 
> > --
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> question: how is mounting filesystems (loopback,
> fuse, etc) secured in such way that the user
> cannot 'create' device nodes with 'unfortunate'
> permissions?

All unprivileged mounts have "nosuid,nodev" added to their options.

Miklos

Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Eric W. Biederman]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Quoting Miklos Szeredi (miklos@szeredi.hu):
>> From: Miklos Szeredi <mszeredi@suse.cz>
>> 
>> If CLONE_NEWNS and CLONE_NEWNS_USERMNT are given to clone(2) or
>> unshare(2), then allow user mounts within the new namespace.
>> 
>> This is not flexible enough, because user mounts can't be enabled for
>> the initial namespace.
>> 
>> The remaining clone bits also getting dangerously few...
>> 
>> Alternatives are:
>> 
>>   - prctl() flag
>>   - setting through the containers filesystem
>
> Sorry, I know I had mentioned it, but this is definately my least
> favorite approach.
>
> Curious whether are any other suggestions/opinions from the containers
> list?

Given the existence of shared subtrees allowing/denying this at the mount
namespace level is silly and wrong.

If we need more than just the filesystem permission checks can we
make it a mount flag settable with mount and remount that allows
non-privileged users the ability to create mount points under it
in directories they have full read/write access to.

I don't like the use of clone flags for this purpose but in this
case the shared subtress are a much more fundamental reasons for not
doing this at the namespace level.

Eric

Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> Given the existence of shared subtrees allowing/denying this at the mount
> namespace level is silly and wrong.
> 
> If we need more than just the filesystem permission checks can we
> make it a mount flag settable with mount and remount that allows
> non-privileged users the ability to create mount points under it
> in directories they have full read/write access to.

OK, that makes sense.

> I don't like the use of clone flags for this purpose but in this
> case the shared subtress are a much more fundamental reasons for not
> doing this at the namespace level.

I'll drop the clone flag, and add a mount flag instead.

Thanks,
Miklos

Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Serge E. Hallyn]

Quoting Miklos Szeredi (miklos@szeredi.hu):
> > Given the existence of shared subtrees allowing/denying this at the mount
> > namespace level is silly and wrong.
> > 
> > If we need more than just the filesystem permission checks can we
> > make it a mount flag settable with mount and remount that allows
> > non-privileged users the ability to create mount points under it
> > in directories they have full read/write access to.
> 
> OK, that makes sense.
> 
> > I don't like the use of clone flags for this purpose but in this
> > case the shared subtress are a much more fundamental reasons for not
> > doing this at the namespace level.
> 
> I'll drop the clone flag, and add a mount flag instead.
> 
> Thanks,
> Miklos

Makes sense, so then on login pam has to spawn a new user namespace and
construct a root fs with no shared subtrees and with the
user-mounts-allowed flag specified?

-serge

Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Eric W. Biederman]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Quoting Miklos Szeredi (miklos@szeredi.hu):
>> > Given the existence of shared subtrees allowing/denying this at the mount
>> > namespace level is silly and wrong.
>> > 
>> > If we need more than just the filesystem permission checks can we
>> > make it a mount flag settable with mount and remount that allows
>> > non-privileged users the ability to create mount points under it
>> > in directories they have full read/write access to.
>> 
>> OK, that makes sense.
>> 
>> > I don't like the use of clone flags for this purpose but in this
>> > case the shared subtress are a much more fundamental reasons for not
>> > doing this at the namespace level.
>> 
>> I'll drop the clone flag, and add a mount flag instead.
>> 
>> Thanks,
>> Miklos
>
> Makes sense, so then on login pam has to spawn a new user namespace and
> construct a root fs with no shared subtrees and with the
> user-mounts-allowed flag specified?

I was expecting the usage in the normal case to be the Al Viro style
with shared subtrees setup for each user, with the shared subtree marked
with user-mounts-allowed.  Then on login pam would unshare the namespace
and restrict the user to their specific portion of the shared subtree.

If you don't use multiple mount namespaces all of the users have to
agree on what they want the non-privileged part of the namespace
to look like.

If you don't use shared subtrees you have to deal with all of the
joys of implementing enter, or else multiple logins from the same user
have problems.

Eric

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Ram Pai]

> 
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> > Quoting Miklos Szeredi (miklos@szeredi.hu):
> >> From: Miklos Szeredi <mszeredi@suse.cz>
> >> 
> >> If CLONE_NEWNS and CLONE_NEWNS_USERMNT are given to clone(2) or
> >> unshare(2), then allow user mounts within the new namespace.
> >> 
> >> This is not flexible enough, because user mounts can't be enabled
> for
> >> the initial namespace.
> >> 
> >> The remaining clone bits also getting dangerously few...
> >> 
> >> Alternatives are:
> >> 
> >>   - prctl() flag
> >>   - setting through the containers filesystem
> >
> > Sorry, I know I had mentioned it, but this is definately my least
> > favorite approach.
> >
> > Curious whether are any other suggestions/opinions from the
> containers
> > list?
> 
> Given the existence of shared subtrees allowing/denying this at the
> mount
> namespace level is silly and wrong.
> 
> If we need more than just the filesystem permission checks can we
> make it a mount flag settable with mount and remount that allows
> non-privileged users the ability to create mount points under it
> in directories they have full read/write access to.

Also for bind-mount and remount operations the flag has to be propagated
down its propagation tree.  Otherwise a unpriviledged mount in a shared
mount wont get reflected in its peers and slaves, leading to unidentical
shared-subtrees.

RP


> 
> I don't like the use of clone flags for this purpose but in this
> case the shared subtress are a much more fundamental reasons for not
> doing this at the namespace level.
> 
> Eric
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/containers 

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > Given the existence of shared subtrees allowing/denying this at the
> > mount
> > namespace level is silly and wrong.
> > 
> > If we need more than just the filesystem permission checks can we
> > make it a mount flag settable with mount and remount that allows
> > non-privileged users the ability to create mount points under it
> > in directories they have full read/write access to.
> 
> Also for bind-mount and remount operations the flag has to be propagated
> down its propagation tree.  Otherwise a unpriviledged mount in a shared
> mount wont get reflected in its peers and slaves, leading to unidentical
> shared-subtrees.

That's an interesting question.  Do we want shared mounts to be
totally identical, including mnt_flags?  It doesn't look as if
do_remount() guarantees that currently.

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Ram Pai]

On Mon, 2007-04-16 at 11:32 +0200, Miklos Szeredi wrote:
> > > Given the existence of shared subtrees allowing/denying this at the
> > > mount
> > > namespace level is silly and wrong.
> > > 
> > > If we need more than just the filesystem permission checks can we
> > > make it a mount flag settable with mount and remount that allows
> > > non-privileged users the ability to create mount points under it
> > > in directories they have full read/write access to.
> > 
> > Also for bind-mount and remount operations the flag has to be propagated
> > down its propagation tree.  Otherwise a unpriviledged mount in a shared
> > mount wont get reflected in its peers and slaves, leading to unidentical
> > shared-subtrees.
> 
> That's an interesting question.  Do we want shared mounts to be
> totally identical, including mnt_flags?  It doesn't look as if
> do_remount() guarantees that currently.

Depends on the semantics of each of the flags. Some flags like of the
read/write flag, would not interfere with the propagation semantics
AFAICT.  But this one certainly seems to interfere.

RP

> Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > > Also for bind-mount and remount operations the flag has to be propagated
> > > down its propagation tree.  Otherwise a unpriviledged mount in a shared
> > > mount wont get reflected in its peers and slaves, leading to unidentical
> > > shared-subtrees.
> > 
> > That's an interesting question.  Do we want shared mounts to be
> > totally identical, including mnt_flags?  It doesn't look as if
> > do_remount() guarantees that currently.
> 
> Depends on the semantics of each of the flags. Some flags like of the
> read/write flag, would not interfere with the propagation semantics
> AFAICT.  But this one certainly seems to interfere.

That depends.  Current patches check the "unprivileged submounts
allowed under this mount" flag only on the requested mount and not on
the propagated mounts.  Do you see a problem with this?

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Eric W. Biederman]

Miklos Szeredi <miklos@szeredi.hu> writes:

> That depends.  Current patches check the "unprivileged submounts
> allowed under this mount" flag only on the requested mount and not on
> the propagated mounts.  Do you see a problem with this?

I think privileges of this sort should propagate.  If I read what you
just said correctly if I have a private mount namespace I won't be able
to mount anything unless when it was setup the unprivileged submount
command was explicitly set.

Eric

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > That depends.  Current patches check the "unprivileged submounts
> > allowed under this mount" flag only on the requested mount and not on
> > the propagated mounts.  Do you see a problem with this?
> 
> I think privileges of this sort should propagate.  If I read what you
> just said correctly if I have a private mount namespace I won't be able
> to mount anything unless when it was setup the unprivileged submount
> command was explicitly set.

By design yes.  Why is that a problem?

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Eric W. Biederman]

Miklos Szeredi <miklos@szeredi.hu> writes:

>> > That depends.  Current patches check the "unprivileged submounts
>> > allowed under this mount" flag only on the requested mount and not on
>> > the propagated mounts.  Do you see a problem with this?
>> 
>> I think privileges of this sort should propagate.  If I read what you
>> just said correctly if I have a private mount namespace I won't be able
>> to mount anything unless when it was setup the unprivileged submount
>> command was explicitly set.
>
> By design yes.  Why is that a problem?

It certainly doesn't match my intuition.

Why are directory permissions not sufficient to allow/deny non-priveleged mounts?
I don't understand that contention yet.

I should probably go back and look and see how plan9 handles mount/unmount
permissions.  Plan9 gets away with a lot more because it doesn't have
a suid bit and mount namespaces were always present, so they don't have
backwards compatibility problems.

My best guess at the moment is that plan9 treated mount/unmount as
completely unprivileged and used the mount namespaces to limit the
scope of what would be affected by a mount/unmount operation.  I think
that may be reasonable in linux as well but it will require the
presence of a mount namespace to limit the affects of what a user can
do.

So short of a more thorough audit I believe the final semantics should
be: 
- mount/unmount for non-priveleged processes should only be limited
  by the mount namespace and directory permissions.
- CLONE_NEWNS should not be a privileged operation. 

What prevents us from allowing these things?

- Unprivileged CLONE_NEWNS and unprivileged mounts needs resource
  accounting so we don't have a denial of service attack.

- Unprivileged mounts must be limited to directories that we have
  permission to modify in a way that we could get the same effect
  as the mount or unmount operation in terms of what files are visible
  otherwise we can mess up SUID executables.

- Anything else?

There are user space issues such as a reasonable pam module and how
to do backups.  However those are user space issues.

What am I missing that requires us to add MNT_USER and MNT_USERMNT?

Eric

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@xmission.com):
> Miklos Szeredi <miklos@szeredi.hu> writes:
> 
> >> > That depends.  Current patches check the "unprivileged submounts
> >> > allowed under this mount" flag only on the requested mount and not on
> >> > the propagated mounts.  Do you see a problem with this?
> >> 
> >> I think privileges of this sort should propagate.  If I read what you
> >> just said correctly if I have a private mount namespace I won't be able
> >> to mount anything unless when it was setup the unprivileged submount
> >> command was explicitly set.
> >
> > By design yes.  Why is that a problem?
> 
> It certainly doesn't match my intuition.
> 
> Why are directory permissions not sufficient to allow/deny non-priveleged mounts?
> I don't understand that contention yet.

The same scenarios laid out previously in this thread.  I.e.

1. user hallyn does mount --bind / /home/hallyn/root
2. (...)
3. admin does "deluser hallyn"

and deluser starts wiping out root

Or,

1. user hallyn does mount --bind / /home/hallyn/root
2. backup daemon starts backing up /home/hallyn/root/home/hallyn/root/home...

So we started down the path of forcing users to clone a new namespace
before doing user mounts, which is what the clone flag was about.  Using
per-mount flags also suffices as you had pointed out, which is being
done here.  But directory permissions are inadequate.

(Unless you want to tackle each problem legacy tool one at a time to
remove problems - i.e. deluser should umount everything under
/home/hallyn before deleting, backup should be spawned from it's own
namespace cloned right after boot or just back up on one filesystem,
etc.)

-serge

> I should probably go back and look and see how plan9 handles mount/unmount
> permissions.  Plan9 gets away with a lot more because it doesn't have
> a suid bit and mount namespaces were always present, so they don't have
> backwards compatibility problems.
> 
> My best guess at the moment is that plan9 treated mount/unmount as
> completely unprivileged and used the mount namespaces to limit the
> scope of what would be affected by a mount/unmount operation.  I think
> that may be reasonable in linux as well but it will require the
> presence of a mount namespace to limit the affects of what a user can
> do.
> 
> So short of a more thorough audit I believe the final semantics should
> be: 
> - mount/unmount for non-priveleged processes should only be limited
>   by the mount namespace and directory permissions.
> - CLONE_NEWNS should not be a privileged operation. 
> 
> What prevents us from allowing these things?
> 
> - Unprivileged CLONE_NEWNS and unprivileged mounts needs resource
>   accounting so we don't have a denial of service attack.
> 
> - Unprivileged mounts must be limited to directories that we have
>   permission to modify in a way that we could get the same effect
>   as the mount or unmount operation in terms of what files are visible
>   otherwise we can mess up SUID executables.
> 
> - Anything else?
> 
> There are user space issues such as a reasonable pam module and how
> to do backups.  However those are user space issues.
> 
> What am I missing that requires us to add MNT_USER and MNT_USERMNT?
> 
> Eric
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Eric W. Biederman]

"Serge E. Hallyn" <serue@us.ibm.com> writes:
>> 
>> Why are directory permissions not sufficient to allow/deny non-priveleged
> mounts?
>> I don't understand that contention yet.
>
> The same scenarios laid out previously in this thread.  I.e.
>
> 1. user hallyn does mount --bind / /home/hallyn/root
> 2. (...)
> 3. admin does "deluser hallyn"
>
> and deluser starts wiping out root
>
> Or,
>
> 1. user hallyn does mount --bind / /home/hallyn/root
> 2. backup daemon starts backing up /home/hallyn/root/home/hallyn/root/home...
>
> So we started down the path of forcing users to clone a new namespace
> before doing user mounts, which is what the clone flag was about.  Using
> per-mount flags also suffices as you had pointed out, which is being
> done here.  But directory permissions are inadequate.

Interesting....

So far even today these things can happen, however they are sufficiently
unlikely the tools don't account for them.

Once a hostile user can cause them things are more of a problem.

> (Unless you want to tackle each problem legacy tool one at a time to
> remove problems - i.e. deluser should umount everything under
> /home/hallyn before deleting, backup should be spawned from it's own
> namespace cloned right after boot or just back up on one filesystem,
> etc.)

I don't see a way that backup and deluser won't need to be modified
to work properly in a system where non-priveleged mounts are allowed,
at least they will need to account for /share.

That said it is clearly a hazard if we enable this functionality by
default.

If we setup a pam module that triggers on login and perhaps when
cron and at jobs run to setup an additional mount namespace I think
keeping applications locked away in their own mount namespace is
sufficient to avoid hostile users from doing unexpected things to
the initial mount namespace.  So unless I am mistake it should be
relatively simple to prevent user space from encountering problems.

That still leaves the question of how we handle systems with an old
user space that is insufficiently robust to deal with mounts occurring
at unexpected locations.


  I think a simple sysctl to enable/disable of non-priveleged mounts 
  defaulting to disabled is enough.

Am I correct or will it be more difficult than just a little pam
module to ensure non-trusted users never run in the initial mount
namespace?

Eric

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> Interesting....
> 
> So far even today these things can happen, however they are sufficiently
> unlikely the tools don't account for them.
> 
> Once a hostile user can cause them things are more of a problem.
> 
> > (Unless you want to tackle each problem legacy tool one at a time to
> > remove problems - i.e. deluser should umount everything under
> > /home/hallyn before deleting, backup should be spawned from it's own
> > namespace cloned right after boot or just back up on one filesystem,
> > etc.)
> 
> I don't see a way that backup and deluser won't need to be modified
> to work properly in a system where non-priveleged mounts are allowed,
> at least they will need to account for /share.
> 
> That said it is clearly a hazard if we enable this functionality by
> default.
> 
> If we setup a pam module that triggers on login and perhaps when
> cron and at jobs run to setup an additional mount namespace I think
> keeping applications locked away in their own mount namespace is
> sufficient to avoid hostile users from doing unexpected things to
> the initial mount namespace.  So unless I am mistake it should be
> relatively simple to prevent user space from encountering problems.
> 
> That still leaves the question of how we handle systems with an old
> user space that is insufficiently robust to deal with mounts occurring
> at unexpected locations.
> 
> 
>   I think a simple sysctl to enable/disable of non-priveleged mounts 
>   defaulting to disabled is enough.
> 
> Am I correct or will it be more difficult than just a little pam
> module to ensure non-trusted users never run in the initial mount
> namespace?

I'm still not sure, what your problem is.

With the v3 of the usermounts patchset, by default, user mounts are
disabled, because the "allow unpriv submounts" flag is cleared on all
mounts.

There are several options available to sysadmins and distro builders
to enable user mounts in a secure way:

  - pam module, which creates a private namespace, and sets "allow
    unpriv submounts" on the mounts within the namespace

  - pam module, which rbinds / onto /mnt/ns/$USER, and chroots into
    /mnt/ns/$USER, then sets the "allow unpriv submounts" on the
    mounts under /mnt/ns/$USER.

  - sysadmin creates /mnt/usermounts writable to all users, with
    sticky bit (same as /tmp), does "mount --bind /mnt/usermounts
    /mnt/usermounts" and sets the "allow unpriv submounts" on
    /mnt/usermounts.

All of these are perfectly safe wrt userdel and backup (assuming it
doesn't try back up /mnt).

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Eric W. Biederman]

Miklos Szeredi <miklos@szeredi.hu> writes:

> I'm still not sure, what your problem is.

My problem right now is that I see a serious complexity escalation in
the user interface that we must support indefinitely.

I see us taking a nice powerful concept and seriously watering it down.
To some extent we have to avoid confusing suid applications.  (I would
so love to remove the SUID bit...).

I'm being contrary to ensure we have a good code review.

I have heard it said that there are two kinds of design.  Something
so simple it obviously has no deficiencies.  Something so complex it has
no obvious deficiencies.  I am very much afraid we are slipping the
mount namespace into the latter category of work.  Which is a bad
bad thing for core OS feature.

> With the v3 of the usermounts patchset, by default, user mounts are
> disabled, because the "allow unpriv submounts" flag is cleared on all
> mounts.
>
> There are several options available to sysadmins and distro builders
> to enable user mounts in a secure way:
>
>   - pam module, which creates a private namespace, and sets "allow
>     unpriv submounts" on the mounts within the namespace
>
>   - pam module, which rbinds / onto /mnt/ns/$USER, and chroots into
>     /mnt/ns/$USER, then sets the "allow unpriv submounts" on the
>     mounts under /mnt/ns/$USER.

In part this really disturbs me because we now have two mechanisms for
controlling the scope of what a user can do.

A flag or a new namespace.  Two mechanisms to accomplish the same
thing sound wrong, and hard to manage.

>   - sysadmin creates /mnt/usermounts writable to all users, with
>     sticky bit (same as /tmp), does "mount --bind /mnt/usermounts
>     /mnt/usermounts" and sets the "allow unpriv submounts" on
>     /mnt/usermounts.
>
> All of these are perfectly safe wrt userdel and backup (assuming it
> doesn't try back up /mnt).

I also don't understand at all the user= mount flag and options.
All it seemed to be used for was adding permissions to unmount.  In user
space to deal with the lack of any form of untrusted mounts I can understand
this.  In kernel space this seems to be more of a problem.

Eric

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > I'm still not sure, what your problem is.
> 
> My problem right now is that I see a serious complexity escalation in
> the user interface that we must support indefinitely.
> 
> I see us taking a nice powerful concept and seriously watering it down.
> To some extent we have to avoid confusing suid applications.  (I would
> so love to remove the SUID bit...).
> 
> I'm being contrary to ensure we have a good code review.

OK.  And it's very much appreciated :)

> I have heard it said that there are two kinds of design.  Something
> so simple it obviously has no deficiencies.  Something so complex it has
> no obvious deficiencies.  I am very much afraid we are slipping the
> mount namespace into the latter category of work.  Which is a bad
> bad thing for core OS feature.

I've tried to make this unprivileged mount thing as simple as
possible, and no simpler.  If we can make it even simpler, all the
better.

> > With the v3 of the usermounts patchset, by default, user mounts are
> > disabled, because the "allow unpriv submounts" flag is cleared on all
> > mounts.
> >
> > There are several options available to sysadmins and distro builders
> > to enable user mounts in a secure way:
> >
> >   - pam module, which creates a private namespace, and sets "allow
> >     unpriv submounts" on the mounts within the namespace
> >
> >   - pam module, which rbinds / onto /mnt/ns/$USER, and chroots into
> >     /mnt/ns/$USER, then sets the "allow unpriv submounts" on the
> >     mounts under /mnt/ns/$USER.
> 
> In part this really disturbs me because we now have two mechanisms for
> controlling the scope of what a user can do.

You mean rbind+chroot and clone(CLONE_NS)?  Yes, those are two
different mechanisms achieving very similar results.  But what has
this to do with unprivileged mounts?

> A flag or a new namespace.  Two mechanisms to accomplish the same
> thing sound wrong, and hard to manage.

The flag permitting the unprivileged mounts (which we now agreed to
name "allowusermnt") is used in both cases.

Just creating a new namespace doesn't always imply that you want to
allow user mounts inside, does it?  These are orthogonal features.

> >   - sysadmin creates /mnt/usermounts writable to all users, with
> >     sticky bit (same as /tmp), does "mount --bind /mnt/usermounts
> >     /mnt/usermounts" and sets the "allow unpriv submounts" on
> >     /mnt/usermounts.
> >
> > All of these are perfectly safe wrt userdel and backup (assuming it
> > doesn't try back up /mnt).
> 
> I also don't understand at all the user= mount flag and options.

The "user=UID" or (or MNT_USER flag) serves multiple purposes:

  - help mount(8) move away from /etc/mtab
  - allow unprivileged umounts
  - account user mounts

> All it seemed to be used for was adding permissions to unmount.  In user
> space to deal with the lack of any form of untrusted mounts I can understand
> this.  In kernel space this seems to be more of a problem.

Why is handling unprivileged mounts in kernel different from handling
them in userspace in this respect?

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Eric W. Biederman]

Miklos Szeredi <miklos@szeredi.hu> writes:

>> > I'm still not sure, what your problem is.
>> 
>> My problem right now is that I see a serious complexity escalation in
>> the user interface that we must support indefinitely.
>> 
>> I see us taking a nice powerful concept and seriously watering it down.
>> To some extent we have to avoid confusing suid applications.  (I would
>> so love to remove the SUID bit...).
>> 
>> I'm being contrary to ensure we have a good code review.
>
> OK.  And it's very much appreciated :)
>
>> I have heard it said that there are two kinds of design.  Something
>> so simple it obviously has no deficiencies.  Something so complex it has
>> no obvious deficiencies.  I am very much afraid we are slipping the
>> mount namespace into the latter category of work.  Which is a bad
>> bad thing for core OS feature.
>
> I've tried to make this unprivileged mount thing as simple as
> possible, and no simpler.  If we can make it even simpler, all the
> better.

We are certainly much more complex then the code in plan9 (just
read through it) so I think we have room for improvement.

Just for reference what I saw in plan 9 was:
- No super user checks in it's mount, unmount, or namespace creation paths.
- A flag to deny new mounts but not new bind mounts (for administrative purposes
  the comment said).

Our differences from plan9.
- suid capable binaries. (SUID please go away).
- A history of programs assuming only root could call mount/unmount.

>> In part this really disturbs me because we now have two mechanisms for
>> controlling the scope of what a user can do.
>
> You mean rbind+chroot and clone(CLONE_NS)?  Yes, those are two
> different mechanisms achieving very similar results.  But what has
> this to do with unprivileged mounts?

The practical question is how do we limit what a user can mount and
unmount.


I would contend that at first glance stuffing a user in their own
mount namespace is sufficient, on a system with utilities aware
of the consequences of mount/unmount.

So we may not need a unprivileged mount disable except as a way
to allow an old user space to run a new kernel.

>> A flag or a new namespace.  Two mechanisms to accomplish the same
>> thing sound wrong, and hard to manage.
>
> The flag permitting the unprivileged mounts (which we now agreed to
> name "allowusermnt") is used in both cases.
>
> Just creating a new namespace doesn't always imply that you want to
> allow user mounts inside, does it?  These are orthogonal features.

After user space has been updated we always want to allow unprivileged
mounts. 

If I get pushed I will say that we need to remove suid exec capability
from user space as well.  At which point we don't even need directory
security checks, there is enough benefit there I certainly think
it is worth considering having an entire NOSUID user space.

Removing suid is probably excessive but if it isn't much harder
then sane mount namespace support we should probably consider it.


>> >   - sysadmin creates /mnt/usermounts writable to all users, with
>> >     sticky bit (same as /tmp), does "mount --bind /mnt/usermounts
>> >     /mnt/usermounts" and sets the "allow unpriv submounts" on
>> >     /mnt/usermounts.
>> >
>> > All of these are perfectly safe wrt userdel and backup (assuming it
>> > doesn't try back up /mnt).
>> 
>> I also don't understand at all the user= mount flag and options.
>
> The "user=UID" or (or MNT_USER flag) serves multiple purposes:
>
>   - help mount(8) move away from /etc/mtab
>   - allow unprivileged umounts
>   - account user mounts
>
>> All it seemed to be used for was adding permissions to unmount.  In user
>> space to deal with the lack of any form of untrusted mounts I can understand
>> this.  In kernel space this seems to be more of a problem.
>
> Why is handling unprivileged mounts in kernel different from handling
> them in userspace in this respect?

Ok.  I just looked at what user space is doing.  The difference is that
what user space is doing predates mount namespaces, and was there as
far as I can tell to keep one user from causing problems for
another user.   If we choose to make mount namespaces to be
the unit of granularity we don't need this capability.

All we have to do is deny unmounts that would confuse a suid
executable.  Which mounts are those?

Eric

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > I've tried to make this unprivileged mount thing as simple as
> > possible, and no simpler.  If we can make it even simpler, all the
> > better.
> 
> We are certainly much more complex then the code in plan9 (just
> read through it) so I think we have room for improvement.
> 
> Just for reference what I saw in plan 9 was:
> - No super user checks in it's mount, unmount, or namespace creation paths.
> - A flag to deny new mounts but not new bind mounts (for administrative purposes
>   the comment said).
> 
> Our differences from plan9.
> - suid capable binaries. (SUID please go away).
> - A history of programs assuming only root could call mount/unmount.

I hate suid as well.  _The_ motivation behind this patchset was to get
rid of "fusermount", a suid mount helper for fuse.

But I don't think suid is going away, and definitely not overnight.
Also I don't think we want to require auditing userspace before
enabling user mounts.

If I understand correctly, your proposal is to get rid of MNT_USER and
MNT_ALLOWUSERMNT and allow/deny unprivileged mounts and umounts based
on a boolean sysctl flag and on a check if the target namespace is the
initial namespace or not.  And maybe add some extra checks which
prevent ugliness from happening with suid programs.  Is this correct?

If so, how are we going to make sure this won't break existing
userspace without doing a full audit of all suid programs in every
distro that wants this feature?

Also how are we going to prevent the user from creating millions of
mounts, and using up all the kernel memory for vfsmounts?

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Trond Myklebust]

On Wed, 2007-04-18 at 11:11 +0200, Miklos Szeredi wrote:
> > > I've tried to make this unprivileged mount thing as simple as
> > > possible, and no simpler.  If we can make it even simpler, all the
> > > better.
> > 
> > We are certainly much more complex then the code in plan9 (just
> > read through it) so I think we have room for improvement.
> > 
> > Just for reference what I saw in plan 9 was:
> > - No super user checks in it's mount, unmount, or namespace creation paths.
> > - A flag to deny new mounts but not new bind mounts (for administrative purposes
> >   the comment said).
> > 
> > Our differences from plan9.
> > - suid capable binaries. (SUID please go away).
> > - A history of programs assuming only root could call mount/unmount.
> 
> I hate suid as well.  _The_ motivation behind this patchset was to get
> rid of "fusermount", a suid mount helper for fuse.
> 
> But I don't think suid is going away, and definitely not overnight.
> Also I don't think we want to require auditing userspace before
> enabling user mounts.
> 
> If I understand correctly, your proposal is to get rid of MNT_USER and
> MNT_ALLOWUSERMNT and allow/deny unprivileged mounts and umounts based
> on a boolean sysctl flag and on a check if the target namespace is the
> initial namespace or not.  And maybe add some extra checks which
> prevent ugliness from happening with suid programs.  Is this correct?
> 
> If so, how are we going to make sure this won't break existing
> userspace without doing a full audit of all suid programs in every
> distro that wants this feature?
> 
> Also how are we going to prevent the user from creating millions of
> mounts, and using up all the kernel memory for vfsmounts?

Don't forget that almost all mount flags are per-superblock. How are you
planning on dealing with the case that one user mounts a filesystem
read-only, while another is trying to mount the same one read-write?

Trond

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > > > I've tried to make this unprivileged mount thing as simple as
> > > > possible, and no simpler.  If we can make it even simpler, all the
> > > > better.
> > > 
> > > We are certainly much more complex then the code in plan9 (just
> > > read through it) so I think we have room for improvement.
> > > 
> > > Just for reference what I saw in plan 9 was:
> > > - No super user checks in it's mount, unmount, or namespace creation paths.
> > > - A flag to deny new mounts but not new bind mounts (for administrative purposes
> > >   the comment said).
> > > 
> > > Our differences from plan9.
> > > - suid capable binaries. (SUID please go away).
> > > - A history of programs assuming only root could call mount/unmount.
> > 
> > I hate suid as well.  _The_ motivation behind this patchset was to get
> > rid of "fusermount", a suid mount helper for fuse.
> > 
> > But I don't think suid is going away, and definitely not overnight.
> > Also I don't think we want to require auditing userspace before
> > enabling user mounts.
> > 
> > If I understand correctly, your proposal is to get rid of MNT_USER and
> > MNT_ALLOWUSERMNT and allow/deny unprivileged mounts and umounts based
> > on a boolean sysctl flag and on a check if the target namespace is the
> > initial namespace or not.  And maybe add some extra checks which
> > prevent ugliness from happening with suid programs.  Is this correct?
> > 
> > If so, how are we going to make sure this won't break existing
> > userspace without doing a full audit of all suid programs in every
> > distro that wants this feature?
> > 
> > Also how are we going to prevent the user from creating millions of
> > mounts, and using up all the kernel memory for vfsmounts?
> 
> Don't forget that almost all mount flags are per-superblock. How are you
> planning on dealing with the case that one user mounts a filesystem
> read-only, while another is trying to mount the same one read-write?

Yeah, I forgot, the per-mount read-only patches are not yet in.

That doesn't really change my agrument though.  _If_ the flag is per
mount, then it makes sense to be able to change it on a master and not
on a slave.  If mount flags are propagated, this is not possible.

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Trond Myklebust]

On Wed, 2007-04-18 at 16:03 +0200, Miklos Szeredi wrote:
> > Don't forget that almost all mount flags are per-superblock. How are you
> > planning on dealing with the case that one user mounts a filesystem
> > read-only, while another is trying to mount the same one read-write?
> 
> Yeah, I forgot, the per-mount read-only patches are not yet in.
> 
> That doesn't really change my agrument though.  _If_ the flag is per
> mount, then it makes sense to be able to change it on a master and not
> on a slave.  If mount flags are propagated, this is not possible.

Read-only isn't the only issue. On something like NFS, there are flags
to set the security flavour, turn on/off encryption etc.

If I mount your home directory using no encryption in my namespace, for
instance, then neither you nor the administrator will be able to turn it
on afterwards in yours without first unmounting it from mine so that the
superblock is destroyed.

Cheers
  Trond

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Christoph Hellwig]

On Wed, Apr 18, 2007 at 10:26:29AM -0400, Trond Myklebust wrote:
> > That doesn't really change my agrument though.  _If_ the flag is per
> > mount, then it makes sense to be able to change it on a master and not
> > on a slave.  If mount flags are propagated, this is not possible.
> 
> Read-only isn't the only issue. On something like NFS, there are flags
> to set the security flavour, turn on/off encryption etc.
> 
> If I mount your home directory using no encryption in my namespace, for
> instance, then neither you nor the administrator will be able to turn it
> on afterwards in yours without first unmounting it from mine so that the
> superblock is destroyed.

I suspect the right answer here is to make nfs mount handling smarter.
The way mounting works the filesystem is allowed to choose whether it
can re-used a superblock or needs a new one.  In the NFS case we probably
want to allow multiple superblocks for the same export if important
paramaters like the security model mismatch.

This is however only the technical part of the answer.  There's a more
important one, and that's the user experience - we need a much better
way to document which flags are per-superblock and which are per-mount.

Due to this and some other issues we'll probably need a new mount API
in some time.

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Trond Myklebust]

On Wed, 2007-04-18 at 16:01 +0100, Christoph Hellwig wrote:
> I suspect the right answer here is to make nfs mount handling smarter.
> The way mounting works the filesystem is allowed to choose whether it
> can re-used a superblock or needs a new one.  In the NFS case we probably
> want to allow multiple superblocks for the same export if important
> paramaters like the security model mismatch.

It might also be another application for layering. If I were able to
share enough of the inode and dentry information between superblocks,
then all sorts of interesting possibilities arise...

Cheers
  Trond

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > > Don't forget that almost all mount flags are per-superblock. How are you
> > > planning on dealing with the case that one user mounts a filesystem
> > > read-only, while another is trying to mount the same one read-write?
> > 
> > Yeah, I forgot, the per-mount read-only patches are not yet in.
> > 
> > That doesn't really change my agrument though.  _If_ the flag is per
> > mount, then it makes sense to be able to change it on a master and not
> > on a slave.  If mount flags are propagated, this is not possible.
> 
> Read-only isn't the only issue. On something like NFS, there are flags
> to set the security flavour, turn on/off encryption etc.
> 
> If I mount your home directory using no encryption in my namespace, for
> instance, then neither you nor the administrator will be able to turn it
> on afterwards in yours without first unmounting it from mine so that the
> superblock is destroyed.

OK, that's interesting, but I fail to grasp the relevance of this to
unprivileged mounts.

Or are you thinking of unprivileged NFS mounts?  Well, think again,
because that involves _much_ more than it seems at first glance.

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Eric W. Biederman]

Miklos Szeredi <miklos@szeredi.hu> writes:

>> > I've tried to make this unprivileged mount thing as simple as
>> > possible, and no simpler.  If we can make it even simpler, all the
>> > better.
>> 
>> We are certainly much more complex then the code in plan9 (just
>> read through it) so I think we have room for improvement.
>> 
>> Just for reference what I saw in plan 9 was:
>> - No super user checks in it's mount, unmount, or namespace creation paths.
>> - A flag to deny new mounts but not new bind mounts (for administrative
> purposes
>>   the comment said).
>> 
>> Our differences from plan9.
>> - suid capable binaries. (SUID please go away).
>> - A history of programs assuming only root could call mount/unmount.
>
> I hate suid as well.  _The_ motivation behind this patchset was to get
> rid of "fusermount", a suid mount helper for fuse.
>
> But I don't think suid is going away, and definitely not overnight.

Agreed, unless it just happens that killing is equally as hard as
doing non-privileged mounts.   Which I really don't think will be
the case.  suid executables can always be replaced by a non-privileged
part that connect to a daemon on localhost.

> Also I don't think we want to require auditing userspace before
> enabling user mounts.

Given the complexity of the code to avoids audits adds, and especially
the uncertainty of how all of the pieces add up together I really
think we do need to audit user space (but only in a limited way).

This is a paradigm shift in how mounts are managed and to get
the full benefit of it we need to be prepared to deal with the
paradigm shift.

Essentially what the audit must ensure is that 
(a) non-root users are always run in a non-default namespace so
    mounts and unmounts they generate will not have global effect.
(b) If we setup something like the proposed /share that administrative
    tools know how to treat it properly.

That is not an especially hard audit of user space and it noticeably
reduces the complexity of what we must implement.

> If I understand correctly, your proposal is to get rid of MNT_USER and
> MNT_ALLOWUSERMNT and allow/deny unprivileged mounts and umounts based
> on a boolean sysctl flag and on a check if the target namespace is the
> initial namespace or not.  

No.  I really do not want to treat the initial namespace in a special way
from an implementation point of view.

>From a distro point of view I don't think we should ever allow a user
into the initial mount namespace, and that is what we should audit.
All of the ways a user can login to a machine.

We need to audit the login paths anyway if we are going to do anything
interesting with the mount namespace.  So I see this as no real
additional overhead to make things usable.

> And maybe add some extra checks which
> prevent ugliness from happening with suid programs.  Is this correct?

Definitely add some extra permission checks to prevent ugliness from
happening with suid executables.  In the general case the problem
with suid executables is that they expect parts of the mount namespace
that a user cannot modify not to be modified.

Ensuring that our permission checks keep that promise for mount
and umount is going to be a bit challenging, because we need to
think through a lot of scenarios.  But it is not that hard.

> If so, how are we going to make sure this won't break existing
> userspace without doing a full audit of all suid programs in every
> distro that wants this feature?

The permission checks to ensure you can not modify a filesystem with
mounts in a way that you could not modify it by creating or deleting
files should be enough.

That probably means that we are restricted to mounting filesystems
with the equivalent of uid=$(id -u) gid=$(id -g).  That is if you
aren't root all files in the filesystem you cause to be mounted
must be owned by you.  That way you have permission to unmount or
delete them.

At the very least the user who causes a mount to be created should
be the owner and have complete control over the directory mount point.
So that they can at least theoretically remove all of the files and
directories at the mount point (which would be equivalent to
unmounting it).

> Also how are we going to prevent the user from creating millions of
> mounts, and using up all the kernel memory for vfsmounts?

I definitely agree that we need some kind of accounting.  I'm don't
think we can do this per user (which would be nice) and I don't
believe your code does attempts to limit mounts by user either.
A simple global limit on the number of mounts should be sufficient.
If necessary we can allow the limit to be ignored if you have
the appropriate capability.

Eric

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> >> > I've tried to make this unprivileged mount thing as simple as
> >> > possible, and no simpler.  If we can make it even simpler, all the
> >> > better.
> >> 
> >> We are certainly much more complex then the code in plan9 (just
> >> read through it) so I think we have room for improvement.
> >> 
> >> Just for reference what I saw in plan 9 was:
> >> - No super user checks in it's mount, unmount, or namespace creation paths.
> >> - A flag to deny new mounts but not new bind mounts (for administrative
> > purposes
> >>   the comment said).
> >> 
> >> Our differences from plan9.
> >> - suid capable binaries. (SUID please go away).
> >> - A history of programs assuming only root could call mount/unmount.
> >
> > I hate suid as well.  _The_ motivation behind this patchset was to get
> > rid of "fusermount", a suid mount helper for fuse.
> >
> > But I don't think suid is going away, and definitely not overnight.
> 
> Agreed, unless it just happens that killing is equally as hard as
> doing non-privileged mounts.   Which I really don't think will be
> the case.  suid executables can always be replaced by a non-privileged
> part that connect to a daemon on localhost.
> 
> > Also I don't think we want to require auditing userspace before
> > enabling user mounts.
> 
> Given the complexity of the code to avoids audits adds, and especially
> the uncertainty of how all of the pieces add up together I really
> think we do need to audit user space (but only in a limited way).
> 
> This is a paradigm shift in how mounts are managed and to get
> the full benefit of it we need to be prepared to deal with the
> paradigm shift.
> 
> Essentially what the audit must ensure is that 
> (a) non-root users are always run in a non-default namespace so
>     mounts and unmounts they generate will not have global effect.

But unprivileged proceses are not only created through login.  What
happens, when some process does setuid(NONZERO).  With your proposal
it now gained the privilege of mounting in the initial namespace.  IOW
every program calling setuid() now has to be audited for any problems
this could cause.

> (b) If we setup something like the proposed /share that administrative
>     tools know how to treat it properly.
> 
> That is not an especially hard audit of user space and it noticeably
> reduces the complexity of what we must implement.
> 
> > If I understand correctly, your proposal is to get rid of MNT_USER and
> > MNT_ALLOWUSERMNT and allow/deny unprivileged mounts and umounts based
> > on a boolean sysctl flag and on a check if the target namespace is the
> > initial namespace or not.  
> 
> No.  I really do not want to treat the initial namespace in a special way
> from an implementation point of view.

Ah.

> >From a distro point of view I don't think we should ever allow a user
> into the initial mount namespace, and that is what we should audit.
> All of the ways a user can login to a machine.
> 
> We need to audit the login paths anyway if we are going to do anything
> interesting with the mount namespace.  So I see this as no real
> additional overhead to make things usable.
> 
> > And maybe add some extra checks which
> > prevent ugliness from happening with suid programs.  Is this correct?
> 
> Definitely add some extra permission checks to prevent ugliness from
> happening with suid executables.  In the general case the problem
> with suid executables is that they expect parts of the mount namespace
> that a user cannot modify not to be modified.
> 
> Ensuring that our permission checks keep that promise for mount
> and umount is going to be a bit challenging, because we need to
> think through a lot of scenarios.  But it is not that hard.
> 
> > If so, how are we going to make sure this won't break existing
> > userspace without doing a full audit of all suid programs in every
> > distro that wants this feature?
> 
> The permission checks to ensure you can not modify a filesystem with
> mounts in a way that you could not modify it by creating or deleting
> files should be enough.

Umm, well.  What if user mounts a filesystem read-only.  Then wants to
unmount, but hey, it's read-only, no permission to write, no
unmount...

That's just a stupid example, but it shows, that file permissions are
not always useful for determining what you can mount, and especially
what you can unmount.

> That probably means that we are restricted to mounting filesystems
> with the equivalent of uid=$(id -u) gid=$(id -g).  That is if you
> aren't root all files in the filesystem you cause to be mounted
> must be owned by you.  That way you have permission to unmount or
> delete them.

That rules out unprivileged bind mounts.

> At the very least the user who causes a mount to be created should
> be the owner and have complete control over the directory mount point.
> So that they can at least theoretically remove all of the files and
> directories at the mount point (which would be equivalent to
> unmounting it).

Checking the permissions on the mountpoint to allow unmounting is

  - rather inelegant: user can't see those permissions, can only
    determine if umount is allowed by trial and error

  - may be a security hole, e.g.:

      sysadmin:

          mkdir -m 777 /mnt/disk
          mount /dev/hda2 /mnt/disk

Of course the user doesn't have the right to delete the contents of
the mount, yet the permissions on the mountpoint would imply that s/he
has permission to umount the disk.

> > Also how are we going to prevent the user from creating millions of
> > mounts, and using up all the kernel memory for vfsmounts?
> 
> I definitely agree that we need some kind of accounting.  I'm don't
> think we can do this per user (which would be nice) and I don't
> believe your code does attempts to limit mounts by user either.
> A simple global limit on the number of mounts should be sufficient.
> If necessary we can allow the limit to be ignored if you have
> the appropriate capability.

Yup, that would work.  Accounting only unprivileged mounts may be more
intuitive though:

  http://lkml.org/lkml/2005/5/11/38

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> Checking the permissions on the mountpoint to allow unmounting is
> 
>   - rather inelegant: user can't see those permissions, can only
>     determine if umount is allowed by trial and error
> 
>   - may be a security hole, e.g.:
> 
>       sysadmin:
> 
>           mkdir -m 777 /mnt/disk
>           mount /dev/hda2 /mnt/disk
> 
> Of course the user doesn't have the right to delete the contents of
> the mount, yet the permissions on the mountpoint would imply that s/he
> has permission to umount the disk.

It is becoming increasingly apparent, that mount/umount permission
based on file permissions is inherently broken:

1) there are user-writable files under /proc/$PID/, which definitely
   shouldn't be allowed to be overmounted

2) if user mounts an fs read-only, then wants to create a submount of
   this, it will fail with the current patchset

Solving 2) should be trivial: submounting a mount owned by the user
should be always allowed regardless of the file permissions.

Maybe this could be generaized to say, that a mount can be submounted
by an unprivileged user IFF parent mount is owned by said user.

This would get rid of some of the complications in the current
patchset, namely the functionality of MNT_ALLOWUSERMNT and MNT_USER
flags would be merged, and the permission checking would be removed.

For example on login, the user could get a private namespace set up
some that the home directory is owned by the user, and hence can be
freely submounted:

  clone(CLONE_NEWNS)
  mount --bind /home/$USER /home/$USER
  mount --remount -ouser=$USER /home/$USER

This is of course more limiting than allowing mounts based on file
permissions, but it's also a lot cleaner.

Hmm?

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> >> 
> >> Why are directory permissions not sufficient to allow/deny non-priveleged
> > mounts?
> >> I don't understand that contention yet.
> >
> > The same scenarios laid out previously in this thread.  I.e.
> >
> > 1. user hallyn does mount --bind / /home/hallyn/root
> > 2. (...)
> > 3. admin does "deluser hallyn"
> >
> > and deluser starts wiping out root
> >
> > Or,
> >
> > 1. user hallyn does mount --bind / /home/hallyn/root
> > 2. backup daemon starts backing up /home/hallyn/root/home/hallyn/root/home...
> >
> > So we started down the path of forcing users to clone a new namespace
> > before doing user mounts, which is what the clone flag was about.  Using
> > per-mount flags also suffices as you had pointed out, which is being
> > done here.  But directory permissions are inadequate.
> 
> Interesting....
> 
> So far even today these things can happen, however they are sufficiently
> unlikely the tools don't account for them.
> 
> Once a hostile user can cause them things are more of a problem.
> 
> > (Unless you want to tackle each problem legacy tool one at a time to
> > remove problems - i.e. deluser should umount everything under
> > /home/hallyn before deleting, backup should be spawned from it's own
> > namespace cloned right after boot or just back up on one filesystem,
> > etc.)
> 
> I don't see a way that backup and deluser won't need to be modified
> to work properly in a system where non-priveleged mounts are allowed,
> at least they will need to account for /share.

Yes, all the tools need to avoid /share.  Though at least it's a single
location we can avoid, and it is purely a system configuration issue,
whereas fixing deluser to watch for user mounts under /home involves (I
assume) rewriting a part of it.

> That said it is clearly a hazard if we enable this functionality by
> default.
> 
> If we setup a pam module that triggers on login and perhaps when
> cron and at jobs run to setup an additional mount namespace I think
> keeping applications locked away in their own mount namespace is
> sufficient to avoid hostile users from doing unexpected things to
> the initial mount namespace.  So unless I am mistake it should be
> relatively simple to prevent user space from encountering problems.
> 
> That still leaves the question of how we handle systems with an old
> user space that is insufficiently robust to deal with mounts occurring
> at unexpected locations.
> 
> 
>   I think a simple sysctl to enable/disable of non-priveleged mounts 
>   defaulting to disabled is enough.
> 
> Am I correct or will it be more difficult than just a little pam
> module to ensure non-trusted users never run in the initial mount
> namespace?

The danger with relying on the pam module is that you have to plug it in
all the right places.  For instance, if we're talking about malicious
users, now we have to start worrying about an ftp daemon with user login
that isn't using pam, and happens to have an exploitable bug.

So it seems to me the per-mount flag you suggested really is the best
solution.  Now the pam module is still needed, but only to set things up
so that the user *can* do user mounts.  If there's a way to login
bypassing the pam module, then the user simply won't be able to do user
mounts anywhere but under /share, and as Miklos suggested the perms on
share can probably be set to 000.

-serge

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> >> 
> >> Why are directory permissions not sufficient to allow/deny non-priveleged
> > mounts?
> >> I don't understand that contention yet.
> >
> > The same scenarios laid out previously in this thread.  I.e.
> >
> > 1. user hallyn does mount --bind / /home/hallyn/root
> > 2. (...)
> > 3. admin does "deluser hallyn"
> >
> > and deluser starts wiping out root
> >
> > Or,
> >
> > 1. user hallyn does mount --bind / /home/hallyn/root
> > 2. backup daemon starts backing up /home/hallyn/root/home/hallyn/root/home...
> >
> > So we started down the path of forcing users to clone a new namespace
> > before doing user mounts, which is what the clone flag was about.  Using
> > per-mount flags also suffices as you had pointed out, which is being
> > done here.  But directory permissions are inadequate.
> 
> Interesting....
> 
> So far even today these things can happen, however they are sufficiently
> unlikely the tools don't account for them.
> 
> Once a hostile user can cause them things are more of a problem.
> 
> > (Unless you want to tackle each problem legacy tool one at a time to
> > remove problems - i.e. deluser should umount everything under
> > /home/hallyn before deleting, backup should be spawned from it's own
> > namespace cloned right after boot or just back up on one filesystem,
> > etc.)
> 
> I don't see a way that backup and deluser won't need to be modified
> to work properly in a system where non-priveleged mounts are allowed,
> at least they will need to account for /share.
> 
> That said it is clearly a hazard if we enable this functionality by
> default.
> 
> If we setup a pam module that triggers on login and perhaps when
> cron and at jobs run to setup an additional mount namespace I think
> keeping applications locked away in their own mount namespace is
> sufficient to avoid hostile users from doing unexpected things to
> the initial mount namespace.  So unless I am mistake it should be
> relatively simple to prevent user space from encountering problems.
> 
> That still leaves the question of how we handle systems with an old
> user space that is insufficiently robust to deal with mounts occurring
> at unexpected locations.
> 
> 
>   I think a simple sysctl to enable/disable of non-priveleged mounts 
>   defaulting to disabled is enough.

There is a sysctl for max_user_mounts which can be set to 0.

So a simple on/off sysctl is unnecessary, but given that admins might
wonder whether 0 means infinite :), and I agree on/off is important, a
second one wouldn't hurt.

> Am I correct or will it be more difficult than just a little pam
> module to ensure non-trusted users never run in the initial mount
> namespace?
> 
> Eric

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Ram Pai]

On Mon, 2007-04-16 at 11:56 +0200, Miklos Szeredi wrote:
> > > > Also for bind-mount and remount operations the flag has to be propagated
> > > > down its propagation tree.  Otherwise a unpriviledged mount in a shared
> > > > mount wont get reflected in its peers and slaves, leading to unidentical
> > > > shared-subtrees.
> > > 
> > > That's an interesting question.  Do we want shared mounts to be
> > > totally identical, including mnt_flags?  It doesn't look as if
> > > do_remount() guarantees that currently.
> > 
> > Depends on the semantics of each of the flags. Some flags like of the
> > read/write flag, would not interfere with the propagation semantics
> > AFAICT.  But this one certainly seems to interfere.
> 
> That depends.  Current patches check the "unprivileged submounts
> allowed under this mount" flag only on the requested mount and not on
> the propagated mounts.  Do you see a problem with this?

Don't see a problem if the flag is propagated to all peers and slave
mounts. 

If not, I see a problem. What if the propagated mount has its flag set
to not do un-priviledged mounts, whereas the requested mount has it
allowed?

RP



> 
> Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > > > > Also for bind-mount and remount operations the flag has to be propagated
> > > > > down its propagation tree.  Otherwise a unpriviledged mount in a shared
> > > > > mount wont get reflected in its peers and slaves, leading to unidentical
> > > > > shared-subtrees.
> > > > 
> > > > That's an interesting question.  Do we want shared mounts to be
> > > > totally identical, including mnt_flags?  It doesn't look as if
> > > > do_remount() guarantees that currently.
> > > 
> > > Depends on the semantics of each of the flags. Some flags like of the
> > > read/write flag, would not interfere with the propagation semantics
> > > AFAICT.  But this one certainly seems to interfere.
> > 
> > That depends.  Current patches check the "unprivileged submounts
> > allowed under this mount" flag only on the requested mount and not on
> > the propagated mounts.  Do you see a problem with this?
> 
> Don't see a problem if the flag is propagated to all peers and slave
> mounts. 
> 
> If not, I see a problem. What if the propagated mount has its flag set
> to not do un-priviledged mounts, whereas the requested mount has it
> allowed?

Then the mount is allowed.

It is up to the sysadmin/distro to design set up the propagations in a
way that this is not a problem.

I think it would be much less clear conceptually, if unprivileged
mounting would have to check propagations as well.

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Serge E. Hallyn]

Quoting Miklos Szeredi (miklos@szeredi.hu):
> > > > > > Also for bind-mount and remount operations the flag has to be propagated
> > > > > > down its propagation tree.  Otherwise a unpriviledged mount in a shared
> > > > > > mount wont get reflected in its peers and slaves, leading to unidentical
> > > > > > shared-subtrees.
> > > > > 
> > > > > That's an interesting question.  Do we want shared mounts to be
> > > > > totally identical, including mnt_flags?  It doesn't look as if
> > > > > do_remount() guarantees that currently.
> > > > 
> > > > Depends on the semantics of each of the flags. Some flags like of the
> > > > read/write flag, would not interfere with the propagation semantics
> > > > AFAICT.  But this one certainly seems to interfere.
> > > 
> > > That depends.  Current patches check the "unprivileged submounts
> > > allowed under this mount" flag only on the requested mount and not on
> > > the propagated mounts.  Do you see a problem with this?
> > 
> > Don't see a problem if the flag is propagated to all peers and slave
> > mounts. 
> > 
> > If not, I see a problem. What if the propagated mount has its flag set
> > to not do un-priviledged mounts, whereas the requested mount has it
> > allowed?
> 
> Then the mount is allowed.
> 
> It is up to the sysadmin/distro to design set up the propagations in a
> way that this is not a problem.
> 
> I think it would be much less clear conceptually, if unprivileged
> mounting would have to check propagations as well.
> 
> Miklos

I'm a bit lost about what is currently done and who advocates for what.

It seems to me the MNT_ALLOWUSERMNT (or whatever :) flag should be
propagated.  In the /share rbind+chroot example, I assume the admin
would start by doing

	mount --bind /share /share
	mount --make-slave /share
	mount --bind -o allow_user_mounts /share (or whatever)
	mount --make-shared /share

then on login, pam does

	chroot /share/$USER

or some sort of

	mount --bind /share /home/$USER/root
	chroot /home/$USER/root

or whatever.  In any case, the user cannot make user mounts except under
/share, and any cloned namespaces will still allow user mounts.

Or are you guys talking about something else?

-serge

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> I'm a bit lost about what is currently done and who advocates for what.
> 
> It seems to me the MNT_ALLOWUSERMNT (or whatever :) flag should be
> propagated.  In the /share rbind+chroot example, I assume the admin
> would start by doing
> 
> 	mount --bind /share /share
> 	mount --make-slave /share
> 	mount --bind -o allow_user_mounts /share (or whatever)
> 	mount --make-shared /share
> 
> then on login, pam does
> 
> 	chroot /share/$USER
> 
> or some sort of
> 
> 	mount --bind /share /home/$USER/root
> 	chroot /home/$USER/root
> 
> or whatever.  In any case, the user cannot make user mounts except under
> /share, and any cloned namespaces will still allow user mounts.

I don't quite understand your method.  This is how I think of it:

mount --make-rshared /
mkdir -p /mnt/ns/$USER
mount --rbind / /mnt/ns/$USER
mount --make-rslave /mnt/ns/$USER
mount --set-flags --recursive -oallowusermnt /mnt/ns/$USER
chroot /mnt/ns/$USER
su - $USER

I did actually try something equivalent (without the fancy mount
commands though), and it worked fine.  The only "problem" is the
proliferation of mounts in /proc/mounts.  There was a recently posted
patch in AppArmor, that at least hides unreachable mounts from
/proc/mounts, so the user wouldn't see all those.  But it could still
be pretty confusing to the sysadmin.

So in that sense doing it the complicated way, by first cloning the
namespace, and then copying and sharing mounts individually which need
to be shared could relieve this somewhat.

Another point: user mounts under /proc and /sys shouldn't be allowed.
There are files there (at least in /proc) that are seemingly writable
by the user, but they are still not writable in the sense, that
"normal" files are.

Anyway, there are lots of userspace policy issues, but those don't
impact the kernel part.

As for the original question of propagating the "allowusermnt" flag, I
think it doesn't matter, as long as it's consistent and documented.

Propagating some mount flags and not propagating others is
inconsistent and confusing, so I wouldn't want that.  Currently
remount doesn't propagate mount flags, that may be a bug, dunno.

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Serge E. Hallyn]

Quoting Miklos Szeredi (miklos@szeredi.hu):
> > I'm a bit lost about what is currently done and who advocates for what.
> > 
> > It seems to me the MNT_ALLOWUSERMNT (or whatever :) flag should be
> > propagated.  In the /share rbind+chroot example, I assume the admin
> > would start by doing
> > 
> > 	mount --bind /share /share
> > 	mount --make-slave /share
> > 	mount --bind -o allow_user_mounts /share (or whatever)
> > 	mount --make-shared /share
> > 
> > then on login, pam does
> > 
> > 	chroot /share/$USER
> > 
> > or some sort of
> > 
> > 	mount --bind /share /home/$USER/root
> > 	chroot /home/$USER/root
> > 
> > or whatever.  In any case, the user cannot make user mounts except under
> > /share, and any cloned namespaces will still allow user mounts.
> 
> I don't quite understand your method.  This is how I think of it:
> 
> mount --make-rshared /
> mkdir -p /mnt/ns/$USER
> mount --rbind / /mnt/ns/$USER
> mount --make-rslave /mnt/ns/$USER

This was my main point - that the tree in which users can mount will be
a slave of /, so that propagating the "are user mounts allowed" flag
among peers is safe and intuitive.

> mount --set-flags --recursive -oallowusermnt /mnt/ns/$USER
> chroot /mnt/ns/$USER
> su - $USER
> 
> I did actually try something equivalent (without the fancy mount
> commands though), and it worked fine.  The only "problem" is the
> proliferation of mounts in /proc/mounts.  There was a recently posted
> patch in AppArmor, that at least hides unreachable mounts from
> /proc/mounts, so the user wouldn't see all those.  But it could still
> be pretty confusing to the sysadmin.
> 
> So in that sense doing it the complicated way, by first cloning the
> namespace, and then copying and sharing mounts individually which need
> to be shared could relieve this somewhat.

True.  But the kernel functionality you provide enables both ways so no
problem in either case :)

> Another point: user mounts under /proc and /sys shouldn't be allowed.
> There are files there (at least in /proc) that are seemingly writable
> by the user, but they are still not writable in the sense, that
> "normal" files are.

Good point.

> Anyway, there are lots of userspace policy issues, but those don't
> impact the kernel part.

Though it might make sense to enforce /proc and /sys not allowing user
mounts under them in the kernel.

> As for the original question of propagating the "allowusermnt" flag, I
> think it doesn't matter, as long as it's consistent and documented.
> 
> Propagating some mount flags and not propagating others is
> inconsistent and confusing, so I wouldn't want that.  Currently
> remount doesn't propagate mount flags, that may be a bug, dunno.

Dave, any thoughts on safety of propagating the vfsmount read-only
flags?

-serge

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > mount --make-rshared /
> > mkdir -p /mnt/ns/$USER
> > mount --rbind / /mnt/ns/$USER
> > mount --make-rslave /mnt/ns/$USER
> 
> This was my main point - that the tree in which users can mount will be
> a slave of /, so that propagating the "are user mounts allowed" flag
> among peers is safe and intuitive.

I think it's equally intuitive if flags are not propagated.  After
all, I may want to set the mount flag _only_ on this particular mount,
and not anything else.

This is something I wouln't be sure about either way without
consulting the man.

> True.  But the kernel functionality you provide enables both ways so no
> problem in either case :)
> 
> > Another point: user mounts under /proc and /sys shouldn't be allowed.
> > There are files there (at least in /proc) that are seemingly writable
> > by the user, but they are still not writable in the sense, that
> > "normal" files are.
> 
> Good point.
> 
> > Anyway, there are lots of userspace policy issues, but those don't
> > impact the kernel part.
> 
> Though it might make sense to enforce /proc and /sys not allowing user
> mounts under them in the kernel.

I think not, it would just be another policy decision having to be
made in the kernel on an fs by fs basis, and getting it wrong would be
much worse than getting it wrong in userspace.

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Ram Pai]

On Tue, 2007-04-17 at 19:44 +0200, Miklos Szeredi wrote:
> > I'm a bit lost about what is currently done and who advocates for what.
> > 
> > It seems to me the MNT_ALLOWUSERMNT (or whatever :) flag should be
> > propagated.  In the /share rbind+chroot example, I assume the admin
> > would start by doing
> > 
> > 	mount --bind /share /share
> > 	mount --make-slave /share
> > 	mount --bind -o allow_user_mounts /share (or whatever)
> > 	mount --make-shared /share
> > 
> > then on login, pam does
> > 
> > 	chroot /share/$USER
> > 
> > or some sort of
> > 
> > 	mount --bind /share /home/$USER/root
> > 	chroot /home/$USER/root
> > 
> > or whatever.  In any case, the user cannot make user mounts except under
> > /share, and any cloned namespaces will still allow user mounts.
> 
> I don't quite understand your method.  This is how I think of it:
> 
> mount --make-rshared /
> mkdir -p /mnt/ns/$USER
> mount --rbind / /mnt/ns/$USER
> mount --make-rslave /mnt/ns/$USER
> mount --set-flags --recursive -oallowusermnt /mnt/ns/$USER
> chroot /mnt/ns/$USER
> su - $USER
> 
> I did actually try something equivalent (without the fancy mount
> commands though), and it worked fine.  The only "problem" is the
> proliferation of mounts in /proc/mounts.  There was a recently posted
> patch in AppArmor, that at least hides unreachable mounts from
> /proc/mounts, so the user wouldn't see all those.  But it could still
> be pretty confusing to the sysadmin.

unbindable mounts were designed to overcome the proliferation problem.

Your steps should be something like this:

mount --make-rshared /
mkdir -p /mnt/ns
mount --bind /mnt/ns /mnt/ns
mount --make-unbindable /mnt/ns
mkdir -p /mnt/ns/$USER
mount --rbind / /mnt/ns/$USER
mount --make-rslave /mnt/ns/$USER
mount --set-flags --recursive -oallowusermnt /mnt/ns/$USER
chroot /mnt/ns/$USER
su - $USER

try this and your proliferation problem will disappear. :-)

> 
> So in that sense doing it the complicated way, by first cloning the
> namespace, and then copying and sharing mounts individually which need
> to be shared could relieve this somewhat.

the unbindable mount will just provide you permanent relief.

> 
> Another point: user mounts under /proc and /sys shouldn't be allowed.
> There are files there (at least in /proc) that are seemingly writable
> by the user, but they are still not writable in the sense, that
> "normal" files are.
> 
> Anyway, there are lots of userspace policy issues, but those don't
> impact the kernel part.
> 
> As for the original question of propagating the "allowusermnt" flag, I
> think it doesn't matter, as long as it's consistent and documented.
> 
> Propagating some mount flags and not propagating others is
> inconsistent and confusing, so I wouldn't want that.  Currently
> remount doesn't propagate mount flags, that may be a bug, 

For consistency reason, one can propagate all the flags. But
propagating only those flags that interfere with shared-subtree
semantics should suffice.

wait...Dave's read-only bind mounts infact need the ability to
selectively make some mounts readonly. In such cases propagating
the read-only flag will just step on Dave's feature. Wont' it?

RP



> 
> Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > > I'm a bit lost about what is currently done and who advocates for what.
> > > 
> > > It seems to me the MNT_ALLOWUSERMNT (or whatever :) flag should be
> > > propagated.  In the /share rbind+chroot example, I assume the admin
> > > would start by doing
> > > 
> > > 	mount --bind /share /share
> > > 	mount --make-slave /share
> > > 	mount --bind -o allow_user_mounts /share (or whatever)
> > > 	mount --make-shared /share
> > > 
> > > then on login, pam does
> > > 
> > > 	chroot /share/$USER
> > > 
> > > or some sort of
> > > 
> > > 	mount --bind /share /home/$USER/root
> > > 	chroot /home/$USER/root
> > > 
> > > or whatever.  In any case, the user cannot make user mounts except under
> > > /share, and any cloned namespaces will still allow user mounts.
> > 
> > I don't quite understand your method.  This is how I think of it:
> > 
> > mount --make-rshared /
> > mkdir -p /mnt/ns/$USER
> > mount --rbind / /mnt/ns/$USER
> > mount --make-rslave /mnt/ns/$USER
> > mount --set-flags --recursive -oallowusermnt /mnt/ns/$USER
> > chroot /mnt/ns/$USER
> > su - $USER
> > 
> > I did actually try something equivalent (without the fancy mount
> > commands though), and it worked fine.  The only "problem" is the
> > proliferation of mounts in /proc/mounts.  There was a recently posted
> > patch in AppArmor, that at least hides unreachable mounts from
> > /proc/mounts, so the user wouldn't see all those.  But it could still
> > be pretty confusing to the sysadmin.
> 
> unbindable mounts were designed to overcome the proliferation problem.
> 
> Your steps should be something like this:
> 
> mount --make-rshared /
> mkdir -p /mnt/ns
> mount --bind /mnt/ns /mnt/ns
> mount --make-unbindable /mnt/ns
> mkdir -p /mnt/ns/$USER
> mount --rbind / /mnt/ns/$USER
> mount --make-rslave /mnt/ns/$USER
> mount --set-flags --recursive -oallowusermnt /mnt/ns/$USER
> chroot /mnt/ns/$USER
> su - $USER
> 
> try this and your proliferation problem will disappear. :-)

Right, this is needed.

My problem wasn't actually this (which would only have hit, if I tried
with more than one user), just that the number of mounts in
/proc/mounts grows linearly with the number of users.

That can't be helped in such an easy way unfortunately.

> > Propagating some mount flags and not propagating others is
> > inconsistent and confusing, so I wouldn't want that.  Currently
> > remount doesn't propagate mount flags, that may be a bug, 
> 
> For consistency reason, one can propagate all the flags. But
> propagating only those flags that interfere with shared-subtree
> semantics should suffice.

I still don't believe not propagating "allowusermnt" interferes with
mount propagation.  In my posted patches the mount (including
propagations) is allowed based on the "allowusermnt" flag on the
parent of the requested mount.  The flag is _not_ checked during
propagation.

Allowing this and other flags to NOT be propagated just makes it
possible to have a set of shared mounts with asymmetric properties,
which may actually be desirable.

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Ram Pai]

On Tue, 2007-04-17 at 21:43 +0200, Miklos Szeredi wrote:
> > > > I'm a bit lost about what is currently done and who advocates for what.
> > > > 
> > > > It seems to me the MNT_ALLOWUSERMNT (or whatever :) flag should be
> > > > propagated.  In the /share rbind+chroot example, I assume the admin
> > > > would start by doing
> > > > 
> > > > 	mount --bind /share /share
> > > > 	mount --make-slave /share
> > > > 	mount --bind -o allow_user_mounts /share (or whatever)
> > > > 	mount --make-shared /share
> > > > 
> > > > then on login, pam does
> > > > 
> > > > 	chroot /share/$USER
> > > > 
> > > > or some sort of
> > > > 
> > > > 	mount --bind /share /home/$USER/root
> > > > 	chroot /home/$USER/root
> > > > 
> > > > or whatever.  In any case, the user cannot make user mounts except under
> > > > /share, and any cloned namespaces will still allow user mounts.
> > > 
> > > I don't quite understand your method.  This is how I think of it:
> > > 
> > > mount --make-rshared /
> > > mkdir -p /mnt/ns/$USER
> > > mount --rbind / /mnt/ns/$USER
> > > mount --make-rslave /mnt/ns/$USER
> > > mount --set-flags --recursive -oallowusermnt /mnt/ns/$USER
> > > chroot /mnt/ns/$USER
> > > su - $USER
> > > 
> > > I did actually try something equivalent (without the fancy mount
> > > commands though), and it worked fine.  The only "problem" is the
> > > proliferation of mounts in /proc/mounts.  There was a recently posted
> > > patch in AppArmor, that at least hides unreachable mounts from
> > > /proc/mounts, so the user wouldn't see all those.  But it could still
> > > be pretty confusing to the sysadmin.
> > 
> > unbindable mounts were designed to overcome the proliferation problem.
> > 
> > Your steps should be something like this:
> > 
> > mount --make-rshared /
> > mkdir -p /mnt/ns
> > mount --bind /mnt/ns /mnt/ns
> > mount --make-unbindable /mnt/ns
> > mkdir -p /mnt/ns/$USER
> > mount --rbind / /mnt/ns/$USER
> > mount --make-rslave /mnt/ns/$USER
> > mount --set-flags --recursive -oallowusermnt /mnt/ns/$USER
> > chroot /mnt/ns/$USER
> > su - $USER
> > 
> > try this and your proliferation problem will disappear. :-)
> 
> Right, this is needed.
> 
> My problem wasn't actually this (which would only have hit, if I tried
> with more than one user), just that the number of mounts in
> /proc/mounts grows linearly with the number of users.
> 
> That can't be helped in such an easy way unfortunately.
> 
> > > Propagating some mount flags and not propagating others is
> > > inconsistent and confusing, so I wouldn't want that.  Currently
> > > remount doesn't propagate mount flags, that may be a bug, 
> > 
> > For consistency reason, one can propagate all the flags. But
> > propagating only those flags that interfere with shared-subtree
> > semantics should suffice.
> 
> I still don't believe not propagating "allowusermnt" interferes with
> mount propagation.  In my posted patches the mount (including
> propagations) is allowed based on the "allowusermnt" flag on the
> parent of the requested mount.  The flag is _not_ checked during
> propagation.
> 
> Allowing this and other flags to NOT be propagated just makes it
> possible to have a set of shared mounts with asymmetric properties,
> which may actually be desirable.

The shared mount feature was designed to ensure that the mount remained
identical at all the locations. Now designing features 
to make it un-identical but still naming it shared, will break its
original purpose.  Slave mounts were designed to make it asymmetric.

Whatever feature that is desired to be exploited; can that be exploited
with the current set of semantics that we have? Is there a real need to
make the mounts asymmetric but at the same time name them as shared?
Maybe I dont understand what the desired application is? 

RP

> 
> Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > Allowing this and other flags to NOT be propagated just makes it
> > possible to have a set of shared mounts with asymmetric properties,
> > which may actually be desirable.
> 
> The shared mount feature was designed to ensure that the mount remained
> identical at all the locations.

OK, so remount not propagating mount flags is a bug then?

> Now designing features to make it un-identical but still naming it
> shared, will break its original purpose.  Slave mounts were designed
> to make it asymmetric.

What if I want to modify flags in a master mount, but not the slave
mount?  Would I be screwed?  For example: mount is read-only in both
master and slave.  I want to mark it read-write in master but not in
slave.  What do I do?

> Whatever feature that is desired to be exploited; can that be exploited
> with the current set of semantics that we have? Is there a real need to
> make the mounts asymmetric but at the same time name them as shared?
> Maybe I dont understand what the desired application is? 

I do think this question of propagating mount flags is totally
independent of user mounts.

As it stands, currently remount doesn't propagate mount flags, and I
don't see any compelling reasons why it should.

The patchset introduces a new mount flag "allowusermnt", but I don't
see any compelling reason to propagate this flag _either_.

Please say so if you do have such a reason.  As I've explained, having
this flag set differently in parts of a propagation tree does not
interfere with or break propagation in any way.

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Ram Pai]

On Wed, 2007-04-18 at 11:19 +0200, Miklos Szeredi wrote:
> > > Allowing this and other flags to NOT be propagated just makes it
> > > possible to have a set of shared mounts with asymmetric properties,
> > > which may actually be desirable.
> > 
> > The shared mount feature was designed to ensure that the mount remained
> > identical at all the locations.
> 
> OK, so remount not propagating mount flags is a bug then?

As I said earlier, are there any flags currently that if not propagated 
can lead to conflicts with the shared subtree semantics? I am not aware
of any.  If you did notice a case, than according to me its a bug.

But the new proposed 'allow unpriviledged mounts' flag; if not
propagated among peers (and slaves) of a shared mount can lead to
conflicts with shared subtree semantics. Since mount in one
shared-mount; when propagated to its peer fails to mount and hence lead
to un-identical peers.



> 
> > Now designing features to make it un-identical but still naming it
> > shared, will break its original purpose.  Slave mounts were designed
> > to make it asymmetric.
> 
> What if I want to modify flags in a master mount, but not the slave
> mount?  Would I be screwed?  For example: mount is read-only in both
> master and slave.  I want to mark it read-write in master but not in
> slave.  What do I do?

Making mounts read-only or read-write -- will that effect mount
propagation in such a way that future mounts in any one of the
peers will not be able to propagate that mount to its peers or slaves?

I don't think it will. Hence its ok to selectively mark some mounts
read-only and some mounts read-write.

However with the introduction of unpriviledged mount semantics, there 
can be cases where a user has priviledges to mount at one location but
not at a different location. if these two location happen to share
a peer-relationship than I see a case of interference of read-write
flag semantics with shared subtree semantics. And hence we will end up
propagating the read-write flag too or have to craft a different
semantics that stays consistent. 



> 
> > Whatever feature that is desired to be exploited; can that be exploited
> > with the current set of semantics that we have? Is there a real need to
> > make the mounts asymmetric but at the same time name them as shared?
> > Maybe I dont understand what the desired application is? 
> 
> I do think this question of propagating mount flags is totally
> independent of user mounts.
> 
> As it stands, currently remount doesn't propagate mount flags, and I
> don't see any compelling reasons why it should.
> 
> The patchset introduces a new mount flag "allowusermnt", but I don't
> see any compelling reason to propagate this flag _either_.
> 
> Please say so if you do have such a reason.  As I've explained, having
> this flag set differently in parts of a propagation tree does not
> interfere with or break propagation in any way.

As I said earlier, I see a case where two mounts that are peers of each
other can become un-identical if we dont propagate the "allowusermnt".

As a practical example.

/tmp and /mnt are peers of each other.
/tmp has its "allowusermnt" flag set, which has not been propagated
to /mnt.

now a normal-user mounts an ext2 file system under /tmp at /tmp/1

unfortunately the mount wont appear under /mnt/1 

and this breaks the shared-subtree semantics which promises: whatever is
mounted under /tmp will also be visible under /mnt


and in case if you allow the mount to appear under /mnt/1, you will
break unpriviledge mounts semantics which promises: a normal user will
not be able to mount at a location that does not allow user-mounts.



RP


> 
> Miklos
> 

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> As I said earlier, I see a case where two mounts that are peers of each
> other can become un-identical if we dont propagate the "allowusermnt".
> 
> As a practical example.
> 
> /tmp and /mnt are peers of each other.
> /tmp has its "allowusermnt" flag set, which has not been propagated
> to /mnt.
> 
> now a normal-user mounts an ext2 file system under /tmp at /tmp/1
> 
> unfortunately the mount wont appear under /mnt/1 

Argh, that is not true.  That's what I've been trying to explain to
you all along.

The propagation will be done _regardless_ of the flag.  The flag is
only checked for the parent of the _requested_ mount.  If it is
allowed there, the mount, including any propagations are allowed.  If
it's denied, then obviously it's denied everywhere.

> and in case if you allow the mount to appear under /mnt/1, you will
> break unpriviledge mounts semantics which promises: a normal user will
> not be able to mount at a location that does not allow user-mounts.

No, it does not promise that.  The flag just promises, that the user
cannot _request_ a mount on the parent mount.

Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Ram Pai]

On Wed, 2007-04-18 at 21:14 +0200, Miklos Szeredi wrote:
> > As I said earlier, I see a case where two mounts that are peers of each
> > other can become un-identical if we dont propagate the "allowusermnt".
> > 
> > As a practical example.
> > 
> > /tmp and /mnt are peers of each other.
> > /tmp has its "allowusermnt" flag set, which has not been propagated
> > to /mnt.
> > 
> > now a normal-user mounts an ext2 file system under /tmp at /tmp/1
> > 
> > unfortunately the mount wont appear under /mnt/1 
> 
> Argh, that is not true.  That's what I've been trying to explain to
> you all along.

I now realize you did, but I failed to catch it. sorry :-(

> 
> The propagation will be done _regardless_ of the flag.  The flag is
> only checked for the parent of the _requested_ mount.  If it is
> allowed there, the mount, including any propagations are allowed.  If
> it's denied, then obviously it's denied everywhere.
> 
> > and in case if you allow the mount to appear under /mnt/1, you will
> > break unpriviledge mounts semantics which promises: a normal user will
> > not be able to mount at a location that does not allow user-mounts.
> 
> No, it does not promise that.  The flag just promises, that the user
> cannot _request_ a mount on the parent mount.

ok. if the ability for a normal user to mount something *indirectly*
under a mount that has its 'allowusermnt flag' unset, 
is acceptable under the definition of 'allowusermnt', i guess my only
choice is to accept it. :-)

RP

> 
> Miklos

Re: [Devel] Re: [patch 05/10] add "permit user mounts in new namespace" clone flag

[Miklos Szeredi]

> > > As I said earlier, I see a case where two mounts that are peers of each
> > > other can become un-identical if we dont propagate the "allowusermnt".
> > > 
> > > As a practical example.
> > > 
> > > /tmp and /mnt are peers of each other.
> > > /tmp has its "allowusermnt" flag set, which has not been propagated
> > > to /mnt.
> > > 
> > > now a normal-user mounts an ext2 file system under /tmp at /tmp/1
> > > 
> > > unfortunately the mount wont appear under /mnt/1 
> > 
> > Argh, that is not true.  That's what I've been trying to explain to
> > you all along.
> 
> I now realize you did, but I failed to catch it. sorry :-(

It is my fault also.  I will add better description to the patch
headers.

> > The propagation will be done _regardless_ of the flag.  The flag is
> > only checked for the parent of the _requested_ mount.  If it is
> > allowed there, the mount, including any propagations are allowed.  If
> > it's denied, then obviously it's denied everywhere.
> > 
> > > and in case if you allow the mount to appear under /mnt/1, you will
> > > break unpriviledge mounts semantics which promises: a normal user will
> > > not be able to mount at a location that does not allow user-mounts.
> > 
> > No, it does not promise that.  The flag just promises, that the user
> > cannot _request_ a mount on the parent mount.
> 
> ok. if the ability for a normal user to mount something *indirectly*
> under a mount that has its 'allowusermnt flag' unset, 
> is acceptable under the definition of 'allowusermnt', i guess my only
> choice is to accept it. :-)

It is the only "sane" way I think.

Maybe the MS_SETFLAGS/MS_CLEARFLAGS mount operation could have an
option for propagating the given flag(s).  Then it's really up to the
sysadmin, to determine in each case what the desired behavior is.

Yes, you are right, that most of the time propagating the
"allowusermnt" between peers may be the right thing to do.  But for
example propagating it from master to slave may not be a good thing at
all.  So it shouldn't be for the kernel to decide.

Thanks,
Miklos

[patch 06/10] propagate error values from clone_mnt

[Miklos Szeredi]

[-- Attachment #1: clone_return_errno.patch --]
[-- Type: text/plain, Size: 5150 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Allow clone_mnt() to return errors other than ENOMEM.  This will be
used for returning a different error value when the number of user
mounts goes over the limit.

Fix copy_tree() to return EPERM for unbindable mounts.

Don't propagate further from dup_mnt_ns() as that copy_tree() can only
fail with -ENOMEM.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2007-04-12 13:54:36.000000000 +0200
+++ linux/fs/namespace.c	2007-04-12 14:04:25.000000000 +0200
@@ -261,42 +261,42 @@ static struct vfsmount *clone_mnt(struct
 {
 	struct super_block *sb = old->mnt_sb;
 	struct vfsmount *mnt = alloc_vfsmnt(old->mnt_devname);
+	if (!mnt)
+		return ERR_PTR(-ENOMEM);
 
-	if (mnt) {
-		mnt->mnt_flags = old->mnt_flags;
-		atomic_inc(&sb->s_active);
-		mnt->mnt_sb = sb;
-		mnt->mnt_root = dget(root);
-		mnt->mnt_mountpoint = mnt->mnt_root;
-		mnt->mnt_parent = mnt;
-
-		/* don't copy the MNT_USER flag */
-		mnt->mnt_flags &= ~MNT_USER;
-		if (flag & CL_SETUSER)
-			set_mnt_user(mnt);
-
-		if (flag & CL_SLAVE) {
-			list_add(&mnt->mnt_slave, &old->mnt_slave_list);
-			mnt->mnt_master = old;
-			CLEAR_MNT_SHARED(mnt);
-		} else {
-			if ((flag & CL_PROPAGATION) || IS_MNT_SHARED(old))
-				list_add(&mnt->mnt_share, &old->mnt_share);
-			if (IS_MNT_SLAVE(old))
-				list_add(&mnt->mnt_slave, &old->mnt_slave);
-			mnt->mnt_master = old->mnt_master;
-		}
-		if (flag & CL_MAKE_SHARED)
-			set_mnt_shared(mnt);
+	mnt->mnt_flags = old->mnt_flags;
+	atomic_inc(&sb->s_active);
+	mnt->mnt_sb = sb;
+	mnt->mnt_root = dget(root);
+	mnt->mnt_mountpoint = mnt->mnt_root;
+	mnt->mnt_parent = mnt;
+
+	/* don't copy the MNT_USER flag */
+	mnt->mnt_flags &= ~MNT_USER;
+	if (flag & CL_SETUSER)
+		set_mnt_user(mnt);
 
-		/* stick the duplicate mount on the same expiry list
-		 * as the original if that was on one */
-		if (flag & CL_EXPIRE) {
-			spin_lock(&vfsmount_lock);
-			if (!list_empty(&old->mnt_expire))
-				list_add(&mnt->mnt_expire, &old->mnt_expire);
-			spin_unlock(&vfsmount_lock);
-		}
+	if (flag & CL_SLAVE) {
+		list_add(&mnt->mnt_slave, &old->mnt_slave_list);
+		mnt->mnt_master = old;
+		CLEAR_MNT_SHARED(mnt);
+	} else {
+		if ((flag & CL_PROPAGATION) || IS_MNT_SHARED(old))
+			list_add(&mnt->mnt_share, &old->mnt_share);
+		if (IS_MNT_SLAVE(old))
+			list_add(&mnt->mnt_slave, &old->mnt_slave);
+		mnt->mnt_master = old->mnt_master;
+	}
+	if (flag & CL_MAKE_SHARED)
+		set_mnt_shared(mnt);
+
+	/* stick the duplicate mount on the same expiry list
+	 * as the original if that was on one */
+	if (flag & CL_EXPIRE) {
+		spin_lock(&vfsmount_lock);
+		if (!list_empty(&old->mnt_expire))
+			list_add(&mnt->mnt_expire, &old->mnt_expire);
+		spin_unlock(&vfsmount_lock);
 	}
 	return mnt;
 }
@@ -781,11 +781,11 @@ struct vfsmount *copy_tree(struct vfsmou
 	struct nameidata nd;
 
 	if (!(flag & CL_COPY_ALL) && IS_MNT_UNBINDABLE(mnt))
-		return NULL;
+		return ERR_PTR(-EPERM);
 
 	res = q = clone_mnt(mnt, dentry, flag);
-	if (!q)
-		goto Enomem;
+	if (IS_ERR(q))
+		goto error;
 	q->mnt_mountpoint = mnt->mnt_mountpoint;
 
 	p = mnt;
@@ -806,8 +806,8 @@ struct vfsmount *copy_tree(struct vfsmou
 			nd.mnt = q;
 			nd.dentry = p->mnt_mountpoint;
 			q = clone_mnt(p, p->mnt_root, flag);
-			if (!q)
-				goto Enomem;
+			if (IS_ERR(q))
+				goto error;
 			spin_lock(&vfsmount_lock);
 			list_add_tail(&q->mnt_list, &res->mnt_list);
 			attach_mnt(q, &nd);
@@ -815,7 +815,7 @@ struct vfsmount *copy_tree(struct vfsmou
 		}
 	}
 	return res;
-Enomem:
+ error:
 	if (res) {
 		LIST_HEAD(umount_list);
 		spin_lock(&vfsmount_lock);
@@ -823,7 +823,7 @@ Enomem:
 		spin_unlock(&vfsmount_lock);
 		release_mounts(&umount_list);
 	}
-	return NULL;
+	return q;
 }
 
 /*
@@ -999,13 +999,13 @@ static int do_loopback(struct nameidata 
 		goto out;
 
 	clone_flags = (flags & MS_SETUSER) ? CL_SETUSER : 0;
-	err = -ENOMEM;
 	if (flags & MS_REC)
 		mnt = copy_tree(old_nd.mnt, old_nd.dentry, clone_flags);
 	else
 		mnt = clone_mnt(old_nd.mnt, old_nd.dentry, clone_flags);
 
-	if (!mnt)
+	err = PTR_ERR(mnt);
+	if (IS_ERR(mnt))
 		goto out;
 
 	err = graft_tree(mnt, nd);
@@ -1558,7 +1558,7 @@ static struct mnt_namespace *dup_mnt_ns(
 	/* First pass: copy the tree topology */
 	new_ns->root = copy_tree(mnt_ns->root, mnt_ns->root->mnt_root,
 					CL_COPY_ALL | CL_EXPIRE);
-	if (!new_ns->root) {
+	if (IS_ERR(new_ns->root)) {
 		up_write(&namespace_sem);
 		kfree(new_ns);
 		return NULL;
Index: linux/fs/pnode.c
===================================================================
--- linux.orig/fs/pnode.c	2007-04-12 13:26:42.000000000 +0200
+++ linux/fs/pnode.c	2007-04-12 14:04:25.000000000 +0200
@@ -187,8 +187,9 @@ int propagate_mnt(struct vfsmount *dest_
 
 		source =  get_source(m, prev_dest_mnt, prev_src_mnt, &type);
 
-		if (!(child = copy_tree(source, source->mnt_root, type))) {
-			ret = -ENOMEM;
+		child = copy_tree(source, source->mnt_root, type);
+		if (IS_ERR(child)) {
+			ret = PTR_ERR(child);
 			list_splice(tree_list, tmp_list.prev);
 			goto out;
 		}

--

[patch 07/10] allow unprivileged bind mounts

[Miklos Szeredi]

[-- Attachment #1: unprivileged_bind_mount.patch --]
[-- Type: text/plain, Size: 4128 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Allow bind mounts to unprivileged users if the following conditions
are met:

  - user mounts are permitted in the current mount namespace
  - mountpoint is not a symlink or special file
  - mountpoint is not a sticky directory or is owned by the current user
  - mountpoint is writable by user
  - the number of user mounts is below the maximum

Unprivileged mounts imply MS_SETUSER, and will also have the "nosuid"
and "nodev" mount flags set.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2007-04-12 14:04:25.000000000 +0200
+++ linux/fs/namespace.c	2007-04-12 14:04:27.000000000 +0200
@@ -237,11 +237,30 @@ static void dec_nr_user_mounts(void)
 	spin_unlock(&vfsmount_lock);
 }
 
-static void set_mnt_user(struct vfsmount *mnt)
+static int reserve_user_mount(void)
+{
+	int err = 0;
+	spin_lock(&vfsmount_lock);
+	if (nr_user_mounts >= max_user_mounts && !capable(CAP_SYS_ADMIN))
+		err = -EPERM;
+	else
+		nr_user_mounts++;
+	spin_unlock(&vfsmount_lock);
+	return err;
+}
+
+static void __set_mnt_user(struct vfsmount *mnt)
 {
 	BUG_ON(mnt->mnt_flags & MNT_USER);
 	mnt->mnt_uid = current->uid;
 	mnt->mnt_flags |= MNT_USER;
+	if (!capable(CAP_SYS_ADMIN))
+		mnt->mnt_flags |= MNT_NOSUID | MNT_NODEV;
+}
+
+static void set_mnt_user(struct vfsmount *mnt)
+{
+	__set_mnt_user(mnt);
 	spin_lock(&vfsmount_lock);
 	nr_user_mounts++;
 	spin_unlock(&vfsmount_lock);
@@ -260,9 +279,16 @@ static struct vfsmount *clone_mnt(struct
 					int flag)
 {
 	struct super_block *sb = old->mnt_sb;
-	struct vfsmount *mnt = alloc_vfsmnt(old->mnt_devname);
+	struct vfsmount *mnt;
+
+	if (flag & CL_SETUSER) {
+		int err = reserve_user_mount();
+		if (err)
+			return ERR_PTR(err);
+	}
+	mnt = alloc_vfsmnt(old->mnt_devname);
 	if (!mnt)
-		return ERR_PTR(-ENOMEM);
+		goto alloc_failed;
 
 	mnt->mnt_flags = old->mnt_flags;
 	atomic_inc(&sb->s_active);
@@ -274,7 +300,7 @@ static struct vfsmount *clone_mnt(struct
 	/* don't copy the MNT_USER flag */
 	mnt->mnt_flags &= ~MNT_USER;
 	if (flag & CL_SETUSER)
-		set_mnt_user(mnt);
+		__set_mnt_user(mnt);
 
 	if (flag & CL_SLAVE) {
 		list_add(&mnt->mnt_slave, &old->mnt_slave_list);
@@ -299,6 +325,11 @@ static struct vfsmount *clone_mnt(struct
 		spin_unlock(&vfsmount_lock);
 	}
 	return mnt;
+
+ alloc_failed:
+	if (flag & CL_SETUSER)
+		dec_nr_user_mounts();
+	return ERR_PTR(-ENOMEM);
 }
 
 static inline void __mntput(struct vfsmount *mnt)
@@ -745,22 +776,35 @@ asmlinkage long sys_oldumount(char __use
 
 #endif
 
-static int mount_is_safe(struct nameidata *nd)
+/*
+ * Conditions for unprivileged mounts are:
+ * - user mounts are permitted in the current mount namespace
+ * - mountpoint is not a symlink or special file
+ * - mountpoint is "absolutely" writable by user
+ *  o if it's a sticky directory, it must be owned by the user
+ *  o it must not be an append-only file/directory
+ */
+static int mount_is_safe(struct nameidata *nd, int *flags)
 {
+	struct inode *inode = nd->dentry->d_inode;
+
 	if (capable(CAP_SYS_ADMIN))
 		return 0;
-	return -EPERM;
-#ifdef notyet
-	if (S_ISLNK(nd->dentry->d_inode->i_mode))
+
+	if (!(current->nsproxy->mnt_ns->flags & MNT_NS_PERMIT_USERMOUNTS))
 		return -EPERM;
-	if (nd->dentry->d_inode->i_mode & S_ISVTX) {
-		if (current->uid != nd->dentry->d_inode->i_uid)
-			return -EPERM;
-	}
-	if (vfs_permission(nd, MAY_WRITE))
+
+	if (!S_ISDIR(inode->i_mode) && !S_ISREG(inode->i_mode))
+		return -EPERM;
+
+	if ((inode->i_mode & S_ISVTX) && current->fsuid != inode->i_uid)
 		return -EPERM;
+
+	if (vfs_permission(nd, MAY_WRITE) || IS_APPEND(inode))
+		return -EPERM;
+
+	*flags |= MS_SETUSER;
 	return 0;
-#endif
 }
 
 static int lives_below_in_same_fs(struct dentry *d, struct dentry *dentry)
@@ -981,7 +1025,7 @@ static int do_loopback(struct nameidata 
 	int clone_flags;
 	struct nameidata old_nd;
 	struct vfsmount *mnt = NULL;
-	int err = mount_is_safe(nd);
+	int err = mount_is_safe(nd, &flags);
 	if (err)
 		return err;
 	if (!old_name || !*old_name)

--

[patch 08/10] put declaration of put_filesystem() in fs.h

[Miklos Szeredi]

[-- Attachment #1: put_filesystem_in_header.patch --]
[-- Type: text/plain, Size: 1282 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Declarations go into headers.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/fs/super.c
===================================================================
--- linux.orig/fs/super.c	2007-04-12 13:26:42.000000000 +0200
+++ linux/fs/super.c	2007-04-12 14:04:29.000000000 +0200
@@ -40,10 +40,6 @@
 #include <asm/uaccess.h>
 
 
-void get_filesystem(struct file_system_type *fs);
-void put_filesystem(struct file_system_type *fs);
-struct file_system_type *get_fs_type(const char *name);
-
 LIST_HEAD(super_blocks);
 DEFINE_SPINLOCK(sb_lock);
 
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h	2007-04-12 13:26:42.000000000 +0200
+++ linux/include/linux/fs.h	2007-04-12 14:04:29.000000000 +0200
@@ -1918,6 +1918,8 @@ extern int vfs_fstat(unsigned int, struc
 
 extern int vfs_ioctl(struct file *, unsigned int, unsigned int, unsigned long);
 
+extern void get_filesystem(struct file_system_type *fs);
+extern void put_filesystem(struct file_system_type *fs);
 extern struct file_system_type *get_fs_type(const char *name);
 extern struct super_block *get_super(struct block_device *);
 extern struct super_block *user_get_super(dev_t);

--

[patch 09/10] allow unprivileged mounts

[Miklos Szeredi]

[-- Attachment #1: unprivileged_mount.patch --]
[-- Type: text/plain, Size: 3748 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Define a new fs flag FS_SAFE, which denotes, that unprivileged
mounting of this filesystem may not constitute a security problem.

Since most filesystems haven't been designed with unprivileged
mounting in mind, a thorough audit is needed before setting this flag.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2007-04-12 14:04:27.000000000 +0200
+++ linux/fs/namespace.c	2007-04-12 14:04:32.000000000 +0200
@@ -784,7 +784,8 @@ asmlinkage long sys_oldumount(char __use
  *  o if it's a sticky directory, it must be owned by the user
  *  o it must not be an append-only file/directory
  */
-static int mount_is_safe(struct nameidata *nd, int *flags)
+static int mount_is_safe(struct nameidata *nd, struct file_system_type *type,
+			 int *flags)
 {
 	struct inode *inode = nd->dentry->d_inode;
 
@@ -794,6 +795,9 @@ static int mount_is_safe(struct nameidat
 	if (!(current->nsproxy->mnt_ns->flags & MNT_NS_PERMIT_USERMOUNTS))
 		return -EPERM;
 
+	if (type && !(type->fs_flags & FS_SAFE))
+		return -EPERM;
+
 	if (!S_ISDIR(inode->i_mode) && !S_ISREG(inode->i_mode))
 		return -EPERM;
 
@@ -1025,7 +1029,7 @@ static int do_loopback(struct nameidata 
 	int clone_flags;
 	struct nameidata old_nd;
 	struct vfsmount *mnt = NULL;
-	int err = mount_is_safe(nd, &flags);
+	int err = mount_is_safe(nd, NULL, &flags);
 	if (err)
 		return err;
 	if (!old_name || !*old_name)
@@ -1187,26 +1191,46 @@ out:
  * create a new mount for userspace and request it to be added into the
  * namespace's tree
  */
-static int do_new_mount(struct nameidata *nd, char *type, int flags,
+static int do_new_mount(struct nameidata *nd, char *fstype, int flags,
 			int mnt_flags, char *name, void *data)
 {
+	int err;
 	struct vfsmount *mnt;
+	struct file_system_type *type;
 
-	if (!type || !memchr(type, 0, PAGE_SIZE))
+	if (!fstype || !memchr(fstype, 0, PAGE_SIZE))
 		return -EINVAL;
 
-	/* we need capabilities... */
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
+	type = get_fs_type(fstype);
+	if (!type)
+		return -ENODEV;
 
-	mnt = do_kern_mount(type, flags & ~MS_SETUSER, name, data);
-	if (IS_ERR(mnt))
+	err = mount_is_safe(nd, type, &flags);
+	if (err)
+		goto out_put_filesystem;
+
+	if (flags & MS_SETUSER) {
+		err = reserve_user_mount();
+		if (err)
+			goto out_put_filesystem;
+	}
+
+	mnt = vfs_kern_mount(type, flags & ~MS_SETUSER, name, data);
+	put_filesystem(type);
+	if (IS_ERR(mnt)) {
+		if (flags & MS_SETUSER)
+			dec_nr_user_mounts();
 		return PTR_ERR(mnt);
+	}
 
 	if (flags & MS_SETUSER)
-		set_mnt_user(mnt);
+		__set_mnt_user(mnt);
 
 	return do_add_mount(mnt, nd, mnt_flags, NULL);
+
+ out_put_filesystem:
+	put_filesystem(type);
+	return err;
 }
 
 /*
@@ -1236,7 +1260,7 @@ int do_add_mount(struct vfsmount *newmnt
 	if (S_ISLNK(newmnt->mnt_root->d_inode->i_mode))
 		goto unlock;
 
-	/* MNT_USER was set earlier */
+	/* some flags may have been set earlier */
 	newmnt->mnt_flags |= mnt_flags;
 	if ((err = graft_tree(newmnt, nd)))
 		goto unlock;
Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h	2007-04-12 14:04:29.000000000 +0200
+++ linux/include/linux/fs.h	2007-04-12 14:04:32.000000000 +0200
@@ -96,6 +96,7 @@ extern int dir_notify_enable;
 #define FS_REQUIRES_DEV 1 
 #define FS_BINARY_MOUNTDATA 2
 #define FS_HAS_SUBTYPE 4
+#define FS_SAFE 8		/* Safe to mount by unprivileged users */
 #define FS_REVAL_DOT	16384	/* Check the paths ".", ".." for staleness */
 #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move()
 					 * during rename() internally.

--

[patch 10/10] allow unprivileged fuse mounts

[Miklos Szeredi]

[-- Attachment #1: fuse_safe.patch --]
[-- Type: text/plain, Size: 2493 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Use FS_SAFE for "fuse" fs type, but not for "fuseblk".

FUSE was designed from the beginning to be safe for unprivileged
users.  This has also been verified in practice over many years.  And
unprivileged fuse mounts still require a private namespace with user
mounts enabled, this is more strict than the current userspace policy.

This will enable future installations to remove the suid-root
fusermount utility.

Don't require the "user_id=" and "group_id=" options for unprivileged
mounts, but if they are present verify them for sanity.

Disallow the "allow_other" option for unprivileged mounts.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---

Index: linux/fs/fuse/inode.c
===================================================================
--- linux.orig/fs/fuse/inode.c	2007-04-12 13:26:42.000000000 +0200
+++ linux/fs/fuse/inode.c	2007-04-12 14:04:34.000000000 +0200
@@ -311,6 +311,19 @@ static int parse_fuse_opt(char *opt, str
 	d->max_read = ~0;
 	d->blksize = 512;
 
+	/*
+	 * For unprivileged mounts use current uid/gid.  Still allow
+	 * "user_id" and "group_id" options for compatibility, but
+	 * only if they match these values.
+	 */
+	if (!capable(CAP_SYS_ADMIN)) {
+		d->user_id = current->uid;
+		d->user_id_present = 1;
+		d->group_id = current->gid;
+		d->group_id_present = 1;
+
+	}
+
 	while ((p = strsep(&opt, ",")) != NULL) {
 		int token;
 		int value;
@@ -339,6 +352,8 @@ static int parse_fuse_opt(char *opt, str
 		case OPT_USER_ID:
 			if (match_int(&args[0], &value))
 				return 0;
+			if (d->user_id_present && d->user_id != value)
+				return 0;
 			d->user_id = value;
 			d->user_id_present = 1;
 			break;
@@ -346,6 +361,8 @@ static int parse_fuse_opt(char *opt, str
 		case OPT_GROUP_ID:
 			if (match_int(&args[0], &value))
 				return 0;
+			if (d->group_id_present && d->group_id != value)
+				return 0;
 			d->group_id = value;
 			d->group_id_present = 1;
 			break;
@@ -536,6 +553,10 @@ static int fuse_fill_super(struct super_
 	if (!parse_fuse_opt((char *) data, &d, is_bdev))
 		return -EINVAL;
 
+	/* This is a privileged option */
+	if ((d.flags & FUSE_ALLOW_OTHER) && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	if (is_bdev) {
 #ifdef CONFIG_BLOCK
 		if (!sb_set_blocksize(sb, d.blksize))
@@ -639,6 +660,7 @@ static struct file_system_type fuse_fs_t
 	.fs_flags	= FS_HAS_SUBTYPE,
 	.get_sb		= fuse_get_sb,
 	.kill_sb	= kill_anon_super,
+	.fs_flags	= FS_SAFE,
 };
 
 #ifdef CONFIG_BLOCK

--