CVE-2023-7258: Insufficient Information (NVD-CWE-noinfo)
value: A denial of service exists in Gvisor Sandbox where a bug in reference counting code in mount point tracking could lead to a panic, making it possible for an attacker running as root and with permission to mount volumes to kill the sandbox. We recommend upgrading past commit 6a112c60a257dadac59962e0bc9e9b5aee70b5b6
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-05-15T17:15Z
Repository: https://github.com/google/gvisor
Language: Go
Files changed: pkg/sentry/vfs/mount.go, pkg/sentry/vfs/propagation.go, test/syscalls/linux/BUILD, test/syscalls/linux/mount.cc
Commit date: 2023-11-30 12:07:22 -0800
Commit message: 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

Diff: diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go
index 5249f44e9..b8785a6ce 100644
--- a/pkg/sentry/vfs/mount.go
+++ b/pkg/sentry/vfs/mount.go
@@ -31,11 +31,15 @@ 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
 // (Mount.fs), which applies to path resolution in the context of a particular
 // Mount (Mount.key.parent).
@@ -212,10 +216,23 @@ func (mnt *Mount) coveringMount() *Mount {
 		return nil
 	}
 	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
 // this function, mount flags are ignored. To set mount flags, pass them to a
 // corresponding ConnectMountAt.
@@ -251,24 +268,21 @@ func (vfs *VirtualFilesystem) MountDisconnected(ctx context.Context, creds *auth
 		return nil, err
 	}
 	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
 	// anon mounts and sentry-internal filesystems like pipefs.
 	if mp.mount.neverConnected() {
@@ -277,11 +291,14 @@ func (vfs *VirtualFilesystem) attachTreeLocked(ctx context.Context, mnt *Mount,
 	defer func() { mp.mount.ns.pending = 0 }()
 	if err := mp.mount.ns.checkMountCount(ctx, mnt); err != nil {
 		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
 		}
 		propMnts, err = vfs.doPropagation(ctx, mnt, mp)
@@ -323,11 +340,20 @@ func (vfs *VirtualFilesystem) ConnectMountAt(ctx context.Context, creds *auth.Cr
 	if err != nil {
 		return err
 	}
 	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
 // mountpoint, the method returns a VirtualDentry with a locked Dentry that is
 // the top most mount stacked on that Dentry. This method consumes a reference
@@ -498,22 +524,41 @@ func (vfs *VirtualFilesystem) BindAt(ctx context.Context, creds *auth.Credential
 		return err
 	}
 
 	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)
 	} else {
 		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
 	}
 	return nil
 }
@@ -526,15 +571,12 @@ func (vfs *VirtualFilesystem) RemountAt(ctx context.Context, creds *auth.Credent
 	}
 	defer vd.DecRef(ctx)
 	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)
 }
 
 // MountAt creates and mounts a Filesystem configured by the given arguments.
@@ -574,19 +616,15 @@ func (vfs *VirtualFilesystem) UmountAt(ctx context.Context, creds *auth.Credenti
 	}
 	defer vd.DecRef(ctx)
 
 	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) {
 		return linuxerr.EBUSY
 	}
