From 283b80a456aaf4d8881301bd3bceb65fe7300185 Mon Sep 17 00:00:00 2001
From: Lucas Manning <lucasmanning@google.com>
Date: Tue, 16 May 2023 18:50:42 -0700
Subject: [PATCH] Fix logic bug in attaching mounts.

Before this fix, the vd provided to connectMountAt wouldn't be consumed if
we ran into a dead mount, the caller was responsible. This dead mount
could be rooted at a different vd than the one provided, which meant that we
were DecRef'ing the wrong vd if the method returned an error. The method now
always consumes the vd to simplify its use.

Reported-by: syzbot+e8a05606f54650464cc0@syzkaller.appspotmail.com
PiperOrigin-RevId: 532636615
---
 pkg/sentry/vfs/mount.go      | 25 +++++++++++++------------
 test/syscalls/linux/mount.cc | 21 +++++++++++++++++++++
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go
index 469431a21..8a350e0f4 100644
--- a/pkg/sentry/vfs/mount.go
+++ b/pkg/sentry/vfs/mount.go
@@ -276,18 +276,18 @@ func (vfs *VirtualFilesystem) ConnectMountAt(ctx context.Context, creds *auth.Cr
 	tree := vfs.preparePropagationTree(mnt, vd)
 	cleanup := cleanup.Make(func() {
 		vfs.abortPropagationTree(ctx, tree) // +checklocksforce
+	})
+	defer cleanup.Clean()
+	// Check if the new mount + all the propagation mounts puts us over the max.
+	if uint32(len(tree)+1)+vd.mount.ns.mounts > MountMax {
 		// We need to unlock mountMu first because DecRef takes a lock on the
 		// filesystem mutex in some implementations, which can lead to circular
 		// locking.
 		vfs.mountMu.Unlock()
 		vd.DecRef(ctx)
-	})
-	defer cleanup.Clean()
-	// Check if the new mount + all the propagation mounts puts us over the max.
-	if uint32(len(tree)+1)+vd.mount.ns.mounts > MountMax {
 		return linuxerr.ENOSPC
 	}
-	if err := vfs.connectMountAt(ctx, mnt, vd); err != nil {
+	if err := vfs.connectMountAtLocked(ctx, mnt, vd); err != nil {
 		return err
 	}
 	vfs.commitPropagationTree(ctx, tree)
@@ -296,20 +296,20 @@ func (vfs *VirtualFilesystem) ConnectMountAt(ctx context.Context, creds *auth.Cr
 	return nil
 }
 
-// connectMountAtLocked attaches mnt at vd. If the method returns an error that
-// is not nil, then it did not consume a reference on vd and the caller is
-// responsible for calling DecRef.
+// connectMountAtLocked attaches mnt at vd. This method consumes a reference on
+// vd.
 //
 // Preconditions:
 //   - mnt must be disconnected.
 //   - vfs.mountMu must be locked.
 //
 // +checklocks:vfs.mountMu
-func (vfs *VirtualFilesystem) connectMountAt(ctx context.Context, mnt *Mount, vd VirtualDentry) error {
+func (vfs *VirtualFilesystem) connectMountAtLocked(ctx context.Context, mnt *Mount, vd VirtualDentry) error {
 	vd.dentry.mu.Lock()
 	for {
 		if vd.mount.umounted || vd.dentry.dead {
 			vd.dentry.mu.Unlock()
+			vd.DecRef(ctx)
 			return linuxerr.ENOENT
 		}
 		// vd might have been mounted over between vfs.GetDentryAt() and
@@ -408,14 +408,15 @@ func (vfs *VirtualFilesystem) BindAt(ctx context.Context, creds *auth.Credential
 		// Checklocks doesn't work with anon functions.
 		vfs.setPropagation(clone, Private)  // +checklocksforce
 		vfs.abortPropagationTree(ctx, tree) // +checklocksforce
-		vfs.mountMu.Unlock()
-		targetVd.DecRef(ctx)
 	})
 	defer cleanup.Clean()
 	if uint32(1+len(tree))+targetVd.mount.ns.mounts > MountMax {
+		vfs.mountMu.Unlock()
+		targetVd.DecRef(ctx)
 		return nil, linuxerr.ENOSPC
 	}
-	if err := vfs.connectMountAt(ctx, clone, targetVd); err != nil {
+	if err := vfs.connectMountAtLocked(ctx, clone, targetVd); err != nil {
+		vfs.mountMu.Unlock()
 		return nil, err
 	}
 	vfs.commitPropagationTree(ctx, tree)
diff --git a/test/syscalls/linux/mount.cc b/test/syscalls/linux/mount.cc
index 21e5b7585..e9e8dbd66 100644
--- a/test/syscalls/linux/mount.cc
+++ b/test/syscalls/linux/mount.cc
@@ -24,6 +24,7 @@
 #include <unistd.h>
 
 #include <cstdint>
+#include <cstdlib>
 #include <functional>
 #include <iostream>
 #include <memory>
@@ -1418,6 +1419,26 @@ TEST(MountTest, MountInfoHasRoot) {
   }
 }
 
+TEST(MountTest, DeadMountsAreDecRefd) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
+  DisableSave ds;
+  std::string home = NewTempAbsPath();
+  ASSERT_NO_ERRNO(Mkdir(home));
+  ASSERT_THAT(chdir(home.c_str()), SyscallSucceeds());
+  constexpr char dirpath[] = "./file";
+
+  for (int i = 0; i < 10; ++i) {
+    const auto rest = [&] {
+      mkdir(dirpath, 0);
+      mount(dirpath, ".", 0, MS_BIND, 0);
+      rmdir(dirpath);
+      mkdir(dirpath, 0);
+      mount(dirpath, ".", 0, MS_BIND, 0);
+    };
+    EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0));
+  }
+}
+
 }  // namespace
 
 }  // namespace testing