From 9085d334deed528d3f9336c9fd98cf3282e0eee0 Mon Sep 17 00:00:00 2001
From: Rahat Mahmood <rahat@google.com>
Date: Wed, 23 Mar 2022 17:23:57 -0700
Subject: [PATCH] kernfs: Handle duplicate unlink on orphaned directories.

Also don't print kernfs inode internals on panic, when we can't
acquire the necessary locks.

Reported-by: syzbot+101505e52936904e7d9f@syzkaller.appspotmail.com
Reported-by: syzbot+b9fe0fa83736b77030d2@syzkaller.appspotmail.com
PiperOrigin-RevId: 436869482
---
 pkg/sentry/fsimpl/kernfs/filesystem.go      |  9 +++++++++
 pkg/sentry/fsimpl/kernfs/inode_impl_util.go |  2 +-
 test/syscalls/linux/cgroup.cc               | 20 ++++++++++++++++++++
 test/util/cgroup_util.cc                    |  2 ++
 test/util/cgroup_util.h                     |  3 +++
 5 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go
index 363ebc466..9a132930a 100644
--- a/pkg/sentry/fsimpl/kernfs/filesystem.go
+++ b/pkg/sentry/fsimpl/kernfs/filesystem.go
@@ -242,6 +242,15 @@ func checkDeleteLocked(ctx context.Context, rp *vfs.ResolvingPath, d *Dentry) er
 	if parent.vfsd.IsDead() {
 		return linuxerr.ENOENT
 	}
+	if d.vfsd.IsDead() {
+		// This implies a duplicate unlink on an orphaned dentry, where the path
+		// resolution was successful. This is possible when the orphan is
+		// replaced by a new node of the same name (so the path resolution
+		// succeeds), and the orphan is unlinked again through a dirfd using
+		// unlinkat(2) (so the unlink refers to the orphan and not the new
+		// node). See Linux, fs/namei.c:do_rmdir().
+		return linuxerr.EINVAL
+	}
 	if err := parent.inode.CheckPermissions(ctx, rp.Credentials(), vfs.MayWrite|vfs.MayExec); err != nil {
 		return err
 	}
diff --git a/pkg/sentry/fsimpl/kernfs/inode_impl_util.go b/pkg/sentry/fsimpl/kernfs/inode_impl_util.go
index b96dc9ef7..5bea0a605 100644
--- a/pkg/sentry/fsimpl/kernfs/inode_impl_util.go
+++ b/pkg/sentry/fsimpl/kernfs/inode_impl_util.go
@@ -590,7 +590,7 @@ func (o *OrderedChildren) checkExistingLocked(name string, child Inode) error {
 		return linuxerr.ENOENT
 	}
 	if s.inode != child {
-		panic(fmt.Sprintf("Inode doesn't match what kernfs thinks! OrderedChild: %+v, kernfs: %+v", s.inode, child))
+		panic(fmt.Sprintf("Inode doesn't match what kernfs thinks! Name: %q, OrderedChild: %p, kernfs: %p", name, s.inode, child))
 	}
 	return nil
 }
diff --git a/test/syscalls/linux/cgroup.cc b/test/syscalls/linux/cgroup.cc
index 08ce85fd4..02a47b23f 100644
--- a/test/syscalls/linux/cgroup.cc
+++ b/test/syscalls/linux/cgroup.cc
@@ -403,6 +403,26 @@ TEST(Cgroup, MigrateToSubcontainerThread) {
   EXPECT_FALSE(tasks.contains(tid));
 }
 
+// Regression test for b/222278194.
+TEST(Cgroup, DuplicateUnlinkOnDirFD) {
+  SKIP_IF(!CgroupsAvailable());
+  Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()));
+  Cgroup c = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs(""));
+  Cgroup child = ASSERT_NO_ERRNO_AND_VALUE(c.CreateChild("child"));
+
+  // Orphan child directory by opening FD to it then deleting it.
+  const FileDescriptor dirfd =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(child.Path(), 0, 0));
+  ASSERT_NO_ERRNO(child.Delete());
+
+  // Replace orphan with new directory of same name, so path resolution
+  // succeeds.
+  Cgroup child_new = ASSERT_NO_ERRNO_AND_VALUE(c.CreateChild("child"));
+
+  // Attempt to delete orphaned child again through dirfd.
+  EXPECT_THAT(UnlinkAt(dirfd, ".", AT_REMOVEDIR), PosixErrorIs(EINVAL));
+}
+
 TEST(MemoryCgroup, MemoryUsageInBytes) {
   SKIP_IF(!CgroupsAvailable());
 
diff --git a/test/util/cgroup_util.cc b/test/util/cgroup_util.cc
index 4a88e0b1b..55dacd319 100644
--- a/test/util/cgroup_util.cc
+++ b/test/util/cgroup_util.cc
@@ -41,6 +41,8 @@ PosixErrorOr<Cgroup> Cgroup::Create(absl::string_view path) {
   return Cgroup(path);
 }
 
+PosixError Cgroup::Delete() { return Rmdir(cgroup_path_); }
+
 PosixErrorOr<Cgroup> Cgroup::CreateChild(absl::string_view name) const {
   return Cgroup::Create(JoinPath(Path(), name));
 }
diff --git a/test/util/cgroup_util.h b/test/util/cgroup_util.h
index 2781c0470..870b35714 100644
--- a/test/util/cgroup_util.h
+++ b/test/util/cgroup_util.h
@@ -43,6 +43,9 @@ class Cgroup {
   // cgroupfs directory.
   static PosixErrorOr<Cgroup> Create(std::string_view path);
 
+  // Deletes the current cgroup represented by this object.
+  PosixError Delete();
+
   const std::string& Path() const { return cgroup_path_; }
 
   // Creates a child cgroup under this cgroup with the given name.