[Devel] [PATCH vz10] fs: let the protection flags of a mount be locked from userspace

[Mirian Shilakadze <mirian.shilakadze@virtuozzo.com>]

A container can strip ro, nosuid, nodev or noexec from a bind mount the
host gave it with "mount -o remount,rw,bind" and then write host data.
Nothing raises the MNT_LOCK_* bits that forbid this on such a mount, and
userspace cannot set them.

Add MOUNT_ATTR_VE_LOCK to mount_setattr(). In attr_set it locks those
of the four protections the call leaves set, in attr_clr it drops the
locks. Only a host task with CAP_SYS_ADMIN in the initial user
namespace may use it. MNT_LOCKED is not raised, so the container can
still unmount the bind.

Locks can now appear on an attached mount, so can_change_locked_flags()
is re-tested under the mount hash in do_reconfigure_mnt() and
do_remount().

Feature: ve: ve generic structures
https://virtuozzo.atlassian.net/browse/VSTOR-141092
Signed-off-by: Mirian Shilakadze <mirian.shilakadze@virtuozzo.com>
---
The libvzctl side, which locks bindmounts at container start, on a live
add and again after restore, goes to vzctl.ovz separately.

Tested on 211.52.1.22.8 in a VM:
- a container cannot strip ro, nosuid, nodev or noexec from a locked bind
  by remount or mount_setattr(), cannot unlock it and cannot lock its own
  mounts, a bind it makes of a locked mount inherits the lock
- an unprivileged user, root without CAP_SYS_ADMIN and root in a user
  namespace of its own cannot unlock what lock_mnt_tree() locked
- a VE in pseudosuper cannot lock or unlock, the host can do both
- mount_setattr selftest 21/21, mount/ selftests pass
- a remount racing a lock, with a 2 ms sleep added to widen the window:
  about 30% of rounds end writable but locked without the re-tests, none
  in 26000 rounds with them

 fs/namespace.c                                | 113 ++++++++++++++----
 include/uapi/linux/mount.h                    |  22 ++++
 .../trace/beauty/include/uapi/linux/mount.h   |  22 ++++
 3 files changed, 137 insertions(+), 20 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 75cabf7763ef..07ee860cae6e 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -100,6 +100,7 @@ struct mount_kattr {
 	unsigned int propagation;
 	unsigned int lookup_flags;
 	bool recurse;
+	bool ve_lock;
 	struct user_namespace *mnt_userns;
 	struct mnt_idmap *mnt_idmap;
 };
@@ -2394,26 +2395,42 @@ int iterate_mounts(int (*f)(struct vfsmount *, void *), void *arg,
 	return 0;
 }
 
