From 7520b7f833a44a986edeb7ba916ff0309a8dcc67 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Sat, 5 Nov 2022 14:06:55 -0700 Subject: [PATCH] Modify pivot_root to return errors in cases of shared mounts. This is in line with linux as described here https://man7.org/linux/man-pages/man2/pivot_root.2.html PiperOrigin-RevId: 486388848 --- pkg/sentry/vfs/mount.go | 15 +++- test/syscalls/linux/pivot_root.cc | 111 ++++++++++++++++++++++++++---- 2 files changed, 111 insertions(+), 15 deletions(-) diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go index 400078a2e..7ba30a66e 100644 --- a/pkg/sentry/vfs/mount.go +++ b/pkg/sentry/vfs/mount.go @@ -1121,11 +1121,20 @@ retry: if rootVd.mount.ns != ns || newRootVd.mount.ns != ns { return linuxerr.EINVAL } - // TODO(gvisor.dev/issues/221): Update this function to disallow - // pivot_root-ing new_root/put_old mounts with MS_SHARED propagation once it - // is implemented in gVisor. vfs.mountMu.Lock() + // Either the mount point at new_root, or the parent mount of that mount + // point, has propagation type MS_SHARED. + if newRootParent := newRootVd.mount.parent(); newRootVd.mount.propType == Shared || newRootParent.propType == Shared { + vfs.mountMu.Unlock() + return linuxerr.EINVAL + } + // put_old is a mount point and has the propagation type MS_SHARED. + if putOldVd.mount.root == putOldVd.dentry && putOldVd.mount.propType == Shared { + vfs.mountMu.Unlock() + return linuxerr.EINVAL + } + if !vfs.mounts.seq.BeginWriteOk(epoch) { // Checks above raced with a mount change. vfs.mountMu.Unlock() diff --git a/test/syscalls/linux/pivot_root.cc b/test/syscalls/linux/pivot_root.cc index 7d5f32e81..d7a5c10e6 100644 --- a/test/syscalls/linux/pivot_root.cc +++ b/test/syscalls/linux/pivot_root.cc @@ -325,8 +325,8 @@ TEST(PivotRootTest, NewRootNotAMountpoint) { SyscallSucceeds()); const std::string mountpoint_path = absl::StrCat("/", Basename(mountpoint.path())); - auto new_root = ASSERT_NO_ERRNO_AND_VALUE( - TempPath::CreateDirIn(mountpoint.path())); + auto new_root = + ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(mountpoint.path())); const std::string new_root_path = absl::StrCat(mountpoint_path, "/", Basename(new_root.path())); auto put_old = @@ -336,8 +336,9 @@ TEST(PivotRootTest, NewRootNotAMountpoint) { const auto rest = [&] { TEST_CHECK_SUCCESS(chroot(root.path().c_str())); - TEST_CHECK_ERRNO(syscall( - __NR_pivot_root, new_root_path.c_str(), put_old_path.c_str()), EINVAL); + TEST_CHECK_ERRNO( + syscall(__NR_pivot_root, new_root_path.c_str(), put_old_path.c_str()), + EINVAL); }; EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0)); } @@ -349,16 +350,13 @@ TEST(PivotRootTest, PutOldNotUnderNewRoot) { auto root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); EXPECT_THAT(mount("", root.path().c_str(), "tmpfs", 0, "mode=0700"), SyscallSucceeds()); - auto new_root = - ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(root.path())); + auto new_root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(root.path())); const std::string new_root_path = absl::StrCat("/", Basename(new_root.path())); EXPECT_THAT(mount("", new_root.path().c_str(), "tmpfs", 0, "mode=0700"), SyscallSucceeds()); - auto put_old = - ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(root.path())); - const std::string put_old_path = - absl::StrCat("/", Basename(put_old.path())); + auto put_old = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(root.path())); + const std::string put_old_path = absl::StrCat("/", Basename(put_old.path())); EXPECT_THAT(mount("", put_old.path().c_str(), "tmpfs", 0, "mode=0700"), SyscallSucceeds()); @@ -388,8 +386,9 @@ TEST(PivotRootTest, CurrentRootNotAMountPoint) { const auto rest = [&] { TEST_CHECK_SUCCESS(chroot(root.path().c_str())); - TEST_CHECK_ERRNO(syscall( - __NR_pivot_root, new_root_path.c_str(), put_old_path.c_str()), EINVAL); + TEST_CHECK_ERRNO( + syscall(__NR_pivot_root, new_root_path.c_str(), put_old_path.c_str()), + EINVAL); }; EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0)); } @@ -414,6 +413,94 @@ TEST(PivotRootTest, OnRootFS) { EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0)); } +TEST(PivotRootTest, OnSharedNewRootParent) { + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN))); + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_CHROOT))); + + auto root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); + EXPECT_THAT(mount("", root.path().c_str(), "tmpfs", 0, "mode=0700"), + SyscallSucceeds()); + auto new_root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(root.path())); + EXPECT_THAT(mount("", new_root.path().c_str(), "tmpfs", 0, "mode=0700"), + SyscallSucceeds()); + const std::string new_root_path = JoinPath("/", Basename(new_root.path())); + auto put_old = + ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(new_root.path())); + const std::string put_old_path = + JoinPath(new_root_path, "/", Basename(put_old.path())); + + // Fails because parent has propagation type shared. + EXPECT_THAT(mount(nullptr, root.path().c_str(), nullptr, MS_SHARED, nullptr), + SyscallSucceeds()); + const auto rest = [&] { + TEST_CHECK_SUCCESS(chroot(root.path().c_str())); + TEST_CHECK_ERRNO( + syscall(__NR_pivot_root, new_root_path.c_str(), put_old_path.c_str()), + EINVAL); + }; + EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0)); +} + +TEST(PivotRootTest, OnSharedNewRoot) { + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN))); + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_CHROOT))); + + auto root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); + EXPECT_THAT(mount("", root.path().c_str(), "tmpfs", 0, "mode=0700"), + SyscallSucceeds()); + auto new_root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(root.path())); + EXPECT_THAT(mount("", new_root.path().c_str(), "tmpfs", 0, "mode=0700"), + SyscallSucceeds()); + const std::string new_root_path = JoinPath("/", Basename(new_root.path())); + auto put_old = + ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(new_root.path())); + const std::string put_old_path = + JoinPath(new_root_path, "/", Basename(put_old.path())); + + // Fails because new_root has propagation type shared. + EXPECT_THAT( + mount(nullptr, new_root.path().c_str(), nullptr, MS_SHARED, nullptr), + SyscallSucceeds()); + const auto rest = [&] { + TEST_CHECK_SUCCESS(chroot(root.path().c_str())); + TEST_CHECK_ERRNO( + syscall(__NR_pivot_root, new_root_path.c_str(), put_old_path.c_str()), + EINVAL); + }; + EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0)); +} + +TEST(PivotRootTest, OnSharedPutOldMountpoint) { + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN))); + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_CHROOT))); + + auto root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); + EXPECT_THAT(mount("", root.path().c_str(), "tmpfs", 0, "mode=0700"), + SyscallSucceeds()); + auto new_root = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(root.path())); + EXPECT_THAT(mount("", new_root.path().c_str(), "tmpfs", 0, "mode=0700"), + SyscallSucceeds()); + const std::string new_root_path = JoinPath("/", Basename(new_root.path())); + auto put_old = + ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(new_root.path())); + const std::string put_old_path = + JoinPath(new_root_path, "/", Basename(put_old.path())); + + // Fails because put_old is a mountpoint and has propagation type shared. + EXPECT_THAT(mount("", put_old.path().c_str(), "tmpfs", 0, "mode=0700"), + SyscallSucceeds()); + EXPECT_THAT( + mount(nullptr, put_old.path().c_str(), nullptr, MS_SHARED, nullptr), + SyscallSucceeds()); + const auto rest = [&] { + TEST_CHECK_SUCCESS(chroot(root.path().c_str())); + TEST_CHECK_ERRNO( + syscall(__NR_pivot_root, new_root_path.c_str(), put_old_path.c_str()), + EINVAL); + }; + EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0)); +} + } // namespace } // namespace testing