From f857f268eceb1cdee0b2bdfa218c969c84033fcd Mon Sep 17 00:00:00 2001
From: Ayush Ranjan <ayushranjan@google.com>
Date: Fri, 29 Jul 2022 15:46:18 -0700
Subject: [PATCH] Add name length checks to renameat(2).

In Linux, the callers of fs/namei.c:do_renameat2() do this check
by calling fs/namei.c:getname().

This is important of consistency with Linux. Also, without this
we can see panics in overlayfs rename if new file's name is too
long. Because upperlayer RenameAt operation would succeed but
SetXattr operation will fail leading to an inconsistent state.

PiperOrigin-RevId: 464173793
---
 pkg/sentry/fsimpl/gofer/filesystem.go   |  3 +++
 pkg/sentry/fsimpl/kernfs/filesystem.go  |  3 +++
 pkg/sentry/fsimpl/overlay/filesystem.go |  3 +++
 pkg/sentry/fsimpl/tmpfs/filesystem.go   |  3 +++
 pkg/sentry/vfs/vfs.go                   |  4 ++++
 test/syscalls/linux/rename.cc           | 10 ++++++++++
 6 files changed, 26 insertions(+)

diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go
index d1af32204..9ef905d2e 100644
--- a/pkg/sentry/fsimpl/gofer/filesystem.go
+++ b/pkg/sentry/fsimpl/gofer/filesystem.go
@@ -1486,6 +1486,9 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 		}
 		return linuxerr.EBUSY
 	}
+	if len(newName) > MaxFilenameLen {
+		return linuxerr.ENAMETOOLONG
+	}
 	mnt := rp.Mount()
 	if mnt != oldParentVD.Mount() {
 		return linuxerr.EXDEV
diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go
index f42687218..3c3ea5687 100644
--- a/pkg/sentry/fsimpl/kernfs/filesystem.go
+++ b/pkg/sentry/fsimpl/kernfs/filesystem.go
@@ -707,6 +707,9 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 		}
 		return linuxerr.EBUSY
 	}
+	if len(newName) > linux.NAME_MAX {
+		return linuxerr.ENAMETOOLONG
+	}
 
 	err = checkCreateLocked(ctx, rp.Credentials(), newName, dstDir)
 	switch {
diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go
index 799e5c7e2..0bb8b439a 100644
--- a/pkg/sentry/fsimpl/overlay/filesystem.go
+++ b/pkg/sentry/fsimpl/overlay/filesystem.go
@@ -1081,6 +1081,9 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 		}
 		return linuxerr.EBUSY
 	}
+	// Do not check for newName length, since different filesystem
+	// implementations impose different name limits. upperfs.RenameAt() will fail
+	// appropriately if it has to.
 	mnt := rp.Mount()
 	if mnt != oldParentVD.Mount() {
 		return linuxerr.EXDEV
diff --git a/pkg/sentry/fsimpl/tmpfs/filesystem.go b/pkg/sentry/fsimpl/tmpfs/filesystem.go
index 6a2eab7a0..c03b97130 100644
--- a/pkg/sentry/fsimpl/tmpfs/filesystem.go
+++ b/pkg/sentry/fsimpl/tmpfs/filesystem.go
@@ -532,6 +532,9 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 		}
 		return linuxerr.EBUSY
 	}
+	if len(newName) > fs.maxFilenameLen {
+		return linuxerr.ENAMETOOLONG
+	}
 	mnt := rp.Mount()
 	if mnt != oldParentVD.Mount() {
 		return linuxerr.EXDEV
diff --git a/pkg/sentry/vfs/vfs.go b/pkg/sentry/vfs/vfs.go
index 0ce44038b..fd7a4da42 100644
--- a/pkg/sentry/vfs/vfs.go
+++ b/pkg/sentry/vfs/vfs.go
@@ -494,6 +494,10 @@ func (vfs *VirtualFilesystem) RenameAt(ctx context.Context, creds *auth.Credenti
 		oldParentVD.DecRef(ctx)
 		return linuxerr.EBUSY
 	}
+	if len(oldName) > linux.NAME_MAX {
+		oldParentVD.DecRef(ctx)
+		return linuxerr.ENAMETOOLONG
+	}
 
 	if !newpop.Path.Begin.Ok() {
 		oldParentVD.DecRef(ctx)
diff --git a/test/syscalls/linux/rename.cc b/test/syscalls/linux/rename.cc
index dcbc47a88..5ae9dc0d0 100644
--- a/test/syscalls/linux/rename.cc
+++ b/test/syscalls/linux/rename.cc
@@ -88,6 +88,16 @@ TEST(RenameTest, FileToSameDirectory) {
   EXPECT_THAT(Exists(newpath), IsPosixErrorOkAndHolds(true));
 }
 
+TEST(RenameTest, FileNameTooLong) {
+  auto old_file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+  auto new_base = NextTempBasename();
+  int padding = (NAME_MAX + 1) - new_base.size();
+  new_base.append(padding, 'x');
+  auto new_path = JoinPath(Dirname(old_file.path()), new_base);
+  ASSERT_THAT(rename(old_file.path().c_str(), new_path.c_str()),
+              SyscallFailsWithErrno(ENAMETOOLONG));
+}
+
 TEST(RenameTest, RenameAfterWritableFDAndChmod) {
   // Restore will require re-opening the writable FD which will fail.
   const DisableSave ds;