[Devel] [PATCH vz10 2/3] ve/fs: transfer mount ownership when a mount enters another VE

[Mirian Shilakadze]

ve_owner is assigned once in ve_mount_nr_inc() from alloc_vfsmnt() and
nothing updates it afterwards, so a mount the host creates and moves into
a container keeps ve_owner == ve0 while it lives in the container's mount
namespace.

ve_check_trusted_file() reads that field for filesystems with no s_bdev
and lets ve0 execute from any mount it considers host owned, so a host
tmpfs bindmounted into a container is trusted even though the container
can write to it. A tmpfs the container creates itself is refused, the
same binary planted by the same container on a mount the host lent it
runs:

  vzctl set 971 --bindmount_add /root/tex_tmpfs:/mnt/bm_tmpfs --save
  vzctl exec 971 'cp /bin/echo /mnt/bm_tmpfs/planted &&
                  chmod 755 /mnt/bm_tmpfs/planted'
  nsenter -t $INITPID -m /mnt/bm_tmpfs/planted HOST-TMPFS-LENT-TO-CT
  HOST-TMPFS-LENT-TO-CT

Fix it where the mount changes hands. Every path that puts an existing
mount into another VE's namespace goes through commit_tree(), both the
detached tree the container moves in with move_mount() and the copies the
propagation loop in attach_recursive_mnt() commits into a foreign
namespace, so adopt the namespace's owner there. Ownership describes
where the mount is, so it follows the namespace and the direction of the
move is not special cased.

That covers every mount that moves. Mounts created with the wrong owner
to begin with, which copy_mnt_ns() and open_detached_copy() both do for a
ve0 task working inside a container, never pass through commit_tree() and
are fixed by the next patch.

The same field drives per-VE mount accounting, which was wrong in the
same direction: a moved in mount was charged to ve0 rather than to the
container holding it.

A ve0 process that execs or mmaps from a mount this reowns is now
refused, exec with -EACCES and mmap with -EBADF, plus a SIGSEGV for the
first few attempts. That is the point of the change, but it is visible to
host tooling that reaches into a container's mounts to run something.

The transfer does not consult sysctl_ve_mount_nr, commit_tree() cannot
fail. A container can therefore be pushed above its mount limit by mounts
the host gives it, and while over it the container's own mounts are
refused until the count drops. The default limit is 4096 so this takes an
unusual number of lent mounts, but it is the host's action that spends
the container's budget.

The owner change is done under the vfsmount lock, which serializes it
against is_sb_ve_accessible() walking sb->s_mounts.
ve_check_trusted_file() reads ve_owner without that lock, so both the
store and the load are marked. It only compares the pointer against ve0
and the field is never NULL in between, so the reader sees either the old
or the new owner. The reference on the old VE is dropped only after the
new one is taken.

Fixes: d65efacf542b ("trusted/ve/fs/exec: Don't allow a privileged user to execute untrusted files")
Fixes: fc7157b84c32 ("trusted/ve/mmap: Protect from unsecure library load from CT image")
https://virtuozzo.atlassian.net/browse/VSTOR-141322
Feature: ve: ve generic structures
Signed-off-by: Mirian Shilakadze <mirian.shilakadze at virtuozzo.com>
---
 fs/mount.h     |  2 +-
 fs/namespace.c | 27 +++++++++++++++++++++++++++
 kernel/ve/ve.c |  6 +++++-
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/fs/mount.h b/fs/mount.h
index 5cf06431d586..1d41fd15265e 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -71,7 +71,7 @@ struct mount {
 	};
 	struct list_head mnt_umounting; /* list entry for umount propagation */
 #ifdef CONFIG_VE
-	struct ve_struct *ve_owner;	/* VE in which this mount was created */
+	struct ve_struct *ve_owner;	/* VE whose mount namespace holds it */
 #endif /* CONFIG_VE */
 #ifdef CONFIG_FSNOTIFY
 	struct fsnotify_mark_connector __rcu *mnt_fsnotify_marks;
diff --git a/fs/namespace.c b/fs/namespace.c
index 7e27537dcdaf..f8319a2b33df 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -320,6 +320,7 @@ int mnt_get_count(struct mount *mnt)
 static inline int ve_mount_allowed(void);
 static inline void ve_mount_nr_inc(struct mount *mnt, struct ve_struct *ve);
 static inline void ve_mount_nr_dec(struct mount *mnt);
+static inline void ve_mount_reown(struct mount *mnt, struct ve_struct *ve);
 
 static struct mount *alloc_vfsmnt(const char *name, struct ve_struct *owner_ve)
 {
@@ -1182,6 +1183,7 @@ static void commit_tree(struct mount *mnt)
 		m = list_first_entry(&head, typeof(*m), mnt_list);
 		list_del(&m->mnt_list);
 
+		ve_mount_reown(m, n->ve_owner);
 		mnt_add_to_ns(n, m);
 	}
 	n->nr_mounts += n->pending_mounts;
@@ -3370,6 +3372,30 @@ static inline void ve_mount_nr_dec(struct mount *mnt)
 	mnt->ve_owner = NULL;
 }
 
+/*
+ * A mount that enters the mount namespace of another VE changes hands, so
+ * that per-VE mount accounting and the trusted-exec check see it as owned by
+ * the VE it now lives in. Ownership describes where the mount is, so it
+ * simply follows the namespace and nothing here special cases which way the
+ * mount travelled.
+ *
+ * vfsmount lock must be held for write, it serializes the owner change
+ * against is_sb_ve_accessible(). The trusted-exec path reads ve_owner
+ * locklessly but only compares the pointer, which is never NULL here.
+ */
+static inline void ve_mount_reown(struct mount *mnt, struct ve_struct *ve)
+{
+	struct ve_struct *old = mnt->ve_owner;
+
+	if (old == ve)
+		return;
+
+	atomic_inc(&ve->mnt_nr);
+	WRITE_ONCE(mnt->ve_owner, get_ve(ve));
+	atomic_dec(&old->mnt_nr);
+	put_ve(old);
+}
+
 bool is_sb_ve_accessible(struct ve_struct *ve, struct super_block *sb)
 {
 	struct mount *mnt;
@@ -3392,6 +3418,7 @@ bool is_sb_ve_accessible(struct ve_struct *ve, struct super_block *sb)
 static inline int ve_mount_allowed(void) { return 1; }
 static inline void ve_mount_nr_inc(struct mount *mnt, struct ve_struct *ve) { }
 static inline void ve_mount_nr_dec(struct mount *mnt) { }
+static inline void ve_mount_reown(struct mount *mnt, struct ve_struct *ve) { }
 #endif /* CONFIG_VE */
 
 /*
diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
index 73d1c3b4873e..e976ffcc9273 100644
--- a/kernel/ve/ve.c
+++ b/kernel/ve/ve.c
@@ -1716,9 +1716,13 @@ static bool ve_check_trusted_file(struct file *file)
 		/*
 		 * bdev can be NULL if the file is on tmpfs, for example.
 		 * If this is a host's tmpfs - execution is allowed.
+		 *
+		 * The read is unlocked and pairs with the store in
+		 * ve_mount_reown(): a task holding a descriptor on the mount
+		 * can be executing from it while the mount changes hands.
 		 */
 		file_on_host_mount = ve_is_super(
-				     real_mount(file->f_path.mnt)->ve_owner);
+			READ_ONCE(real_mount(file->f_path.mnt)->ve_owner));
 		if (file_on_host_mount)
 			return true;
 	}
-- 
2.43.0