Linux sandbox: cleanup the SandboxBPF class

Remove obsolete code and declarations, fine tune the header file comments,
re-order methods in the header file (and in the implementation to match).

No code is added or changed in this CL (but dead code is removed).

BUG=434820

Review URL: https://codereview.chromium.org/762563002

Cr-Commit-Position: refs/heads/master@{#305733}

2 files changed, 93 insertions, 118 deletions

diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
index dd15478..d352794 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
@@ -11,16 +11,9 @@
 #endif
 
 #include <errno.h>
-#include <fcntl.h>
 #include <linux/filter.h>
-#include <signal.h>
-#include <string.h>
 #include <sys/prctl.h>
-#include <sys/stat.h>
-#include <sys/syscall.h>
 #include <sys/types.h>
-#include <sys/wait.h>
-#include <time.h>
 #include <unistd.h>
 
 #include "base/compiler_specific.h"
@@ -28,7 +21,6 @@
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/posix/eintr_wrapper.h"
-#include "sandbox/linux/bpf_dsl/bpf_dsl.h"
 #include "sandbox/linux/bpf_dsl/dump_bpf.h"
 #include "sandbox/linux/bpf_dsl/policy.h"
 #include "sandbox/linux/bpf_dsl/policy_compiler.h"
@@ -44,10 +36,6 @@
 #include "sandbox/linux/services/syscall_wrappers.h"
 #include "sandbox/linux/services/thread_helpers.h"
 
-using sandbox::bpf_dsl::Allow;
-using sandbox::bpf_dsl::Error;
-using sandbox::bpf_dsl::ResultExpr;
-
 namespace sandbox {
 
 namespace {
@@ -88,7 +76,7 @@ bool KernelSupportsSeccompTsync() {
 }  // namespace
 
 SandboxBPF::SandboxBPF()
-    : quiet_(false), proc_task_fd_(-1), sandbox_has_started_(false), policy_() {
+    : proc_task_fd_(-1), sandbox_has_started_(false), policy_() {
 }
 
 SandboxBPF::~SandboxBPF() {
@@ -96,10 +84,6 @@ SandboxBPF::~SandboxBPF() {
     IGNORE_EINTR(close(proc_task_fd_));
 }
 
-bool SandboxBPF::IsValidSyscallNumber(int sysnum) {
-  return SyscallSet::IsValid(sysnum);
-}
-
 // static
 bool SandboxBPF::SupportsSeccompSandbox(SeccompLevel level) {
   switch (level) {
@@ -112,8 +96,13 @@ bool SandboxBPF::SupportsSeccompSandbox(SeccompLevel level) {
   return false;
 }
 
-void SandboxBPF::set_proc_task_fd(int proc_task_fd) {
-  proc_task_fd_ = proc_task_fd;
+// Don't take a scoped_ptr here, polymorphism makes their use awkward.
+void SandboxBPF::SetSandboxPolicy(bpf_dsl::Policy* policy) {
+  DCHECK(!policy_);
+  if (sandbox_has_started_) {
+    SANDBOX_DIE("Cannot change policy after sandbox has started");
+  }
+  policy_.reset(policy);
 }
 
 bool SandboxBPF::StartSandbox(SeccompLevel seccomp_level) {
@@ -165,13 +154,54 @@ bool SandboxBPF::StartSandbox(SeccompLevel seccomp_level) {
   return true;
 }
 
-// Don't take a scoped_ptr here, polymorphism make their use awkward.
-void SandboxBPF::SetSandboxPolicy(bpf_dsl::Policy* policy) {
-  DCHECK(!policy_);
-  if (sandbox_has_started_) {
-    SANDBOX_DIE("Cannot change policy after sandbox has started");
+void SandboxBPF::set_proc_task_fd(int proc_task_fd) {
+  proc_task_fd_ = proc_task_fd;
+}
+
+// static
+bool SandboxBPF::IsValidSyscallNumber(int sysnum) {
+  return SyscallSet::IsValid(sysnum);
+}
+
+// static
+bool SandboxBPF::IsRequiredForUnsafeTrap(int sysno) {
+  return bpf_dsl::PolicyCompiler::IsRequiredForUnsafeTrap(sysno);
+}
+
+// static
+intptr_t SandboxBPF::ForwardSyscall(const struct arch_seccomp_data& args) {
+  return Syscall::Call(
+      args.nr, static_cast<intptr_t>(args.args[0]),
+      static_cast<intptr_t>(args.args[1]), static_cast<intptr_t>(args.args[2]),
+      static_cast<intptr_t>(args.args[3]), static_cast<intptr_t>(args.args[4]),
+      static_cast<intptr_t>(args.args[5]));
+}
+
+scoped_ptr<CodeGen::Program> SandboxBPF::AssembleFilter(
+    bool force_verification) {
+#if !defined(NDEBUG)
+  force_verification = true;
+#endif
+
+  bpf_dsl::PolicyCompiler compiler(policy_.get(), Trap::Registry());
+  scoped_ptr<CodeGen::Program> program = compiler.Compile();
+
+  // Make sure compilation resulted in a BPF program that executes
+  // correctly. Otherwise, there is an internal error in our BPF compiler.
+  // There is really nothing the caller can do until the bug is fixed.
+  if (force_verification) {
+    // Verification is expensive. We only perform this step, if we are
+    // compiled in debug mode, or if the caller explicitly requested
+    // verification.
+
+    const char* err = NULL;
+    if (!Verifier::VerifyBPF(&compiler, *program, *policy_, &err)) {
+      bpf_dsl::DumpBPF::PrintProgram(*program);
+      SANDBOX_DIE(err);
+    }
   }
-  policy_.reset(policy);
+
+  return program.Pass();
 }
 
 void SandboxBPF::InstallFilter(bool must_sync_threads) {
@@ -200,7 +230,7 @@ void SandboxBPF::InstallFilter(bool must_sync_threads) {
   policy_.reset();
 
   if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
-    SANDBOX_DIE(quiet_ ? NULL : "Kernel refuses to enable no-new-privs");
+    SANDBOX_DIE("Kernel refuses to enable no-new-privs");
   }
 
   // Install BPF filter program. If the thread state indicates multi-threading
@@ -210,57 +240,16 @@ void SandboxBPF::InstallFilter(bool must_sync_threads) {
     int rv =
         sys_seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC, &prog);
     if (rv) {
-      SANDBOX_DIE(quiet_ ? NULL :
+      SANDBOX_DIE(
           "Kernel refuses to turn on and synchronize threads for BPF filters");
     }
   } else {
     if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) {
-      SANDBOX_DIE(quiet_ ? NULL : "Kernel refuses to turn on BPF filters");
+      SANDBOX_DIE("Kernel refuses to turn on BPF filters");
     }
   }
 
   sandbox_has_started_ = true;
 }
 
-scoped_ptr<CodeGen::Program> SandboxBPF::AssembleFilter(
-    bool force_verification) {
-#if !defined(NDEBUG)
-  force_verification = true;
-#endif
-
-  bpf_dsl::PolicyCompiler compiler(policy_.get(), Trap::Registry());
-  scoped_ptr<CodeGen::Program> program = compiler.Compile();
-
-  // Make sure compilation resulted in BPF program that executes
-  // correctly. Otherwise, there is an internal error in our BPF compiler.
-  // There is really nothing the caller can do until the bug is fixed.
-  if (force_verification) {
-    // Verification is expensive. We only perform this step, if we are
-    // compiled in debug mode, or if the caller explicitly requested
-    // verification.
-
-    const char* err = NULL;
-    if (!Verifier::VerifyBPF(&compiler, *program, *policy_, &err)) {
-      bpf_dsl::DumpBPF::PrintProgram(*program);
-      SANDBOX_DIE(err);
-    }
-  }
-
-  return program.Pass();
-}
-
-bool SandboxBPF::IsRequiredForUnsafeTrap(int sysno) {
-  return bpf_dsl::PolicyCompiler::IsRequiredForUnsafeTrap(sysno);
-}
-
-intptr_t SandboxBPF::ForwardSyscall(const struct arch_seccomp_data& args) {
-  return Syscall::Call(args.nr,
-                       static_cast<intptr_t>(args.args[0]),
-                       static_cast<intptr_t>(args.args[1]),
-                       static_cast<intptr_t>(args.args[2]),
-                       static_cast<intptr_t>(args.args[3]),
-                       static_cast<intptr_t>(args.args[4]),
-                       static_cast<intptr_t>(args.args[5]));
-}
-
 }  // namespace sandbox
diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.h b/sandbox/linux/seccomp-bpf/sandbox_bpf.h
index 10a816d..3df6c19 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.h
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.h
@@ -2,12 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef SANDBOX_LINUX_SECCOMP_BPF_SANDBOX_BPF_H__
-#define SANDBOX_LINUX_SECCOMP_BPF_SANDBOX_BPF_H__
+#ifndef SANDBOX_LINUX_SECCOMP_BPF_SANDBOX_BPF_H_
+#define SANDBOX_LINUX_SECCOMP_BPF_SANDBOX_BPF_H_
 
 #include <stdint.h>
 
-#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "sandbox/linux/seccomp-bpf/codegen.h"
@@ -19,6 +18,10 @@ namespace bpf_dsl {
 class Policy;
 }
 
+// This class can be used to apply a syscall sandboxing policy expressed in a
+// bpf_dsl::Policy object to the current process.
+// Syscall sandboxing policies get inherited by subprocesses and, once applied,
+// can never be removed for the lifetime of the process.
 class SANDBOX_EXPORT SandboxBPF {
  public:
   enum class SeccompLevel {
@@ -26,55 +29,27 @@ class SANDBOX_EXPORT SandboxBPF {
     MULTI_THREADED,
   };
 
-  // Constructors and destructors.
+  SandboxBPF();
   // NOTE: Setting a policy and starting the sandbox is a one-way operation.
   // The kernel does not provide any option for unloading a loaded sandbox. The
   // sandbox remains engaged even when the object is destructed.
-  SandboxBPF();
   ~SandboxBPF();
 
-  // Checks whether a particular system call number is valid on the current
-  // architecture. E.g. on ARM there's a non-contiguous range of private
-  // system calls.
-  static bool IsValidSyscallNumber(int sysnum);
-
   // Detect if the kernel supports the specified seccomp level.
   // See StartSandbox() for a description of these.
   static bool SupportsSeccompSandbox(SeccompLevel level);
 
-  // The sandbox needs to be able to access files in "/proc/self/task/". If
-  // this directory is not accessible when "startSandbox()" gets called, the
-  // caller must provide an already opened file descriptor by calling
-  // "set_proc_task_fd()".
-  // The sandbox becomes the new owner of this file descriptor and will
-  // eventually close it when "StartSandbox()" executes.
-  void set_proc_task_fd(int proc_task_fd);
-
   // Set the BPF policy as |policy|. Ownership of |policy| is transfered here
   // to the sandbox object.
   void SetSandboxPolicy(bpf_dsl::Policy* policy);
 
-  // UnsafeTraps require some syscalls to always be allowed.
-  // This helper function returns true for these calls.
-  static bool IsRequiredForUnsafeTrap(int sysno);
-
-  // From within an UnsafeTrap() it is often useful to be able to execute
-  // the system call that triggered the trap. The ForwardSyscall() method
-  // makes this easy. It is more efficient than calling glibc's syscall()
-  // function, as it avoid the extra round-trip to the signal handler. And
-  // it automatically does the correct thing to report kernel-style error
-  // conditions, rather than setting errno. See the comments for TrapFnc for
-  // details. In other words, the return value from ForwardSyscall() is
-  // directly suitable as a return value for a trap handler.
-  static intptr_t ForwardSyscall(const struct arch_seccomp_data& args);
-
   // This is the main public entry point. It sets up the resources needed by
   // the sandbox, and enters Seccomp mode.
   // The calling process must provide a |level| to tell the sandbox which type
   // of kernel support it should engage.
   // SINGLE_THREADED will only sandbox the calling thread. Since it would be a
   // security risk, the sandbox will also check that the current process is
-  // single threaded.
+  // single threaded and crash if it isn't the case.
   // MULTI_THREADED requires more recent kernel support and allows to sandbox
   // all the threads of the current process. Be mindful of potential races,
   // with other threads using disallowed system calls either before or after
@@ -90,6 +65,32 @@ class SANDBOX_EXPORT SandboxBPF {
   // combined policy. So, it should only be used if there are no alternatives.
   bool StartSandbox(SeccompLevel level) WARN_UNUSED_RESULT;
 
+  // The sandbox needs to be able to access files in "/proc/self/task/". If
+  // this directory is not accessible when "StartSandbox()" gets called, the
+  // caller must provide an already opened file descriptor by calling
+  // "set_proc_task_fd()".
+  // The sandbox becomes the new owner of this file descriptor and will
+  // eventually close it when "StartSandbox()" executes.
+  void set_proc_task_fd(int proc_task_fd);
+
+  // Checks whether a particular system call number is valid on the current
+  // architecture.
+  static bool IsValidSyscallNumber(int sysnum);
+
+  // UnsafeTraps require some syscalls to always be allowed.
+  // This helper function returns true for these calls.
+  static bool IsRequiredForUnsafeTrap(int sysno);
+
+  // From within an UnsafeTrap() it is often useful to be able to execute
+  // the system call that triggered the trap. The ForwardSyscall() method
+  // makes this easy. It is more efficient than calling glibc's syscall()
+  // function, as it avoid the extra round-trip to the signal handler. And
+  // it automatically does the correct thing to report kernel-style error
+  // conditions, rather than setting errno. See the comments for TrapFnc for
+  // details. In other words, the return value from ForwardSyscall() is
+  // directly suitable as a return value for a trap handler.
+  static intptr_t ForwardSyscall(const struct arch_seccomp_data& args);
+
   // Assembles a BPF filter program from the current policy. After calling this
   // function, you must not call any other sandboxing function.
   // Typically, AssembleFilter() is only used by unit tests and by sandbox
@@ -101,25 +102,10 @@ class SANDBOX_EXPORT SandboxBPF {
   scoped_ptr<CodeGen::Program> AssembleFilter(bool force_verification);
 
  private:
-  // Get a file descriptor pointing to "/proc", if currently available.
-  int proc_task_fd() { return proc_task_fd_; }
-
-  // Creates a subprocess and runs "code_in_sandbox" inside of the specified
-  // policy. The caller has to make sure that "this" has not yet been
-  // initialized with any other policies.
-  bool RunFunctionInPolicy(void (*code_in_sandbox)(),
-                           scoped_ptr<bpf_dsl::Policy> policy);
-
   // Assembles and installs a filter based on the policy that has previously
   // been configured with SetSandboxPolicy().
   void InstallFilter(bool must_sync_threads);
 
-  // Verify the correctness of a compiled program by comparing it against the
-  // current policy. This function should only ever be called by unit tests and
-  // by the sandbox internals. It should not be used by production code.
-  void VerifyProgram(const CodeGen::Program& program);
-
-  bool quiet_;
   int proc_task_fd_;
   bool sandbox_has_started_;
   scoped_ptr<bpf_dsl::Policy> policy_;
@@ -129,4 +115,4 @@ class SANDBOX_EXPORT SandboxBPF {
 
 }  // namespace sandbox
 
-#endif  // SANDBOX_LINUX_SECCOMP_BPF_SANDBOX_BPF_H__
+#endif  // SANDBOX_LINUX_SECCOMP_BPF_SANDBOX_BPF_H_