@@ -897,11 +935,11 @@ func (mnt *Mount) LeakMessage() string {
 // LogRefs implements refs.CheckedObject.LogRefs.
 //
 // 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
 // a reference on the returned Mount. If (mnt, d) is not a mount point,
 // getMountAt returns nil.
@@ -1097,13 +1135,11 @@ func (vfs *VirtualFilesystem) PivotRoot(ctx context.Context, creds *auth.Credent
 	// (in the case it has been chrooted).
 	if oldRoot.mount.root != oldRoot.dentry {
 		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.
 	if oldRoot.mount.parent() == nil || newRoot.mount.parent() == nil {
 		return newRoot, oldRoot, linuxerr.EINVAL
@@ -1123,15 +1159,15 @@ func (vfs *VirtualFilesystem) PivotRoot(ctx context.Context, creds *auth.Credent
 	mp := vfs.disconnectLocked(newRoot.mount)
 	vfs.delayDecRef(mp)
 	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()
 
 	vfs.delayDecRef(newRoot.mount)
 	vfs.delayDecRef(oldRoot.mount)
diff --git a/pkg/sentry/vfs/propagation.go b/pkg/sentry/vfs/propagation.go
index ec7e64b5c..5abf9faac 100644
--- a/pkg/sentry/vfs/propagation.go
+++ b/pkg/sentry/vfs/propagation.go
@@ -264,11 +264,11 @@ func (vfs *VirtualFilesystem) peers(m1, m2 *Mount) bool {
 // This method is analogous to fs/pnode.c:propagate_one() in Linux.
 //
 // +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}
 	if !mp.mount.fs.Impl().IsDescendant(VirtualDentry{dstMnt, dstMnt.root}, mp) {
 		return nil
diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD
index 9f3f53920..0fbec1ad1 100644
--- a/test/syscalls/linux/BUILD
+++ b/test/syscalls/linux/BUILD
@@ -1404,10 +1404,11 @@ cc_binary(
     testonly = 1,
     srcs = ["mount.cc"],
     linkstatic = 1,
     deps = [
         "//test/util:capability_util",
+        "//test/util:cleanup",
         "//test/util:file_descriptor",
         "//test/util:fs_util",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/time",
         gtest,
diff --git a/test/syscalls/linux/mount.cc b/test/syscalls/linux/mount.cc
index dd243015c..d8cf9c2e8 100644
--- a/test/syscalls/linux/mount.cc
+++ b/test/syscalls/linux/mount.cc
@@ -50,13 +50,15 @@
 #include "absl/strings/numbers.h"
 #include "absl/strings/str_cat.h"
 #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"
 #include "test/util/linux_capability_util.h"
 #include "test/util/logging.h"
@@ -2266,9 +2268,26 @@ TEST(MountTest, RemountUnmounted) {
 
   EXPECT_THAT(mount("", dir.path().c_str(), kTmpfs, MS_REMOUNT, ""),
               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
 }  // namespace gvisor

Vulnerable code: const MountMax = 10000
// 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.
func (vfs *VirtualFilesystem) attachTreeLocked(ctx context.Context, mnt *Mount, vd VirtualDentry) error {
	mp, err := vfs.lockMountpoint(vd)
	if err != nil {
		return err
	}
		mp.DecRef(ctx)
	var propMnts map[*Mount]struct{}
	return vfs.attachTreeLocked(ctx, mnt, vd)
		vfs.delayDecRef(targetVd)
	if err := vfs.attachTreeLocked(ctx, clone, targetVd); err != nil {
	if mntns := MountNamespaceFromContext(ctx); mntns != nil {
		vfs.delayDecRef(mntns)
		if mntns != mnt.ns {
			return linuxerr.EINVAL
		}
	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
		}
	return false
	ns := MountNamespaceFromContext(ctx)
	vfs.delayDecRef(ns)
	if oldRoot.mount.ns != ns || newRoot.mount.ns != ns {
	vfs.connectLocked(oldRoot.mount, putOld, ns)
	vfs.connectLocked(newRoot.mount, rootMp, ns)
	if dstMnt.neverConnected() {
Fixed code: const (
	MountMax     = 10000
	nsfsName     = "nsfs"
	cgroupFsName = "cgroup"
)
// 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
}

// 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.
func (vfs *VirtualFilesystem) attachTreeLocked(ctx context.Context, mnt *Mount, mp VirtualDentry) error {
		vfs.delayDecRef(mp)
	var (
		propMnts map[*Mount]struct{}
		err      error
	)
	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)
	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
	}

	cleanup.Release()

	if err := vfs.attachTreeLocked(ctx, clone, mp); err != nil {
	if !vfs.validInMountNS(ctx, mnt) {
		return linuxerr.EINVAL
	if !vfs.validInMountNS(ctx, vd.mount) {
		return linuxerr.EINVAL
	}
	if vd.mount == vd.mount.ns.root {
		return linuxerr.EINVAL
	return true
	if !vfs.validInMountNS(ctx, oldRoot.mount) || !vfs.validInMountNS(ctx, newRoot.mount) {
	vfs.connectLocked(oldRoot.mount, putOld, putOld.mount.ns)
	vfs.connectLocked(newRoot.mount, rootMp, rootMp.mount.ns)
	if dstMnt.neverConnected() || dstMnt.umounted {
        "//test/util:cleanup",
#include "absl/strings/substitute.h"
#include "test/util/cleanup.h"
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));
}