Break /proc/[pid]/{uid,gid}_map's dependence on seqfile.

In addition to simplifying the implementation, this fixes two bugs:

- seqfile.NewSeqFile unconditionally creates an inode with mode 0444,
  but {uid,gid}_map have mode 0644.

- idMapSeqFile.Write implements fs.FileOperations.Write ... but it
  doesn't implement any other fs.FileOperations methods and is never
  used as fs.FileOperations. idMapSeqFile.GetFile() =>
  seqfile.SeqFile.GetFile() uses seqfile.seqFileOperations instead,
  which rejects all writes.

PiperOrigin-RevId: 234638212
Change-Id: I4568f741ab07929273a009d7e468c8205a8541bc

4 files changed, 300 insertions, 73 deletions

diff --git a/pkg/sentry/fs/proc/uid_gid_map.go b/pkg/sentry/fs/proc/uid_gid_map.go
index 815c40b7f..d6e278f79 100644
--- a/pkg/sentry/fs/proc/uid_gid_map.go
+++ b/pkg/sentry/fs/proc/uid_gid_map.go
@@ -17,67 +17,42 @@ package proc
 import (
 	"bytes"
 	"fmt"
+	"io"
 
+	"gvisor.googlesource.com/gvisor/pkg/abi/linux"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/context"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/fs"
-	"gvisor.googlesource.com/gvisor/pkg/sentry/fs/proc/seqfile"
+	"gvisor.googlesource.com/gvisor/pkg/sentry/fs/fsutil"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/kernel"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/kernel/auth"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/usermem"
 	"gvisor.googlesource.com/gvisor/pkg/syserror"
+	"gvisor.googlesource.com/gvisor/pkg/waiter"
 )
 
-// An idMapSeqSource is a seqfile.SeqSource that returns UID or GID mappings
-// from a task's user namespace.
+// idMapInodeOperations implements fs.InodeOperations for
+// /proc/[pid]/{uid,gid}_map.
 //
 // +stateify savable
