From f229b3e772417b8a075eda7c444cc76ead29f955 Mon Sep 17 00:00:00 2001
From: Lucas Manning <lucasmanning@google.com>
Date: Thu, 19 Sep 2024 11:16:03 -0700
Subject: [PATCH] Fix small logic bug with CLONE_NEWUSER|CLONE_NEWNS in clone.

The new mount namespace was being created with the old user namespace,
not the new one. This led to permission errors when creating new mounts.

PiperOrigin-RevId: 676489580
---
 pkg/sentry/kernel/task_clone.go |  4 ++--
 pkg/sentry/vfs/namespace.go     |  4 ++--
 test/syscalls/linux/BUILD       |  1 +
 test/syscalls/linux/fork.cc     | 28 +++++++++++++++++++++++++++-
 4 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/pkg/sentry/kernel/task_clone.go b/pkg/sentry/kernel/task_clone.go
index 635b4f995..72a2c0322 100644
--- a/pkg/sentry/kernel/task_clone.go
+++ b/pkg/sentry/kernel/task_clone.go
@@ -201,7 +201,7 @@ func (t *Task) Clone(args *linux.CloneArgs) (ThreadID, *SyscallControl, error) {
 	mntns := t.mountNamespace
 	if args.Flags&linux.CLONE_NEWNS != 0 {
 		var err error
-		mntns, err = t.k.vfs.CloneMountNamespace(t, creds, mntns, &fsContext.root, &fsContext.cwd, t.k)
+		mntns, err = t.k.vfs.CloneMountNamespace(t, userns, mntns, &fsContext.root, &fsContext.cwd, t.k)
 		if err != nil {
 			return 0, nil, err
 		}
@@ -666,7 +666,7 @@ func (t *Task) Unshare(flags int32) error {
 			return linuxerr.EPERM
 		}
 		oldMountNS := t.mountNamespace
-		mntns, err := t.k.vfs.CloneMountNamespace(t, creds, oldMountNS, &t.fsContext.root, &t.fsContext.cwd, t.k)
+		mntns, err := t.k.vfs.CloneMountNamespace(t, creds.UserNamespace, oldMountNS, &t.fsContext.root, &t.fsContext.cwd, t.k)
 		if err != nil {
 			return err
 		}
diff --git a/pkg/sentry/vfs/namespace.go b/pkg/sentry/vfs/namespace.go
index 1d3feaf18..49a4689c1 100644
--- a/pkg/sentry/vfs/namespace.go
+++ b/pkg/sentry/vfs/namespace.go
@@ -158,14 +158,14 @@ type NamespaceInodeGetter interface {
 // with proper mounts from the new namespace.
 func (vfs *VirtualFilesystem) CloneMountNamespace(
 	ctx context.Context,
-	creds *auth.Credentials,
+	uns *auth.UserNamespace,
 	ns *MountNamespace,
 	root *VirtualDentry,
 	cwd *VirtualDentry,
 	nsfs NamespaceInodeGetter,
 ) (*MountNamespace, error) {
 	newns := &MountNamespace{
-		Owner:       creds.UserNamespace,
+		Owner:       uns,
 		mountpoints: make(map[*Dentry]uint32),
 	}
 
diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD
index 6a09c7c75..76a0f082b 100644
--- a/test/syscalls/linux/BUILD
+++ b/test/syscalls/linux/BUILD
@@ -904,6 +904,7 @@ cc_binary(
         "//test/util:capability_util",
         "//test/util:logging",
         "//test/util:memory_util",
+        "//test/util:posix_error",
         "//test/util:test_main",
         "//test/util:test_util",
         "//test/util:thread_util",
diff --git a/test/syscalls/linux/fork.cc b/test/syscalls/linux/fork.cc
index 93df86199..c5abcfb92 100644
--- a/test/syscalls/linux/fork.cc
+++ b/test/syscalls/linux/fork.cc
@@ -17,6 +17,7 @@
 #include <sched.h>
 #include <stdlib.h>
 #include <sys/mman.h>
+#include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
@@ -31,9 +32,10 @@
 #include "gtest/gtest.h"
 #include "absl/time/clock.h"
 #include "absl/time/time.h"
-#include "test/util/capability_util.h"
+#include "test/util/linux_capability_util.h"
 #include "test/util/logging.h"
 #include "test/util/memory_util.h"
+#include "test/util/posix_error.h"
 #include "test/util/test_util.h"
 #include "test/util/thread_util.h"
 
@@ -435,6 +437,30 @@ TEST(CloneTest, NewUserNamespacePermitsAllOtherNamespaces) {
       << "status = " << status;
 }
 
+TEST(CloneTest, NewUserMountNamespace) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace()));
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
+  Mapping child_stack = ASSERT_NO_ERRNO_AND_VALUE(
+      MmapAnon(kPageSize, PROT_READ | PROT_WRITE, MAP_PRIVATE));
+  int child_pid;
+  ASSERT_THAT(child_pid = clone(
+                  +[](void*) {
+                    TEST_CHECK_SUCCESS(mount(nullptr, "/", nullptr,
+                                             MS_REC | MS_PRIVATE, nullptr));
+                    return 0;
+                  },
+                  reinterpret_cast<void*>(child_stack.addr() + kPageSize),
+                  CLONE_NEWUSER | CLONE_NEWIPC | CLONE_NEWNET | CLONE_NEWUTS |
+                      SIGCHLD | CLONE_NEWNS,
+                  /* arg = */ nullptr),
+              SyscallSucceeds());
+  int status;
+  ASSERT_THAT(waitpid(child_pid, &status, 0),
+              SyscallSucceedsWithValue(child_pid));
+  EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0)
+      << "status = " << status;
+}
+
 // Clone with CLONE_SETTLS and a non-canonical TLS address is rejected.
 TEST(CloneTest, NonCanonicalTLS) {
   constexpr uintptr_t kNonCanonical = 1ull << 63;