bpf_dsl: decouple PolicyCompiler from Syscall

[Reland of https://codereview.chromium.org/939943002]

Logically, the "escape hatch PC" is now like a compiler flag, that the
user can set appropriately.  In the case of SandboxBPF, it will set the
PC to Syscall::Call(-1), as before.

This isn't a very satisfying way to resolve this cyclic dependency, but
it's the simplest and least intrusive I could think of.

BUG=449357

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

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

6 files changed, 40 insertions, 46 deletions

diff --git a/sandbox/linux/bpf_dsl/policy_compiler.cc b/sandbox/linux/bpf_dsl/policy_compiler.cc
index 18ed7be..bcfd21b 100644
--- a/sandbox/linux/bpf_dsl/policy_compiler.cc
+++ b/sandbox/linux/bpf_dsl/policy_compiler.cc
@@ -20,7 +20,6 @@
 #include "sandbox/linux/bpf_dsl/syscall_set.h"
 #include "sandbox/linux/seccomp-bpf/die.h"
 #include "sandbox/linux/seccomp-bpf/errorcode.h"
-#include "sandbox/linux/seccomp-bpf/syscall.h"
 #include "sandbox/linux/system_headers/linux_seccomp.h"
 
 namespace sandbox {
@@ -86,6 +85,7 @@ struct PolicyCompiler::Range {
 PolicyCompiler::PolicyCompiler(const Policy* policy, TrapRegistry* registry)
     : policy_(policy),
       registry_(registry),
+      escapepc_(0),
       conds_(),
       gen_(),
       has_unsafe_traps_(HasUnsafeTraps(policy_)) {
@@ -106,11 +106,8 @@ scoped_ptr<CodeGen::Program> PolicyCompiler::Compile() {
     // measures that the sandbox provides, we print a big warning message --
     // and of course, we make sure to only ever enable this feature if it
     // is actually requested by the sandbox policy.
-    if (Syscall::Call(-1) == -1 && errno == ENOSYS) {
-      SANDBOX_DIE(
-          "Support for UnsafeTrap() has not yet been ported to this "
-          "architecture");
-    }
+
+    CHECK_NE(0U, escapepc_) << "UnsafeTrap() requires a valid escape PC";
 
     for (int sysnum : kSyscallsRequiredForUnsafeTraps) {
       if (!policy_->EvaluateSyscall(sysnum)->IsAllow()) {
@@ -136,6 +133,10 @@ scoped_ptr<CodeGen::Program> PolicyCompiler::Compile() {
   return program.Pass();
 }
 
+void PolicyCompiler::DangerousSetEscapePC(uint64_t escapepc) {
+  escapepc_ = escapepc;
+}
+
 CodeGen::Node PolicyCompiler::AssemblePolicy() {
   // A compiled policy consists of three logical parts:
   //   1. Check that the "arch" field matches the expected architecture.
@@ -162,12 +163,13 @@ CodeGen::Node PolicyCompiler::MaybeAddEscapeHatch(CodeGen::Node rest) {
     return rest;
   }
 
-  // Allow system calls, if they originate from our magic return address
-  // (which we can query by calling Syscall::Call(-1)).
-  uint64_t syscall_entry_point =
-      static_cast<uint64_t>(static_cast<uintptr_t>(Syscall::Call(-1)));
-  uint32_t low = static_cast<uint32_t>(syscall_entry_point);
-  uint32_t hi = static_cast<uint32_t>(syscall_entry_point >> 32);
+  // We already enabled unsafe traps in Compile, but enable them again to give
+  // the trap registry a second chance to complain before we add the backdoor.
+  CHECK(registry_->EnableUnsafeTraps());
+
+  // Allow system calls, if they originate from our magic return address.
+  const uint32_t lopc = static_cast<uint32_t>(escapepc_);
+  const uint32_t hipc = static_cast<uint32_t>(escapepc_ >> 32);
 
   // BPF cannot do native 64-bit comparisons, so we have to compare
   // both 32-bit halves of the instruction pointer. If they match what
@@ -179,10 +181,10 @@ CodeGen::Node PolicyCompiler::MaybeAddEscapeHatch(CodeGen::Node rest) {
   return gen_.MakeInstruction(
       BPF_LD + BPF_W + BPF_ABS, SECCOMP_IP_LSB_IDX,
       gen_.MakeInstruction(
-          BPF_JMP + BPF_JEQ + BPF_K, low,
+          BPF_JMP + BPF_JEQ + BPF_K, lopc,
           gen_.MakeInstruction(
               BPF_LD + BPF_W + BPF_ABS, SECCOMP_IP_MSB_IDX,
-              gen_.MakeInstruction(BPF_JMP + BPF_JEQ + BPF_K, hi,
+              gen_.MakeInstruction(BPF_JMP + BPF_JEQ + BPF_K, hipc,
                                    CompileResult(Allow()), rest)),
           rest));
 }
diff --git a/sandbox/linux/bpf_dsl/policy_compiler.h b/sandbox/linux/bpf_dsl/policy_compiler.h
index faf6be5..d648c02 100644
--- a/sandbox/linux/bpf_dsl/policy_compiler.h
+++ b/sandbox/linux/bpf_dsl/policy_compiler.h
@@ -34,6 +34,10 @@ class SANDBOX_EXPORT PolicyCompiler {
   // compiles the policy to a BPF program, which it returns.
   scoped_ptr<CodeGen::Program> Compile();
 
+  // DangerousSetEscapePC sets the "escape PC" that is allowed to issue any
+  // system calls, regardless of policy.
+  void DangerousSetEscapePC(uint64_t escapepc);
+
   // Error returns an ErrorCode to indicate the system call should fail with
   // the specified error number.
   ErrorCode Error(int err);
@@ -88,7 +92,7 @@ class SANDBOX_EXPORT PolicyCompiler {
   CodeGen::Node CheckArch(CodeGen::Node passed);
 
   // If |has_unsafe_traps_| is true, returns an instruction sequence
-  // that allows all system calls from Syscall::Call(), and otherwise
+  // that allows all system calls from |escapepc_|, and otherwise
   // passes control to |rest|. Otherwise, simply returns |rest|.
   CodeGen::Node MaybeAddEscapeHatch(CodeGen::Node rest);
 
@@ -140,6 +144,7 @@ class SANDBOX_EXPORT PolicyCompiler {
 
   const Policy* policy_;
   TrapRegistry* registry_;
+  uint64_t escapepc_;
 
   Conds conds_;
   CodeGen gen_;
diff --git a/sandbox/linux/integration_tests/bpf_dsl_seccomp_unittest.cc b/sandbox/linux/integration_tests/bpf_dsl_seccomp_unittest.cc
index 8e7e144..e884774 100644
--- a/sandbox/linux/integration_tests/bpf_dsl_seccomp_unittest.cc
+++ b/sandbox/linux/integration_tests/bpf_dsl_seccomp_unittest.cc
@@ -557,11 +557,11 @@ SANDBOX_TEST(SandboxBPF, EnableUnsafeTrapsInSigSysHandler) {
   Die::SuppressInfoMessages(true);
 
   unsetenv(kSandboxDebuggingEnv);
-  SANDBOX_ASSERT(Trap::EnableUnsafeTrapsInSigSysHandler() == false);
+  SANDBOX_ASSERT(Trap::Registry()->EnableUnsafeTraps() == false);
   setenv(kSandboxDebuggingEnv, "", 1);
-  SANDBOX_ASSERT(Trap::EnableUnsafeTrapsInSigSysHandler() == false);
+  SANDBOX_ASSERT(Trap::Registry()->EnableUnsafeTraps() == false);
   setenv(kSandboxDebuggingEnv, "t", 1);
-  SANDBOX_ASSERT(Trap::EnableUnsafeTrapsInSigSysHandler() == true);
+  SANDBOX_ASSERT(Trap::Registry()->EnableUnsafeTraps() == true);
 }
 
 intptr_t PrctlHandler(const struct arch_seccomp_data& args, void*) {
diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
index b6553d7..0e1ee4f 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
@@ -79,6 +79,14 @@ bool KernelSupportsSeccompTsync() {
   }
 }
 
+uint64_t EscapePC() {
+  intptr_t rv = Syscall::Call(-1);
+  if (rv == -1 && errno == ENOSYS) {
+    return 0;
+  }
+  return static_cast<uint64_t>(static_cast<uintptr_t>(rv));
+}
+
 }  // namespace
 
 SandboxBPF::SandboxBPF(bpf_dsl::Policy* policy)
@@ -185,6 +193,9 @@ scoped_ptr<CodeGen::Program> SandboxBPF::AssembleFilter(
 #endif
   DCHECK(policy_);
   bpf_dsl::PolicyCompiler compiler(policy_.get(), Trap::Registry());
+  if (Trap::SandboxDebuggingAllowedByUser()) {
+    compiler.DangerousSetEscapePC(EscapePC());
+  }
   scoped_ptr<CodeGen::Program> program = compiler.Compile();
 
   // Make sure compilation resulted in a BPF program that executes
diff --git a/sandbox/linux/seccomp-bpf/trap.cc b/sandbox/linux/seccomp-bpf/trap.cc
index c87bb00..145e624 100644
--- a/sandbox/linux/seccomp-bpf/trap.cc
+++ b/sandbox/linux/seccomp-bpf/trap.cc
@@ -251,10 +251,6 @@ bool Trap::TrapKey::operator<(const TrapKey& o) const {
   }
 }
 
-uint16_t Trap::MakeTrap(TrapFnc fnc, const void* aux, bool safe) {
-  return Registry()->Add(fnc, aux, safe);
-}
-
 uint16_t Trap::Add(TrapFnc fnc, const void* aux, bool safe) {
   if (!safe && !SandboxDebuggingAllowedByUser()) {
     // Unless the user set the CHROME_SANDBOX_DEBUGGING environment variable,
@@ -353,15 +349,11 @@ uint16_t Trap::Add(TrapFnc fnc, const void* aux, bool safe) {
   return id;
 }
 
-bool Trap::SandboxDebuggingAllowedByUser() const {
+bool Trap::SandboxDebuggingAllowedByUser() {
   const char* debug_flag = getenv(kSandboxDebuggingEnv);
   return debug_flag && *debug_flag;
 }
 
-bool Trap::EnableUnsafeTrapsInSigSysHandler() {
-  return Registry()->EnableUnsafeTraps();
-}
-
 bool Trap::EnableUnsafeTraps() {
   if (!has_unsafe_traps_) {
     // Unsafe traps are a one-way fuse. Once enabled, they can never be turned
diff --git a/sandbox/linux/seccomp-bpf/trap.h b/sandbox/linux/seccomp-bpf/trap.h
index fea0052..63fff88 100644
--- a/sandbox/linux/seccomp-bpf/trap.h
+++ b/sandbox/linux/seccomp-bpf/trap.h
@@ -34,23 +34,9 @@ class SANDBOX_EXPORT Trap : public bpf_dsl::TrapRegistry {
   // creating it if necessary.
   static bpf_dsl::TrapRegistry* Registry();
 
-  // Registers a new trap handler and sets up the appropriate SIGSYS handler
-  // as needed.
-  // N.B.: This makes a permanent state change. Traps cannot be unregistered,
-  //   as that would break existing BPF filters that are still active.
-  // TODO(mdempsky): Deprecated; remove.
-  static uint16_t MakeTrap(TrapFnc fnc, const void* aux, bool safe);
-
-  // Enables support for unsafe traps in the SIGSYS signal handler. This is a
-  // one-way fuse. It works in conjunction with the BPF compiler emitting code
-  // that unconditionally allows system calls, if they have a magic return
-  // address (i.e. SandboxSyscall(-1)).
-  // Once unsafe traps are enabled, the sandbox is essentially compromised.
-  // But this is still a very useful feature for debugging purposes. Use with
-  // care. This feature is availably only if enabled by the user (see above).
-  // Returns "true", if unsafe traps were turned on.
-  // TODO(mdempsky): Deprecated; remove.
-  static bool EnableUnsafeTrapsInSigSysHandler();
+  // SandboxDebuggingAllowedByUser returns whether the
+  // "CHROME_SANDBOX_DEBUGGING" environment variable is set.
+  static bool SandboxDebuggingAllowedByUser();
 
  private:
   struct TrapKey {
@@ -77,8 +63,6 @@ class SANDBOX_EXPORT Trap : public bpf_dsl::TrapRegistry {
   // dumps.
   void SigSys(int nr, siginfo_t* info, void* void_context)
       __attribute__((noinline));
-  bool SandboxDebuggingAllowedByUser() const;
-
   // We have a global singleton that handles all of our SIGSYS traps. This
   // variable must never be deallocated after it has been set up initially, as
   // there is no way to reset in-kernel BPF filters that generate SIGSYS