-type idMapSeqSource struct {
+type idMapInodeOperations struct {
+	fsutil.InodeGenericChecker `state:"nosave"`
+	fsutil.InodeNoopRelease    `state:"nosave"`
+	fsutil.InodeNoopWriteOut   `state:"nosave"`
+	fsutil.InodeNotDirectory   `state:"nosave"`
+	fsutil.InodeNotMappable    `state:"nosave"`
+	fsutil.InodeNotSocket      `state:"nosave"`
+	fsutil.InodeNotSymlink     `state:"nosave"`
+	fsutil.InodeNotTruncatable `state:"nosave"`
+	fsutil.InodeVirtual        `state:"nosave"`
+
+	fsutil.InodeSimpleAttributes
+	fsutil.InodeSimpleExtendedAttributes
+
 	t    *kernel.Task
 	gids bool
 }
 
-// NeedsUpdate implements seqfile.SeqSource.NeedsUpdate.
-func (imss *idMapSeqSource) NeedsUpdate(generation int64) bool {
-	return true
-}
-
-// ReadSeqFileData implements seqfile.SeqSource.ReadSeqFileData.
-func (imss *idMapSeqSource) ReadSeqFileData(ctx context.Context, handle seqfile.SeqHandle) ([]seqfile.SeqData, int64) {
-	var start int
-	if handle != nil {
-		start = handle.(*idMapSeqHandle).value
-	}
-	var entries []auth.IDMapEntry
-	if imss.gids {
-		entries = imss.t.UserNamespace().GIDMap()
-	} else {
-		entries = imss.t.UserNamespace().UIDMap()
-	}
-	var data []seqfile.SeqData
-	i := 1
-	for _, e := range entries {
-		if i > start {
-			data = append(data, seqfile.SeqData{
-				Buf:    idMapLineFromEntry(e),
-				Handle: &idMapSeqHandle{i},
-			})
-		}
-		i++
-	}
-	return data, 0
-}
-
-// TODO: Fix issue requiring idMapSeqHandle wrapping an int.
-//
-// +stateify savable
-type idMapSeqHandle struct {
-	value int
-}
-
-// +stateify savable
-type idMapSeqFile struct {
-	seqfile.SeqFile
-}
+var _ fs.InodeOperations = (*idMapInodeOperations)(nil)
 
 // newUIDMap returns a new uid_map file.
 func newUIDMap(t *kernel.Task, msrc *fs.MountSource) *fs.Inode {
@@ -90,25 +65,64 @@ func newGIDMap(t *kernel.Task, msrc *fs.MountSource) *fs.Inode {
 }
 
 func newIDMap(t *kernel.Task, msrc *fs.MountSource, gids bool) *fs.Inode {
-	imsf := &idMapSeqFile{
-		*seqfile.NewSeqFile(t, &idMapSeqSource{
-			t:    t,
-			gids: gids,
-		}),
-	}
-	return newProcInode(imsf, msrc, fs.SpecialFile, t)
+	return newProcInode(&idMapInodeOperations{
+		InodeSimpleAttributes: fsutil.NewInodeSimpleAttributes(t, fs.RootOwner, fs.FilePermsFromMode(0644), linux.PROC_SUPER_MAGIC),
+		t:                     t,
+		gids:                  gids,
+	}, msrc, fs.SpecialFile, t)
 }
 
-func (imsf *idMapSeqFile) source() *idMapSeqSource {
-	return imsf.SeqFile.SeqSource.(*idMapSeqSource)
+// GetFile implements fs.InodeOperations.GetFile.
+func (imio *idMapInodeOperations) GetFile(ctx context.Context, dirent *fs.Dirent, flags fs.FileFlags) (*fs.File, error) {
+	return fs.NewFile(ctx, dirent, flags, &idMapFileOperations{
+		iops: imio,
+	}), nil
 }
 
+// +stateify savable
+type idMapFileOperations struct {
+	waiter.AlwaysReady       `state:"nosave"`
+	fsutil.FileGenericSeek   `state:"nosave"`
+	fsutil.FileNoIoctl       `state:"nosave"`
+	fsutil.FileNoMMap        `state:"nosave"`
+	fsutil.FileNoopFlush     `state:"nosave"`
+	fsutil.FileNoopFsync     `state:"nosave"`
+	fsutil.FileNoopRelease   `state:"nosave"`
+	fsutil.FileNotDirReaddir `state:"nosave"`
+
+	iops *idMapInodeOperations
+}
+
+var _ fs.FileOperations = (*idMapFileOperations)(nil)
+
 // "There is an (arbitrary) limit on the number of lines in the file. As at
 // Linux 3.18, the limit is five lines." - user_namespaces(7)
 const maxIDMapLines = 5
 
+// Read implements fs.FileOperations.Read.
+func (imfo *idMapFileOperations) Read(ctx context.Context, file *fs.File, dst usermem.IOSequence, offset int64) (int64, error) {
+	if offset < 0 {
+		return 0, syserror.EINVAL
+	}
+	var entries []auth.IDMapEntry
+	if imfo.iops.gids {
+		entries = imfo.iops.t.UserNamespace().GIDMap()
+	} else {
+		entries = imfo.iops.t.UserNamespace().UIDMap()
+	}
+	var buf bytes.Buffer
+	for _, e := range entries {
+		fmt.Fprintf(&buf, "%10d %10d %10d\n", e.FirstID, e.FirstParentID, e.Length)
+	}
+	if offset >= int64(buf.Len()) {
+		return 0, io.EOF
+	}
+	n, err := dst.CopyOut(ctx, buf.Bytes()[offset:])
+	return int64(n), err
+}
+
 // Write implements fs.FileOperations.Write.
-func (imsf *idMapSeqFile) Write(ctx context.Context, _ *fs.File, src usermem.IOSequence, offset int64) (int64, error) {
+func (imfo *idMapFileOperations) Write(ctx context.Context, file *fs.File, src usermem.IOSequence, offset int64) (int64, error) {
 	// "In addition, the number of bytes written to the file must be less than
 	// the system page size, and the write must be performed at the start of
 	// the file ..." - user_namespaces(7)
@@ -126,33 +140,21 @@ func (imsf *idMapSeqFile) Write(ctx context.Context, _ *fs.File, src usermem.IOS
 	}
 	entries := make([]auth.IDMapEntry, len(lines))
 	for i, l := range lines {
-		e, err := idMapEntryFromLine(string(l))
+		var e auth.IDMapEntry
+		_, err := fmt.Sscan(string(l), &e.FirstID, &e.FirstParentID, &e.Length)
 		if err != nil {
 			return 0, syserror.EINVAL
 		}
 		entries[i] = e
 	}
-	t := imsf.source().t
 	var err error
-	if imsf.source().gids {
-		err = t.UserNamespace().SetGIDMap(ctx, entries)
+	if imfo.iops.gids {
+		err = imfo.iops.t.UserNamespace().SetGIDMap(ctx, entries)
 	} else {
-		err = t.UserNamespace().SetUIDMap(ctx, entries)
+		err = imfo.iops.t.UserNamespace().SetUIDMap(ctx, entries)
 	}
 	if err != nil {
 		return 0, err
 	}
 	return int64(len(b)), nil
 }
-
-func idMapLineFromEntry(e auth.IDMapEntry) []byte {
-	var b bytes.Buffer
-	fmt.Fprintf(&b, "%10d %10d %10d\n", e.FirstID, e.FirstParentID, e.Length)
-	return b.Bytes()
-}
-
-func idMapEntryFromLine(line string) (auth.IDMapEntry, error) {
-	var e auth.IDMapEntry
-	_, err := fmt.Sscan(line, &e.FirstID, &e.FirstParentID, &e.Length)
-	return e, err
-}
diff --git a/test/syscalls/BUILD b/test/syscalls/BUILD
index ca69f3309..1be7a9bd4 100644
--- a/test/syscalls/BUILD
+++ b/test/syscalls/BUILD
@@ -214,6 +214,8 @@ syscall_test(
     test = "//test/syscalls/linux:proc_test",
 )
 
+syscall_test(test = "//test/syscalls/linux:proc_pid_uid_gid_map_test")
+
 syscall_test(
     size = "medium",
     test = "//test/syscalls/linux:pselect_test",
diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD
index 75fa52a57..3c61c48ef 100644
--- a/test/syscalls/linux/BUILD
+++ b/test/syscalls/linux/BUILD
@@ -1445,6 +1445,25 @@ cc_binary(
 )
 
 cc_binary(
+    name = "proc_pid_uid_gid_map_test",
+    testonly = 1,
+    srcs = ["proc_pid_uid_gid_map.cc"],
+    linkstatic = 1,
+    deps = [
+        "//test/util:capability_util",
+        "//test/util:fs_util",
+        "//test/util:logging",
+        "//test/util:multiprocess_util",
+        "//test/util:posix_error",
+        "//test/util:save_util",
+        "//test/util:test_main",
+        "//test/util:test_util",
+        "@com_google_absl//absl/strings",
+        "@com_google_googletest//:gtest",
+    ],
+)
+
+cc_binary(
     name = "pselect_test",
     testonly = 1,
     srcs = ["pselect.cc"],
diff --git a/test/syscalls/linux/proc_pid_uid_gid_map.cc b/test/syscalls/linux/proc_pid_uid_gid_map.cc
new file mode 100644
index 000000000..bf0f8b2bb
--- /dev/null
+++ b/test/syscalls/linux/proc_pid_uid_gid_map.cc
@@ -0,0 +1,204 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <fcntl.h>
+#include <sched.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include <functional>
+#include <string>
+#include <vector>
+
+#include "gtest/gtest.h"
+#include "absl/strings/str_cat.h"
+#include "absl/strings/str_split.h"
+#include "test/util/capability_util.h"
+#include "test/util/fs_util.h"
+#include "test/util/logging.h"
+#include "test/util/multiprocess_util.h"
+#include "test/util/posix_error.h"
+#include "test/util/save_util.h"
+#include "test/util/test_util.h"
+
+namespace gvisor {
+namespace testing {
+
+PosixErrorOr<int> InNewUserNamespace(const std::function<void()>& fn) {
+  return InForkedProcess([&] {
+    TEST_PCHECK(unshare(CLONE_NEWUSER) == 0);
+    MaybeSave();
+    fn();
+  });
+}
+
+// TEST_CHECK-fails on error, since this function is used in contexts that
+// require async-signal-safety.
+void DenySelfSetgroups() {
+  int fd = open("/proc/self/setgroups", O_WRONLY);
+  if (fd < 0 && errno == ENOENT) {
+    // On kernels where this file doesn't exist, writing "deny" to it isn't
+    // necessary to write to gid_map.
+    return;
+  }
+  TEST_PCHECK(fd >= 0);
+  MaybeSave();
+  char deny[] = "deny";
+  TEST_PCHECK(write(fd, deny, sizeof(deny)) == sizeof(deny));
+  MaybeSave();
+  TEST_PCHECK(close(fd) == 0);
+}
+
+// Returns a valid UID/GID that isn't id.
+uint32_t another_id(uint32_t id) { return (id + 1) % 65535; }
+
+struct TestParam {
+  std::string desc;
+  std::string map_filename;
+  int cap;
+  std::function<uint32_t()> get_current_id;
+};
+
+std::string DescribeTestParam(const ::testing::TestParamInfo<TestParam>& info) {
+  return info.param.desc;
+}
+
+class ProcSelfUidGidMapTest : public ::testing::TestWithParam<TestParam> {
+ protected:
+  PosixErrorOr<int> InNewUserNamespaceWithMapFD(
+      const std::function<void(int)>& fn) {
+    std::string map_filename = GetParam().map_filename;
+    return InNewUserNamespace([&] {
+      int fd = open(map_filename.c_str(), O_RDWR);
+      TEST_PCHECK(fd >= 0);
+      MaybeSave();
+      fn(fd);
+      TEST_PCHECK(close(fd) == 0);
+    });
+  }
+
+  uint32_t CurrentID() { return GetParam().get_current_id(); }
+
+  PosixErrorOr<bool> HaveSetIDCapability() {
+    return HaveCapability(GetParam().cap);
+  }
+
+  // Returns true if the caller is running in a user namespace with all IDs
+  // mapped. This matters for tests that expect to successfully map arbitrary
+  // IDs into a child user namespace, since even with CAP_SET*ID this is only
+  // possible if those IDs are mapped into the current one.
+  PosixErrorOr<bool> AllIDsMapped() {
+    ASSIGN_OR_RETURN_ERRNO(std::string id_map, GetContents(GetParam().map_filename));
+    std::vector<std::string> id_map_parts =
+        absl::StrSplit(id_map, ' ', absl::SkipEmpty());
+    return id_map_parts == std::vector<std::string>({"0", "0", "4294967295"});
+  }
+};
+
+TEST_P(ProcSelfUidGidMapTest, IsInitiallyEmpty) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace()));
+  EXPECT_THAT(InNewUserNamespaceWithMapFD([](int fd) {
+                char buf[64];
+                TEST_PCHECK(read(fd, buf, sizeof(buf)) == 0);
+              }),
+              IsPosixErrorOkAndHolds(0));
+}
+
+TEST_P(ProcSelfUidGidMapTest, IdentityMapOwnID) {
+  // This is the only write permitted if the writer does not have CAP_SET*ID.
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace()));
+  uint32_t id = CurrentID();
+  std::string line = absl::StrCat(id, " ", id, " 1");
+  EXPECT_THAT(
+      InNewUserNamespaceWithMapFD([&](int fd) {
+        DenySelfSetgroups();
+        TEST_PCHECK(write(fd, line.c_str(), line.size()) == line.size());
+      }),
+      IsPosixErrorOkAndHolds(0));
+}
+
+TEST_P(ProcSelfUidGidMapTest, NonIdentityMapOwnID) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace()));
+  SKIP_IF(ASSERT_NO_ERRNO_AND_VALUE(HaveSetIDCapability()));
+  uint32_t id = CurrentID();
+  uint32_t id2 = another_id(id);
+  std::string line = absl::StrCat(id2, " ", id, " 1");
+  EXPECT_THAT(
+      InNewUserNamespaceWithMapFD([&](int fd) {
+        DenySelfSetgroups();
+        TEST_PCHECK(write(fd, line.c_str(), line.size()) == line.size());
+      }),
+      IsPosixErrorOkAndHolds(0));
+}
+
+TEST_P(ProcSelfUidGidMapTest, MapOtherIDUnprivileged) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace()));
+  SKIP_IF(ASSERT_NO_ERRNO_AND_VALUE(HaveSetIDCapability()));
+  uint32_t id = CurrentID();
+  uint32_t id2 = another_id(id);
+  std::string line = absl::StrCat(id, " ", id2, " 1");
+  EXPECT_THAT(InNewUserNamespaceWithMapFD([&](int fd) {
+                DenySelfSetgroups();
+                TEST_PCHECK(write(fd, line.c_str(), line.size()) < 0);
+                TEST_CHECK(errno == EPERM);
+              }),
+              IsPosixErrorOkAndHolds(0));
+}
+
+TEST_P(ProcSelfUidGidMapTest, MapOtherIDPrivileged) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace()));
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveSetIDCapability()));
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(AllIDsMapped()));
+  uint32_t id = CurrentID();
+  uint32_t id2 = another_id(id);
+  std::string line = absl::StrCat(id, " ", id2, " 1");
+  EXPECT_THAT(
+      InNewUserNamespaceWithMapFD([&](int fd) {
+        DenySelfSetgroups();
+        TEST_PCHECK(write(fd, line.c_str(), line.size()) == line.size());
+      }),
+      IsPosixErrorOkAndHolds(0));
+}
+
+TEST_P(ProcSelfUidGidMapTest, MapAnyIDsPrivileged) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace()));
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveSetIDCapability()));
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(AllIDsMapped()));
+  // Test all of:
+  //
+  // - Mapping ranges of length > 1
+  //
+  // - Mapping multiple ranges
+  //
+  // - Non-identity mappings
+  char entries[] = "2 0 2\n4 6 2";
+  EXPECT_THAT(
+      InNewUserNamespaceWithMapFD([&](int fd) {
+        DenySelfSetgroups();
+        TEST_PCHECK(write(fd, entries, sizeof(entries)) == sizeof(entries));
+      }),
+      IsPosixErrorOkAndHolds(0));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    All, ProcSelfUidGidMapTest,
+    ::testing::Values(TestParam{"UID", "/proc/self/uid_map", CAP_SETUID,
+                                []() -> uint32_t { return getuid(); }},
+                      TestParam{"GID", "/proc/self/gid_map", CAP_SETGID,
+                                []() -> uint32_t { return getgid(); }}),
+    DescribeTestParam);
+
+}  // namespace testing
+}  // namespace gvisor