Check for mounts marked as unmounted before propagating.

If the namespace were set to nil during umount (such as in Linux), checks
for mnt.neverConnected() would cover this case. We keep mnt.ns as non-nil,
so we need to also check umounted in some cases.

PiperOrigin-RevId: 586747189

pkg/sentry/vfs/mount.go

@@ -33,7 +33,11 @@ import (
 // MountMax is the maximum number of mounts allowed. In Linux this can be
 // configured by the user at /proc/sys/fs/mount-max, but the default is
 // 100,000. We set the gVisor limit to 10,000.
-const MountMax = 10000
+const (
+	MountMax     = 10000
+	nsfsName     = "nsfs"
+	cgroupFsName = "cgroup"
+)
 
 // A Mount is a replacement of a Dentry (Mount.key.point) from one Filesystem
 // (Mount.key.parent.fs) with a Dentry (Mount.root) from another Filesystem
@@ -214,6 +218,19 @@ func (mnt *Mount) coveringMount() *Mount {
 	return child
 }
 
+// validInMountNS checks if the mount is valid in the current mount namespace. This includes
+// checking if has previously been unmounted. It is analogous to fs/namespace.c:check_mnt() in
+// Linux.
+//
+// +checklocks:vfs.mountMu
+func (vfs *VirtualFilesystem) validInMountNS(ctx context.Context, mnt *Mount) bool {
+	if mntns := MountNamespaceFromContext(ctx); mntns != nil {
+		vfs.delayDecRef(mntns)
+		return mnt.ns == mntns && !mnt.umounted
+	}
+	return false
+}
+
 // NewFilesystem creates a new filesystem object not yet associated with any
 // mounts. It can be installed into the filesystem tree with ConnectMountAt.
 // Note that only the filesystem-specific mount options from opts are used by
@@ -253,20 +270,17 @@ func (vfs *VirtualFilesystem) MountDisconnected(ctx context.Context, creds *auth
 	return newMount(vfs, fs, root, nil /* mntns */, opts), nil
 }
 
-// attachTreeLocked attaches the mount tree at mnt to vd and propagates the
-// mount to vd.mount's peers and followers. This method consumes the reference
-// on vd. It is analogous to fs/namespace.c:attach_recursive_mnt() in Linux.
+// attachTreeLocked attaches the mount tree at mnt to mp and propagates the mount to mp.mount's
+// peers and followers. This method consumes the reference on mp. It is analogous to
+// fs/namespace.c:attach_recursive_mnt() in Linux. The mount point mp must have its dentry locked
+// before calling attachTreeLocked.
 //
 // +checklocks:vfs.mountMu
-func (vfs *VirtualFilesystem) attachTreeLocked(ctx context.Context, mnt *Mount, vd VirtualDentry) error {
-	mp, err := vfs.lockMountpoint(vd)
-	if err != nil {
-		return err
-	}
+func (vfs *VirtualFilesystem) attachTreeLocked(ctx context.Context, mnt *Mount, mp VirtualDentry) error {
 	cleanup := cleanup.Make(func() {
 		vfs.cleanupGroupIDs(mnt.submountsLocked()) // +checklocksforce
 		mp.dentry.mu.Unlock()
-		mp.DecRef(ctx)
+		vfs.delayDecRef(mp)
 	})
 	defer cleanup.Clean()
 	// This is equivalent to checking for SB_NOUSER in Linux, which is set on all
@@ -279,7 +293,10 @@ func (vfs *VirtualFilesystem) attachTreeLocked(ctx context.Context, mnt *Mount,
 		return err
 	}
 
-	var propMnts map[*Mount]struct{}
+	var (
+		propMnts map[*Mount]struct{}
+		err      error
+	)
 	if mp.mount.isShared {
 		if err := vfs.allocMountGroupIDs(mnt, true); err != nil {
 			return err
@@ -325,7 +342,16 @@ func (vfs *VirtualFilesystem) ConnectMountAt(ctx context.Context, creds *auth.Cr
 	}
 	vfs.lockMounts()
 	defer vfs.unlockMounts(ctx)
-	return vfs.attachTreeLocked(ctx, mnt, vd)
+	mp, err := vfs.lockMountpoint(vd)
+	if err != nil {
+		return err
+	}
+	if mp.mount.neverConnected() || mp.mount.umounted {
+		mp.dentry.mu.Unlock()
+		vfs.delayDecRef(mp)
+		return linuxerr.EINVAL
+	}
+	return vfs.attachTreeLocked(ctx, mnt, mp)
 }
 
 // lockMountpoint returns VirtualDentry with a locked Dentry. If vd is a
@@ -500,6 +526,24 @@ func (vfs *VirtualFilesystem) BindAt(ctx context.Context, creds *auth.Credential
 
 	vfs.lockMounts()
 	defer vfs.unlockMounts(ctx)
+	mp, err := vfs.lockMountpoint(targetVd)
+	if err != nil {
+		return err
+	}
+	cleanup := cleanup.Make(func() {
+		mp.dentry.mu.Unlock()
+		vfs.delayDecRef(mp) // +checklocksforce
+	})
+	defer cleanup.Clean()
+	// Namespace mounts can be binded to other mount points.
+	fsName := sourceVd.mount.Filesystem().FilesystemType().Name()
+	if !vfs.validInMountNS(ctx, sourceVd.mount) && fsName != nsfsName && fsName != cgroupFsName {
+		return linuxerr.EINVAL
+	}
+	if !vfs.validInMountNS(ctx, mp.mount) {
+		return linuxerr.EINVAL
+	}
+
 	var clone *Mount
 	if recursive {
 		clone, err = vfs.cloneMountTree(ctx, sourceVd.mount, sourceVd.dentry, 0, nil)
@@ -507,11 +551,12 @@ func (vfs *VirtualFilesystem) BindAt(ctx context.Context, creds *auth.Credential
 		clone, err = vfs.cloneMount(sourceVd.mount, sourceVd.dentry, nil, 0)
 	}
 	if err != nil {
-		vfs.delayDecRef(targetVd)
 		return err
 	}
+	cleanup.Release()
+
 	vfs.delayDecRef(clone)
-	if err := vfs.attachTreeLocked(ctx, clone, targetVd); err != nil {
+	if err := vfs.attachTreeLocked(ctx, clone, mp); err != nil {
 		vfs.abortUncomittedChildren(ctx, clone)
 		return err
 	}
@@ -528,11 +573,8 @@ func (vfs *VirtualFilesystem) RemountAt(ctx context.Context, creds *auth.Credent
 	vfs.lockMounts()
 	defer vfs.unlockMounts(ctx)
 	mnt := vd.Mount()
-	if mntns := MountNamespaceFromContext(ctx); mntns != nil {
-		vfs.delayDecRef(mntns)
-		if mntns != mnt.ns {
-			return linuxerr.EINVAL
-		}
+	if !vfs.validInMountNS(ctx, mnt) {
+		return linuxerr.EINVAL
 	}
 	return mnt.setMountOptions(opts)
 }
@@ -576,15 +618,11 @@ func (vfs *VirtualFilesystem) UmountAt(ctx context.Context, creds *auth.Credenti
 
 	vfs.lockMounts()
 	defer vfs.unlockMounts(ctx)
-	if mntns := MountNamespaceFromContext(ctx); mntns != nil {
-		vfs.delayDecRef(mntns)
-		if mntns != vd.mount.ns {
-			return linuxerr.EINVAL
-		}
-
-		if vd.mount == vd.mount.ns.root {
-			return linuxerr.EINVAL
-		}
+	if !vfs.validInMountNS(ctx, vd.mount) {
+		return linuxerr.EINVAL
+	}
+	if vd.mount == vd.mount.ns.root {
+		return linuxerr.EINVAL
 	}
 
 	if opts.Flags&linux.MNT_DETACH == 0 && vfs.arePropMountsBusy(vd.mount) {
@@ -899,7 +937,7 @@ func (mnt *Mount) LeakMessage() string {
 // This should only be set to true for debugging purposes, as it can generate an
 // extremely large amount of output and drastically degrade performance.
 func (mnt *Mount) LogRefs() bool {
-	return false
+	return true
 }
 
 // getMountAt returns the last Mount in the stack mounted at (mnt, d). It takes
@@ -1099,9 +1137,7 @@ func (vfs *VirtualFilesystem) PivotRoot(ctx context.Context, creds *auth.Credent
 		return newRoot, oldRoot, linuxerr.EINVAL
 	}
 	// The current root and the new root must be in the context's mount namespace.
-	ns := MountNamespaceFromContext(ctx)
-	vfs.delayDecRef(ns)
-	if oldRoot.mount.ns != ns || newRoot.mount.ns != ns {
+	if !vfs.validInMountNS(ctx, oldRoot.mount) || !vfs.validInMountNS(ctx, newRoot.mount) {
 		return newRoot, oldRoot, linuxerr.EINVAL
 	}
 	// The current root and the new root cannot be on the rootfs mount.
@@ -1125,11 +1161,11 @@ func (vfs *VirtualFilesystem) PivotRoot(ctx context.Context, creds *auth.Credent
 	rootMp := vfs.disconnectLocked(oldRoot.mount)
 
 	putOld.IncRef()
-	vfs.connectLocked(oldRoot.mount, putOld, ns)
+	vfs.connectLocked(oldRoot.mount, putOld, putOld.mount.ns)
 	putOld.dentry.mu.Unlock()
 
 	rootMp.dentry.mu.Lock()
-	vfs.connectLocked(newRoot.mount, rootMp, ns)
+	vfs.connectLocked(newRoot.mount, rootMp, rootMp.mount.ns)
 	rootMp.dentry.mu.Unlock()
 	vfs.mounts.seq.EndWrite()
 

pkg/sentry/vfs/propagation.go

@@ -266,7 +266,7 @@ func (vfs *VirtualFilesystem) peers(m1, m2 *Mount) bool {
 // +checklocks:vfs.mountMu
 func (vfs *VirtualFilesystem) propagateMount(ctx context.Context, dstMnt *Mount, dstPoint *Dentry, state *propState) error {
 	// Skip newly added mounts.
-	if dstMnt.neverConnected() {
+	if dstMnt.neverConnected() || dstMnt.umounted {
 		return nil
 	}
 	mp := VirtualDentry{mount: dstMnt, dentry: dstPoint}

test/syscalls/linux/BUILD

@@ -1406,6 +1406,7 @@ cc_binary(
     linkstatic = 1,
     deps = [
         "//test/util:capability_util",
+        "//test/util:cleanup",
         "//test/util:file_descriptor",
         "//test/util:fs_util",
         "@com_google_absl//absl/strings",

test/syscalls/linux/mount.cc

@@ -52,9 +52,11 @@
 #include "absl/strings/str_format.h"
 #include "absl/strings/str_split.h"
 #include "absl/strings/string_view.h"
+#include "absl/strings/substitute.h"
 #include "absl/time/clock.h"
 #include "absl/time/time.h"
 #include "test/util/capability_util.h"
+#include "test/util/cleanup.h"
 #include "test/util/eventfd_util.h"
 #include "test/util/file_descriptor.h"
 #include "test/util/fs_util.h"
@@ -2268,6 +2270,23 @@ TEST(MountTest, RemountUnmounted) {
               SyscallFailsWithErrno(EINVAL));
 }
 
+TEST(MountTest, DetachedMountBindFails) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
+
+  const TempPath path1 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  Cleanup mount_cleanup = ASSERT_NO_ERRNO_AND_VALUE(
+      Mount("", path1.path().c_str(), kTmpfs, 0, "", MNT_DETACH));
+  mount_cleanup.Release();
+  const FileDescriptor fd =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(path1.path().c_str(), O_RDONLY));
+  std::string fd_path = absl::Substitute("/proc/self/fd/$0", fd.get());
+  ASSERT_THAT(umount2(path1.path().c_str(), MNT_DETACH), SyscallSucceeds());
+  const TempPath path2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+
+  EXPECT_THAT(mount(fd_path.c_str(), path2.path().c_str(), "", MS_BIND, 0),
+              SyscallFailsWithErrno(EINVAL));
+}
+
 }  // namespace
 
 }  // namespace testing