[Devel] [PATCH vz10] fs: let the protection flags of a mount be locked from userspace
Pavel Tikhomirov
ptikhomirov at virtuozzo.com
Mon Oct 12 17:02:09 MSK 2026
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 at 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 at 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.
More information about the Devel
mailing list