Second try at landing this patch list. This time, we are super careful about checking all return values from HANDLE_EINTR().

Original CL: https://chromiumcodereview.appspot.com/10545100/

TEST=make && ./demo32 && ./demo64
BUG=130662


Review URL: https://chromiumcodereview.appspot.com/10542149

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@142184 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 75 insertions, 25 deletions

diff --git a/sandbox/linux/seccomp-bpf/Makefile b/sandbox/linux/seccomp-bpf/Makefile
index 222af2a..6519122 100644
--- a/sandbox/linux/seccomp-bpf/Makefile
+++ b/sandbox/linux/seccomp-bpf/Makefile
@@ -1,5 +1,4 @@
-CFLAGS = -g -O3 -Wall -Werror -Wextra -Wno-missing-field-initializers         \
-         -Wno-unused-parameter -Wno-unused-value -Wno-array-bounds -fPIC -I.
+CFLAGS = -g -O3 -Wall -Werror -Wextra -Wno-missing-field-initializers -fPIC -I.
 CPPFLAGS = -D_GNU_SOURCE -DSECCOMP_BPF_STANDALONE -iquote ../../..
 LDFLAGS = -g -lpthread
 DEPFLAGS = -MMD -MF .$@.d
diff --git a/sandbox/linux/seccomp-bpf/demo.cc b/sandbox/linux/seccomp-bpf/demo.cc
index ceff9d4..1286a01 100644
--- a/sandbox/linux/seccomp-bpf/demo.cc
+++ b/sandbox/linux/seccomp-bpf/demo.cc
@@ -166,6 +166,7 @@ static void *threadFnc(void *arg) {
 }
 
 static void *sendmsgStressThreadFnc(void *arg) {
+  if (arg) { }
   static const int repetitions = 100;
   static const int kNumFds = 3;
   for (int rep = 0; rep < repetitions; ++rep) {
@@ -199,6 +200,8 @@ static void *sendmsgStressThreadFnc(void *arg) {
 }
 
 int main(int argc, char *argv[]) {
+  if (argc) { }
+  if (argv) { }
   int proc_fd = open("/proc", O_RDONLY|O_DIRECTORY);
   if (playground2::Sandbox::supportsSeccompSandbox(proc_fd) !=
       playground2::Sandbox::STATUS_AVAILABLE) {
diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
index 56931d1..043846d 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
@@ -31,6 +31,10 @@ bool Sandbox::kernelSupportSeccompBPF(int proc_fd) {
       sigprocmask(SIG_BLOCK, &newMask, &oldMask)) {
     die("sigprocmask() failed");
   }
+  int fds[2];
+  if (pipe2(fds, O_NONBLOCK|O_CLOEXEC)) {
+    die("pipe() failed");
+  }
 
   pid_t pid = fork();
   if (pid < 0) {
@@ -48,18 +52,28 @@ bool Sandbox::kernelSupportSeccompBPF(int proc_fd) {
   if (!pid) {
     // Test a very simple sandbox policy to verify that we can
     // successfully turn on sandboxing.
-    suppressLogging_ = true;
-    evaluators_.clear();
-    setSandboxPolicy(probeEvaluator, NULL);
-    setProcFd(proc_fd);
-    startSandbox();
-    if (syscall(__NR_getpid) < 0 && errno == EPERM) {
-      syscall(__NR_exit_group, (intptr_t)100);
+    dryRun_ = true;
+    if (HANDLE_EINTR(close(fds[0])) ||
+        dup2(fds[1], 2) != 2 ||
+        HANDLE_EINTR(close(fds[1]))) {
+      static const char msg[] = "Failed to set up stderr\n";
+      if (HANDLE_EINTR(write(fds[1], msg, sizeof(msg)-1))) { }
+    } else {
+      evaluators_.clear();
+      setSandboxPolicy(probeEvaluator, NULL);
+      setProcFd(proc_fd);
+      startSandbox();
+      if (syscall(__NR_getpid) < 0 && errno == EPERM) {
+        syscall(__NR_exit_group, (intptr_t)100);
+      }
     }
     die(NULL);
   }
 
   // In the parent process
+  if (HANDLE_EINTR(close(fds[1]))) {
+    die("close() failed");
+  }
   if (sigprocmask(SIG_SETMASK, &oldMask, NULL)) {
     die("sigprocmask() failed");
   }
@@ -67,7 +81,29 @@ bool Sandbox::kernelSupportSeccompBPF(int proc_fd) {
   if (HANDLE_EINTR(waitpid(pid, &status, 0)) != pid) {
     die("waitpid() failed unexpectedly");
   }
-  return WIFEXITED(status) && WEXITSTATUS(status) == 100;
+  bool rc = WIFEXITED(status) && WEXITSTATUS(status) == 100;
+
+  // If we fail to support sandboxing, there might be an additional
+  // error message. If so, this was an entirely unexpected and fatal
+  // failure. We should report the failure and somebody most fix
+  // things. This is probably a security-critical bug in the sandboxing
+  // code.
+  if (!rc) {
+    char buf[4096];
+    ssize_t len = HANDLE_EINTR(read(fds[0], buf, sizeof(buf) - 1));
+    if (len > 0) {
+      while (len > 1 && buf[len-1] == '\n') {
+        --len;
+      }
+      buf[len] = '\000';
+      die(buf);
+    }
+  }
+  if (HANDLE_EINTR(close(fds[0]))) {
+    die("close() failed");
+  }
+
+  return rc;
 }
 
 Sandbox::SandboxStatus Sandbox::supportsSeccompSandbox(int proc_fd) {
@@ -169,7 +205,7 @@ bool Sandbox::isSingleThreaded(int proc_fd) {
       sb.st_nlink != 3 ||
       HANDLE_EINTR(close(task))) {
     if (task >= 0) {
-      (void) HANDLE_EINTR(close(task));
+      if (HANDLE_EINTR(close(task))) { }
     }
     return false;
   }
@@ -312,9 +348,12 @@ void Sandbox::installFilter() {
   delete program;
 
   // Install BPF filter program
-  if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
-      prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
-    goto filter_failed;
+  if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
+    die(dryRun_ ? NULL : "Kernel refuses to enable no-new-privs");
+  } else {
+    if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
+      die(dryRun_ ? NULL : "Kernel refuses to turn on BPF filters");
+    }
   }
 
   return;
@@ -351,7 +390,7 @@ void Sandbox::sigSys(int nr, siginfo_t *info, void *void_context) {
 }
 
 
-bool Sandbox::suppressLogging_          = false;
+bool Sandbox::dryRun_                   = false;
 Sandbox::SandboxStatus Sandbox::status_ = STATUS_UNKNOWN;
 int    Sandbox::proc_fd_                = -1;
 std::vector<std::pair<Sandbox::EvaluateSyscall,
diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.h b/sandbox/linux/seccomp-bpf/sandbox_bpf.h
index 82e4656..f74072f7 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.h
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.h
@@ -190,12 +190,9 @@ class Sandbox {
  protected:
   // Print an error message and terminate the program. Used for fatal errors.
   static void die(const char *msg) __attribute__((noreturn)) {
-    if (!suppressLogging_) {
-      if (msg) {
-#ifdef SECCOMP_BPF_STANDALONE
-        HANDLE_EINTR(write(2, msg, strlen(msg)));
-        HANDLE_EINTR(write(2, "\n", 1));
-#else
+    if (msg) {
+#ifndef SECCOMP_BPF_STANDALONE
+      if (!dryRun_) {
         // LOG(FATAL) is not neccessarily async-signal safe. It would be
         // better to always use the code for the SECCOMP_BPF_STANDALONE case.
         // But that prevents the logging and reporting infrastructure from
@@ -204,7 +201,17 @@ class Sandbox {
         // LOG(FATAL). In the long run, we probably want to rewrite this code
         // to be async-signal safe.
         LOG(FATAL) << msg;
+      } else
 #endif
+      {
+        // If there is no logging infrastructure in place, we just write error
+        // messages to stderr.
+        // We also write to stderr, if we are called in a child process from
+        // supportsSeccompSandbox(). This makes sure we can actually do the
+        // correct logging from the parent process, which is more likely to
+        // have access to logging infrastructure.
+        if (HANDLE_EINTR(write(2, msg, strlen(msg)))) { }
+        if (HANDLE_EINTR(write(2, "\n", 1))) { }
       }
     }
     for (;;) {
@@ -233,7 +240,7 @@ class Sandbox {
   static void      installFilter();
   static void      sigSys(int nr, siginfo_t *info, void *void_context);
 
-  static bool          suppressLogging_;
+  static bool          dryRun_;
   static SandboxStatus status_;
   static int           proc_fd_;
   static std::vector<std::pair<EvaluateSyscall,
diff --git a/sandbox/linux/seccomp-bpf/util.cc b/sandbox/linux/seccomp-bpf/util.cc
index dea3f5a..3d3233b 100644
--- a/sandbox/linux/seccomp-bpf/util.cc
+++ b/sandbox/linux/seccomp-bpf/util.cc
@@ -139,10 +139,12 @@ void Util::closeAllBut(int fd, ...) {
             // Never ever close 0..2. If we cannot redirect to /dev/null,
             // then we are better off leaving the standard descriptors open.
             if (dev_null >= 0) {
-              HANDLE_EINTR(dup2(dev_null, i));
+              if (HANDLE_EINTR(dup2(dev_null, i))) {
+                Sandbox::die("Cannot dup2()");
+              }
             }
           } else {
-            HANDLE_EINTR(close(i));
+            if (HANDLE_EINTR(close(i))) { }
           }
           break;
         } else if (i == f) {
@@ -154,7 +156,7 @@ void Util::closeAllBut(int fd, ...) {
   }
   closedir(dir);
   if (dev_null >= 0) {
-    HANDLE_EINTR(close(dev_null));
+    if (HANDLE_EINTR(close(dev_null))) { }
   }
   return;
 }