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

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

[Mirian Shilakadze]

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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] fs: do_remount() returns -EPERM after reconfigure_super() already changed the sb
- [Low] fs: MOUNT_ATTR_VE_LOCK lock state is write-only, not reported by mnt_to_attr_flags()
- [Low] perf beauty: MOUNT_ATTR_VE_LOCK grows fsmount_attr_flags to 65 entries, UB on 32-bit
--

commit 8c0f169fd03f2f47c6755d18498f04737a8f7f21
Author: Mirian Shilakadze <mirian.shilakadze@virtuozzo.com>

fs: let the protection flags of a mount be locked from userspace

This patch adds MOUNT_ATTR_VE_LOCK to mount_setattr(). With it, a host task
with CAP_SYS_ADMIN can lock or unlock the ro, nosuid, nodev and noexec
protections of a mount. It also re-tests can_change_locked_flags() under the
mount hash in do_reconfigure_mnt() and do_remount().

Link: https://virtuozzo.atlassian.net/browse/VSTOR-141092

> diff --git a/fs/namespace.c b/fs/namespace.c
> --- a/fs/namespace.c
> +++ b/fs/namespace.c

[ ... ]

> @@ -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();
>  			}
>  		}

[Severity: Medium]
Can do_remount() return -EPERM after the superblock has already been
changed?

The first can_change_locked_flags() test is made with no lock held. Then
reconfigure_super(fc) runs under s_umount. It rewrites sb->s_flags (for
example it clears SB_RDONLY) and calls fc->ops->reconfigure(). Neither can
be rolled back.

mount_setattr() takes only the mount hash, not s_umount. So nothing
serializes a host MOUNT_ATTR_VE_LOCK against this window. If the lock lands
between the first test and the re-test, the re-test fails and err is set to
-EPERM.

The caller then sees a failure, but the shared superblock may have gone rw
or taken new options. That affects every other mount of the same sb. The
comment in the patch says "The superblock change above stands".

Would it be possible to run the re-test before reconfigure_super(), or to
exclude the lock path from remount, for example by taking s_umount shared in
the lock path?

[ ... ]

> @@ -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);

[Severity: Low]
This isn't a bug, but the lock state set here looks write-only. Can
userspace read it back?

This is the only place the new flag is consumed. mnt_to_attr_flags(), which
builds the attribute word that statmount() returns as sm.mnt_attr, ignores
MNT_LOCK_READONLY, MNT_LOCK_NOSUID, MNT_LOCK_NODEV and MNT_LOCK_NOEXEC. It
never returns MOUNT_ATTR_VE_LOCK.

The commit message says libvzctl locks bindmounts at container start, on a
live add and again after restore. How is it expected to learn whether a
mount is already locked? The only probe appears to be a remount that is
expected to fail with EPERM.

[ ... ]

> diff --git a/tools/perf/trace/beauty/include/uapi/linux/mount.h b/tools/perf/trace/beauty/include/uapi/linux/mount.h
> --- a/tools/perf/trace/beauty/include/uapi/linux/mount.h
> +++ b/tools/perf/trace/beauty/include/uapi/linux/mount.h

[ ... ]

> +#define MOUNT_ATTR_VE_LOCK	0x8000000000000000ULL

[Severity: Low]
Can this define break the perf beauty flag table on 32-bit builds?

tools/perf/trace/beauty/fsmount.sh matches the define with its regex. The
ULL suffix is absorbed by the trailing ".*". It then emits this entry:

	[ilog2(0x8000000000000000) + 1] = "VE_LOCK",

That is index 64, so fsmount_attr_flags grows to 65 entries.
strarray__scnprintf_flags() then loops over those entries:

tools/perf/trace/beauty/pkey_alloc.c:strarray__scnprintf_flags() {
	for (i = 1; i < sa->nr_entries; ++i) {
		unsigned long bit = 1UL << (i - 1);
		...
}

On a 32-bit host, unsigned long is 32 bits wide. Every iteration with
i >= 34 shifts by 33 or more, which is undefined behaviour. The shift
can alias low flag bits, so perf trace may print wrong flag names for
fsmount() attributes.

Before this patch the highest index was around 22. Also, fsmount() takes
only 32-bit attributes, so the VE_LOCK entry would never be used there.

Could this define be kept out of the copy that the beauty scripts parse, or
could fsmount.sh skip entries that do not fit in 32 bits?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012102058.46896-1-mirian.shilakadze@virtuozzo.com?part=1

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

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

[Pavel Tikhomirov]

On 10/12/26 12:20, Mirian Shilakadze wrote:
> 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.

I like the idea. Looks generally good. Proabbly mainstreamable.

Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

Should we dump those flags in CRIU? Should we also expose them for that.

 MNT_LOCKED is not raised, so the container can
> still unmount the bind.

By the way, it might be a good idea to have a way to raise MNT_LOCKED on the mounts vzctl does for CT, so that CT can't umount it even if it wants to (usecase: protect external mount mountpoint for CRIU to be able to restore it easily (now we fail if external mount was moved in CT).

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

Let's, maybe, add selftests?

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

Maybe confusing name, I would expect ve_may... allow something for ve root, not prohibit.
Also ve_is_super(get_exec_env()) is excess if you already check capable.

> +{
> +	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)

Maybe name it kattr->lock_flags? for more mainstreamability.

> +			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()
>   */

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.

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