+#define MNT_LOCK_PROTECTION_MASK	(MNT_LOCK_READONLY | MNT_LOCK_NOSUID | \
+					 MNT_LOCK_NODEV | MNT_LOCK_NOEXEC)
+
+/*
+ * Raise the MNT_LOCK_* twin of every protection @flags carries, so that whoever
+ * is given the mount cannot clear them again. Shared by lock_mnt_tree(), where
+ * the kernel locks a tree that crosses a user namespace, and by
+ * MOUNT_ATTR_VE_LOCK, where the host asks for it.
+ *
+ * Neither MNT_LOCKED nor MNT_LOCK_ATIME is raised here. lock_mnt_tree() adds
+ * both itself, while a mount locked through MOUNT_ATTR_VE_LOCK can still be
+ * unmounted and keeps its atime settable.
+ */
+static unsigned int lock_protection_flags(unsigned int flags)
+{
+	if (flags & MNT_READONLY)
+		flags |= MNT_LOCK_READONLY;
+	if (flags & MNT_NOSUID)
+		flags |= MNT_LOCK_NOSUID;
+	if (flags & MNT_NODEV)
+		flags |= MNT_LOCK_NODEV;
+	if (flags & MNT_NOEXEC)
+		flags |= MNT_LOCK_NOEXEC;
+
+	return flags;
+}
+
 static void lock_mnt_tree(struct mount *mnt)
 {
 	struct mount *p;
 
 	for (p = mnt; p; p = next_mnt(p, mnt)) {
-		int flags = p->mnt.mnt_flags;
+		int flags = lock_protection_flags(p->mnt.mnt_flags);
+
 		/* Don't allow unprivileged users to change mount flags */
 		flags |= MNT_LOCK_ATIME;
-
-		if (flags & MNT_READONLY)
-			flags |= MNT_LOCK_READONLY;
-
-		if (flags & MNT_NODEV)
-			flags |= MNT_LOCK_NODEV;
-
-		if (flags & MNT_NOSUID)
-			flags |= MNT_LOCK_NOSUID;
-
-		if (flags & MNT_NOEXEC)
-			flags |= MNT_LOCK_NOEXEC;
 		/* Don't allow unprivileged users to reveal what is under a mount */
 		if (list_empty(&p->mnt_expire))
 			flags |= MNT_LOCKED;
@@ -3018,8 +3035,10 @@ SYSCALL_DEFINE3(open_tree, int, dfd, const char __user *, filename, unsigned, fl
 /*
  * Don't allow locked mount flags to be cleared.
  *
- * No locks need to be held here while testing the various MNT_LOCK
- * flags because those flags can never be cleared once they are set.
+ * MOUNT_ATTR_VE_LOCK can raise and drop the MNT_LOCK_* flags on a mount that
+ * is already attached, so a test made without the mount hash held only tells
+ * you what was true a moment ago. Test again under the hash before the change
+ * that depends on it.
  */
 static bool can_change_locked_flags(struct mount *mnt, unsigned int mnt_flags)
 {
@@ -3124,9 +3143,12 @@ static int do_reconfigure_mnt(struct path *path, unsigned int mnt_flags)
 	 */
 	down_read(&sb->s_umount);
 	lock_mount_hash();
-	ret = change_mount_ro_state(mnt, mnt_flags);
-	if (ret == 0)
-		set_mount_attributes(mnt, mnt_flags);
+	ret = -EPERM;
+	if (can_change_locked_flags(mnt, mnt_flags)) {
+		ret = change_mount_ro_state(mnt, mnt_flags);
+		if (ret == 0)
+			set_mount_attributes(mnt, mnt_flags);
+	}
 	unlock_mount_hash();
 	up_read(&sb->s_umount);
 
@@ -3382,6 +3404,20 @@ static inline int ve_try_reserve_mount(struct ve_struct *ve)
 	return ret;
 }
 
+/*
+ * Only the host may lock or unlock the protection flags of a mount, meaning a
+ * task in ve0 with CAP_SYS_ADMIN in the initial user namespace.
+ *
+ * ve0 alone proves nothing. Every host task is there, an unprivileged user in a
+ * user namespace of its own included, and could otherwise unlock what
+ * lock_mnt_tree() locked against it. A VE in pseudosuper is not enough either,
+ * libvzctl runs container binaries inside it while the flag is still raised.
+ */
+static inline bool ve_may_lock_mount(void)
+{
+	return ve_is_super(get_exec_env()) && capable(CAP_SYS_ADMIN);
+}
+
 static inline void ve_mount_put(struct mount *mnt, struct ve_struct *ve)
 {
 	/*
@@ -3440,6 +3476,7 @@ bool is_sb_ve_accessible(struct ve_struct *ve, struct super_block *sb)
 static inline int ve_try_reserve_mount(struct ve_struct *ve) { return 1; }
 static inline void ve_mount_put(struct mount *mnt, struct ve_struct *ve) { }
 static inline void ve_mount_reown(struct mount *mnt, struct ve_struct *ve) { }
+static inline bool ve_may_lock_mount(void) { return capable(CAP_SYS_ADMIN); }
 
 #endif /* CONFIG_VE */
 
@@ -3483,7 +3520,15 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags,
 			err = reconfigure_super(fc);
 			if (!err) {
 				lock_mount_hash();
-				set_mount_attributes(mnt, mnt_flags);
+				/*
+				 * A lock may have landed since the first test.
+				 * The superblock change above stands, the mount
+				 * flags do not change.
+				 */
+				if (can_change_locked_flags(mnt, mnt_flags))
+					set_mount_attributes(mnt, mnt_flags);
+				else
+					err = -EPERM;
 				unlock_mount_hash();
 			}
 		}
@@ -4543,7 +4588,8 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
 	 MOUNT_ATTR_NOEXEC | MOUNT_ATTR__ATIME | MOUNT_ATTR_NODIRATIME |       \
 	 MOUNT_ATTR_NOSYMFOLLOW)
 
-#define MOUNT_SETATTR_VALID_FLAGS (FSMOUNT_VALID_FLAGS | MOUNT_ATTR_IDMAP)
+#define MOUNT_SETATTR_VALID_FLAGS \
+	(FSMOUNT_VALID_FLAGS | MOUNT_ATTR_IDMAP | MOUNT_ATTR_VE_LOCK)
 
 #define MOUNT_SETATTR_PROPAGATION_FLAGS \
 	(MS_UNBINDABLE | MS_PRIVATE | MS_SLAVE | MS_SHARED)
@@ -5052,6 +5098,8 @@ static void mount_setattr_commit(struct mount_kattr *kattr, struct mount *mnt)
 
 		do_idmap_mount(kattr, m);
 		flags = recalc_flags(kattr, m);
+		if (kattr->ve_lock)
+			flags = lock_protection_flags(flags);
 		WRITE_ONCE(m->mnt.mnt_flags, flags);
 
 		/* If we had to hold writers unblock them. */
@@ -5230,8 +5278,33 @@ static int build_mount_kattr(const struct mount_attr *attr, size_t usize,
 	if ((attr->attr_set | attr->attr_clr) & ~MOUNT_SETATTR_VALID_FLAGS)
 		return -EINVAL;
 
+	kattr->ve_lock = !!(attr->attr_set & MOUNT_ATTR_VE_LOCK);
+	if (kattr->ve_lock || (attr->attr_clr & MOUNT_ATTR_VE_LOCK)) {
+		if (kattr->ve_lock && (attr->attr_clr & MOUNT_ATTR_VE_LOCK))
+			return -EINVAL;
+		/*
+		 * Unlocking says nothing about the protections themselves.
+		 * Relaxing one is a separate call, so it still goes through
+		 * can_change_locked_flags() against a mount that is by then
+		 * genuinely unlocked, rather than riding in on this one.
+		 */
+		if ((attr->attr_clr & MOUNT_ATTR_VE_LOCK) &&
+		    (attr->attr_clr & (MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID |
+				       MOUNT_ATTR_NODEV | MOUNT_ATTR_NOEXEC)))
+			return -EINVAL;
+		if (!ve_may_lock_mount())
+			return -EPERM;
+	}
+
 	kattr->attr_set = attr_flags_to_mnt_flags(attr->attr_set);
 	kattr->attr_clr = attr_flags_to_mnt_flags(attr->attr_clr);
+	/*
+	 * Unlocking is a plain clear, so recalc_flags() is already all it
+	 * needs. Locking cannot be: which twin to raise depends on what each
+	 * mount in the tree ends up with, which is only known per mount.
+	 */
+	if (attr->attr_clr & MOUNT_ATTR_VE_LOCK)
+		kattr->attr_clr |= MNT_LOCK_PROTECTION_MASK;
 
 	/*
 	 * Since the MOUNT_ATTR_<atime> values are an enum, not a bitmap,
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index 225bc366ffcb..6368876b37ad 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -125,6 +125,28 @@ enum fsconfig_command {
 #define MOUNT_ATTR_IDMAP	0x00100000 /* Idmap mount to @userns_fd in struct mount_attr. */
 #define MOUNT_ATTR_NOSYMFOLLOW	0x00200000 /* Do not follow symlinks */
 
+/*
+ * Virtuozzo mount attributes use high bits (from 63 downwards) to stay clear of
+ * the upstream range, which allocates upwards from bit 0. Keep them last in this
+ * list, after the mainline entries above, so a future upstream flag creeping up
+ * towards them is easy to spot.
+ */
+
+/*
+ * In attr_set, lock MOUNT_ATTR_RDONLY, MOUNT_ATTR_NOSUID, MOUNT_ATTR_NODEV and
+ * MOUNT_ATTR_NOEXEC, whichever of them the call leaves set, so that whoever is
+ * given the mount cannot clear them again. Those four are the only attributes
+ * this flag locks. In attr_clr, drop those locks again, on its own and not
+ * alongside the attributes it was guarding. mount_setattr() only, fsmount()
+ * takes its attributes as 32 bits and never sees this one.
+ *
+ * Only the host may do either, so a container cannot unlock what it was given
+ * and cannot lock anything against the host. The lock is per mount and is
+ * copied only into binds made after it, so set it before the mount is handed
+ * over, on the detached tree.
+ */
+#define MOUNT_ATTR_VE_LOCK	0x8000000000000000ULL
+
 /*
  * mount_setattr()
  */
diff --git a/tools/perf/trace/beauty/include/uapi/linux/mount.h b/tools/perf/trace/beauty/include/uapi/linux/mount.h
index 225bc366ffcb..6368876b37ad 100644
--- a/tools/perf/trace/beauty/include/uapi/linux/mount.h
+++ b/tools/perf/trace/beauty/include/uapi/linux/mount.h
@@ -125,6 +125,28 @@ enum fsconfig_command {
 #define MOUNT_ATTR_IDMAP	0x00100000 /* Idmap mount to @userns_fd in struct mount_attr. */
 #define MOUNT_ATTR_NOSYMFOLLOW	0x00200000 /* Do not follow symlinks */
 
+/*
+ * Virtuozzo mount attributes use high bits (from 63 downwards) to stay clear of
+ * the upstream range, which allocates upwards from bit 0. Keep them last in this
+ * list, after the mainline entries above, so a future upstream flag creeping up
+ * towards them is easy to spot.
+ */
+
+/*
+ * In attr_set, lock MOUNT_ATTR_RDONLY, MOUNT_ATTR_NOSUID, MOUNT_ATTR_NODEV and
+ * MOUNT_ATTR_NOEXEC, whichever of them the call leaves set, so that whoever is
+ * given the mount cannot clear them again. Those four are the only attributes
+ * this flag locks. In attr_clr, drop those locks again, on its own and not
+ * alongside the attributes it was guarding. mount_setattr() only, fsmount()
+ * takes its attributes as 32 bits and never sees this one.
+ *
+ * Only the host may do either, so a container cannot unlock what it was given
+ * and cannot lock anything against the host. The lock is per mount and is
+ * copied only into binds made after it, so set it before the mount is handed
+ * over, on the detached tree.
+ */
+#define MOUNT_ATTR_VE_LOCK	0x8000000000000000ULL
+
 /*
  * mount_setattr()
  */
-- 
2.43.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel