From f5692f7dcc48a76a5d7b45cdf71b59be876adb42 Mon Sep 17 00:00:00 2001
From: Nicolas Lacasse <nlacasse@google.com>
Date: Wed, 24 Feb 2021 15:37:46 -0800
Subject: Kernfs should not try to rename a file to itself.

One precondition of VFS.PrepareRenameAt is that the `from` and `to` dentries
are not the same. Kernfs was not checking this, which could lead to a deadlock.

PiperOrigin-RevId: 359385974
---
 pkg/sentry/fsimpl/kernfs/filesystem.go | 23 ++++++++++++++---------
 test/syscalls/linux/rename.cc          | 14 ++++++++++++++
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go
index beb9302f6..badca4d9f 100644
--- a/pkg/sentry/fsimpl/kernfs/filesystem.go
+++ b/pkg/sentry/fsimpl/kernfs/filesystem.go
@@ -668,14 +668,14 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 
 	// Can we create the dst dentry?
 	var dst *Dentry
-	pc := rp.Component()
-	if pc == "." || pc == ".." {
+	newName := rp.Component()
+	if newName == "." || newName == ".." {
 		if noReplace {
 			return syserror.EEXIST
 		}
 		return syserror.EBUSY
 	}
-	switch err := checkCreateLocked(ctx, rp.Credentials(), pc, dstDir); err {
+	switch err := checkCreateLocked(ctx, rp.Credentials(), newName, dstDir); err {
 	case nil:
 		// Ok, continue with rename as replacement.
 	case syserror.EEXIST:
@@ -683,13 +683,18 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 			// Won't overwrite existing node since RENAME_NOREPLACE was requested.
 			return syserror.EEXIST
 		}
-		dst = dstDir.children[pc]
+		dst = dstDir.children[newName]
 		if dst == nil {
-			panic(fmt.Sprintf("Child %q for parent Dentry %+v disappeared inside atomic section?", pc, dstDir))
+			panic(fmt.Sprintf("Child %q for parent Dentry %+v disappeared inside atomic section?", newName, dstDir))
 		}
 	default:
 		return err
 	}
+
+	if srcDir == dstDir && oldName == newName {
+		return nil
+	}
+
 	var dstVFSD *vfs.Dentry
 	if dst != nil {
 		dstVFSD = dst.VFSDentry()
@@ -712,7 +717,7 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 	if err := virtfs.PrepareRenameDentry(mntns, srcVFSD, dstVFSD); err != nil {
 		return err
 	}
-	err = srcDir.inode.Rename(ctx, src.name, pc, src.inode, dstDir.inode)
+	err = srcDir.inode.Rename(ctx, src.name, newName, src.inode, dstDir.inode)
 	if err != nil {
 		virtfs.AbortRenameDentry(srcVFSD, dstVFSD)
 		return err
@@ -723,12 +728,12 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 		dstDir.IncRef()        // child (src) takes a ref on the new parent.
 	}
 	src.parent = dstDir
-	src.name = pc
+	src.name = newName
 	if dstDir.children == nil {
 		dstDir.children = make(map[string]*Dentry)
 	}
-	replaced := dstDir.children[pc]
-	dstDir.children[pc] = src
+	replaced := dstDir.children[newName]
+	dstDir.children[newName] = src
 	var replaceVFSD *vfs.Dentry
 	if replaced != nil {
 		// deferDecRef so that fs.mu and dstDir.mu are unlocked by then.
diff --git a/test/syscalls/linux/rename.cc b/test/syscalls/linux/rename.cc
index 22c8c19cf..b1a813de0 100644
--- a/test/syscalls/linux/rename.cc
+++ b/test/syscalls/linux/rename.cc
@@ -424,6 +424,20 @@ TEST(RenameTest, SysfsPathEndingWithDots) {
               SyscallFailsWithErrno(EBUSY));
 }
 
+TEST(RenameTest, SysfsFileToSelf) {
+  // If a non-root user tries to rename inside /sys then we get EPERM.
+  SKIP_IF(geteuid() != 0);
+  std::string const path = "/sys/devices/system/cpu/online";
+  EXPECT_THAT(rename(path.c_str(), path.c_str()), SyscallSucceeds());
+}
+
+TEST(RenameTest, SysfsDirectoryToSelf) {
+  // If a non-root user tries to rename inside /sys then we get EPERM.
+  SKIP_IF(geteuid() != 0);
+  std::string const path = "/sys/devices";
+  EXPECT_THAT(rename(path.c_str(), path.c_str()), SyscallSucceeds());
+}
+
 }  // namespace
 
 }  // namespace testing
-- 
cgit v1.2.3