bpf_dsl: move Verifier into PolicyCompiler

This makes it easier to run verification within bpf_dsl_unittests too,
though it also exposes that FakeTrapRegistry was being sloppy by not
reusing trap IDs when a trap handler is assigned a second time.

BUG=449357

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

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

8 files changed, 90 insertions, 68 deletions

diff --git a/sandbox/linux/BUILD.gn b/sandbox/linux/BUILD.gn
index ccb186a..3b340bd 100644
--- a/sandbox/linux/BUILD.gn
+++ b/sandbox/linux/BUILD.gn
@@ -168,6 +168,8 @@ component("seccomp_bpf") {
     "bpf_dsl/syscall_set.cc",
     "bpf_dsl/syscall_set.h",
     "bpf_dsl/trap_registry.h",
+    "bpf_dsl/verifier.cc",
+    "bpf_dsl/verifier.h",
     "seccomp-bpf/die.cc",
     "seccomp-bpf/die.h",
     "seccomp-bpf/errorcode.cc",
@@ -178,8 +180,6 @@ component("seccomp_bpf") {
     "seccomp-bpf/syscall.h",
     "seccomp-bpf/trap.cc",
     "seccomp-bpf/trap.h",
-    "seccomp-bpf/verifier.cc",
-    "seccomp-bpf/verifier.h",
   ]
   defines = [ "SANDBOX_IMPLEMENTATION" ]
 
diff --git a/sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc b/sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc
index 168eaec..dba421f 100644
--- a/sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc
+++ b/sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc
@@ -12,6 +12,9 @@
 #include <sys/utsname.h>
 #include <unistd.h>
 
+#include <map>
+#include <utility>
+
 #include "base/files/scoped_file.h"
 #include "base/macros.h"
 #include "build/build_config.h"
@@ -21,8 +24,8 @@
 #include "sandbox/linux/bpf_dsl/policy_compiler.h"
 #include "sandbox/linux/bpf_dsl/seccomp_macros.h"
 #include "sandbox/linux/bpf_dsl/trap_registry.h"
+#include "sandbox/linux/bpf_dsl/verifier.h"
 #include "sandbox/linux/seccomp-bpf/errorcode.h"
-#include "sandbox/linux/seccomp-bpf/verifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #define CASES SANDBOX_BPF_DSL_CASES
@@ -56,12 +59,14 @@ struct arch_seccomp_data FakeSyscall(int nr,
 
 class FakeTrapRegistry : public TrapRegistry {
  public:
-  FakeTrapRegistry() : next_id_(1) {}
+  FakeTrapRegistry() : map_() {}
   virtual ~FakeTrapRegistry() {}
 
   uint16_t Add(TrapFnc fnc, const void* aux, bool safe) override {
-    EXPECT_NE(0U, next_id_);
-    return next_id_++;
+    EXPECT_TRUE(safe);
+
+    const uint16_t next_id = map_.size() + 1;
+    return map_.insert(std::make_pair(Key(fnc, aux), next_id)).first->second;
   }
 
   bool EnableUnsafeTraps() override {
@@ -70,15 +75,43 @@ class FakeTrapRegistry : public TrapRegistry {
   }
 
  private:
-  uint16_t next_id_;
+  using Key = std::pair<TrapFnc, const void*>;
+
+  std::map<Key, uint16_t> map_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeTrapRegistry);
 };
 
+intptr_t FakeTrapFuncOne(const arch_seccomp_data& data, void* aux) { return 1; }
+intptr_t FakeTrapFuncTwo(const arch_seccomp_data& data, void* aux) { return 2; }
+
+// Test that FakeTrapRegistry correctly assigns trap IDs to trap handlers.
+TEST(FakeTrapRegistry, TrapIDs) {
+  struct {
+    TrapRegistry::TrapFnc fnc;
+    const void* aux;
+  } funcs[] = {
+      {FakeTrapFuncOne, nullptr},
+      {FakeTrapFuncTwo, nullptr},
+      {FakeTrapFuncOne, funcs},
+      {FakeTrapFuncTwo, funcs},
+  };
+
+  FakeTrapRegistry traps;
+
+  // Add traps twice to test that IDs are reused correctly.
+  for (int i = 0; i < 2; ++i) {
+    for (size_t j = 0; j < arraysize(funcs); ++j) {
+      // Trap IDs start at 1.
+      EXPECT_EQ(j + 1, traps.Add(funcs[j].fnc, funcs[j].aux, true));
+    }
+  }
+}
+
 class PolicyEmulator {
  public:
   explicit PolicyEmulator(const Policy* policy) : program_(), traps_() {
-    program_ = *PolicyCompiler(policy, &traps_).Compile();
+    program_ = *PolicyCompiler(policy, &traps_).Compile(true /* verify */);
   }
   ~PolicyEmulator() {}
 
diff --git a/sandbox/linux/bpf_dsl/policy_compiler.cc b/sandbox/linux/bpf_dsl/policy_compiler.cc
index 9d94968..f508b30 100644
--- a/sandbox/linux/bpf_dsl/policy_compiler.cc
+++ b/sandbox/linux/bpf_dsl/policy_compiler.cc
@@ -15,9 +15,11 @@
 #include "sandbox/linux/bpf_dsl/bpf_dsl.h"
 #include "sandbox/linux/bpf_dsl/bpf_dsl_impl.h"
 #include "sandbox/linux/bpf_dsl/codegen.h"
+#include "sandbox/linux/bpf_dsl/dump_bpf.h"
 #include "sandbox/linux/bpf_dsl/policy.h"
 #include "sandbox/linux/bpf_dsl/seccomp_macros.h"
 #include "sandbox/linux/bpf_dsl/syscall_set.h"
+#include "sandbox/linux/bpf_dsl/verifier.h"
 #include "sandbox/linux/seccomp-bpf/errorcode.h"
 #include "sandbox/linux/system_headers/linux_seccomp.h"
 
@@ -94,7 +96,7 @@ PolicyCompiler::PolicyCompiler(const Policy* policy, TrapRegistry* registry)
 PolicyCompiler::~PolicyCompiler() {
 }
 
-scoped_ptr<CodeGen::Program> PolicyCompiler::Compile() {
+scoped_ptr<CodeGen::Program> PolicyCompiler::Compile(bool verify) {
   CHECK(policy_->InvalidSyscall()->IsDeny())
       << "Policies should deny invalid system calls";
 
@@ -115,6 +117,18 @@ scoped_ptr<CodeGen::Program> PolicyCompiler::Compile() {
   // Assemble the BPF filter program.
   scoped_ptr<CodeGen::Program> program(new CodeGen::Program());
   gen_.Compile(AssemblePolicy(), program.get());
+
+  // 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 (verify) {
+    const char* err = nullptr;
+    if (!Verifier::VerifyBPF(this, *program, *policy_, &err)) {
+      DumpBPF::PrintProgram(*program);
+      LOG(FATAL) << err;
+    }
+  }
+
   return program.Pass();
 }
 
diff --git a/sandbox/linux/bpf_dsl/policy_compiler.h b/sandbox/linux/bpf_dsl/policy_compiler.h
index d648c02..df38d4c 100644
--- a/sandbox/linux/bpf_dsl/policy_compiler.h
+++ b/sandbox/linux/bpf_dsl/policy_compiler.h
@@ -32,7 +32,7 @@ class SANDBOX_EXPORT PolicyCompiler {
 
   // Compile registers any trap handlers needed by the policy and
   // compiles the policy to a BPF program, which it returns.
-  scoped_ptr<CodeGen::Program> Compile();
+  scoped_ptr<CodeGen::Program> Compile(bool verify);
 
   // DangerousSetEscapePC sets the "escape PC" that is allowed to issue any
   // system calls, regardless of policy.
diff --git a/sandbox/linux/seccomp-bpf/verifier.cc b/sandbox/linux/bpf_dsl/verifier.cc
index e533b2d9..adbc960 100644
--- a/sandbox/linux/seccomp-bpf/verifier.cc
+++ b/sandbox/linux/bpf_dsl/verifier.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "sandbox/linux/seccomp-bpf/verifier.h"
+#include "sandbox/linux/bpf_dsl/verifier.h"
 
 #include <string.h>
 
@@ -15,16 +15,15 @@
 #include "sandbox/linux/bpf_dsl/seccomp_macros.h"
 #include "sandbox/linux/bpf_dsl/syscall_set.h"
 #include "sandbox/linux/seccomp-bpf/errorcode.h"
-#include "sandbox/linux/seccomp-bpf/sandbox_bpf.h"
 #include "sandbox/linux/system_headers/linux_seccomp.h"
 
 namespace sandbox {
+namespace bpf_dsl {
 
 namespace {
 
 const uint64_t kLower32Bits = std::numeric_limits<uint32_t>::max();
 const uint64_t kUpper32Bits = static_cast<uint64_t>(kLower32Bits) << 32;
-const uint64_t kFull64Bits = std::numeric_limits<uint64_t>::max();
 
 struct State {
   State(const std::vector<struct sock_filter>& p,
@@ -54,8 +53,8 @@ uint32_t EvaluateErrorCode(bpf_dsl::PolicyCompiler* compiler,
       return compiler->Unexpected64bitArgument().err();
     }
     bool equal = (data.args[code.argno()] & code.mask()) == code.value();
-    return EvaluateErrorCode(
-        compiler, equal ? *code.passed() : *code.failed(), data);
+    return EvaluateErrorCode(compiler, equal ? *code.passed() : *code.failed(),
+                             data);
   } else {
     return SECCOMP_RET_INVALID;
   }
@@ -69,10 +68,12 @@ bool VerifyErrorCode(bpf_dsl::PolicyCompiler* compiler,
                      const char** err) {
   if (code.error_type() == ErrorCode::ET_SIMPLE ||
       code.error_type() == ErrorCode::ET_TRAP) {
-    uint32_t computed_ret = Verifier::EvaluateBPF(program, *data, err);
+    const uint32_t computed_ret = Verifier::EvaluateBPF(program, *data, err);
     if (*err) {
       return false;
-    } else if (computed_ret != EvaluateErrorCode(compiler, root_code, *data)) {
+    }
+    const uint32_t policy_ret = EvaluateErrorCode(compiler, root_code, *data);
+    if (computed_ret != policy_ret) {
       // For efficiency's sake, we'd much rather compare "computed_ret"
       // against "code.err()". This works most of the time, but it doesn't
       // always work for nested conditional expressions. The test values
@@ -97,8 +98,8 @@ bool VerifyErrorCode(bpf_dsl::PolicyCompiler* compiler,
 
     // Verify that we can check a value for simple equality.
     data->args[code.argno()] = code.value();
-    if (!VerifyErrorCode(
-            compiler, program, data, root_code, *code.passed(), err)) {
+    if (!VerifyErrorCode(compiler, program, data, root_code, *code.passed(),
+                         err)) {
       return false;
     }
 
@@ -110,15 +111,15 @@ bool VerifyErrorCode(bpf_dsl::PolicyCompiler* compiler,
     }
     if ((ignored_bits & kLower32Bits) != 0) {
       data->args[code.argno()] = code.value() | (ignored_bits & kLower32Bits);
-      if (!VerifyErrorCode(
-              compiler, program, data, root_code, *code.passed(), err)) {
+      if (!VerifyErrorCode(compiler, program, data, root_code, *code.passed(),
+                           err)) {
         return false;
       }
     }
     if ((ignored_bits & kUpper32Bits) != 0) {
       data->args[code.argno()] = code.value() | (ignored_bits & kUpper32Bits);
-      if (!VerifyErrorCode(
-              compiler, program, data, root_code, *code.passed(), err)) {
+      if (!VerifyErrorCode(compiler, program, data, root_code, *code.passed(),
+                           err)) {
         return false;
       }
     }
@@ -126,15 +127,15 @@ bool VerifyErrorCode(bpf_dsl::PolicyCompiler* compiler,
     // Verify that changing bits included in the mask is detected as inequality.
     if ((code.mask() & kLower32Bits) != 0) {
       data->args[code.argno()] = code.value() ^ (code.mask() & kLower32Bits);
-      if (!VerifyErrorCode(
-              compiler, program, data, root_code, *code.failed(), err)) {
+      if (!VerifyErrorCode(compiler, program, data, root_code, *code.failed(),
+                           err)) {
         return false;
       }
     }
     if ((code.mask() & kUpper32Bits) != 0) {
       data->args[code.argno()] = code.value() ^ (code.mask() & kUpper32Bits);
-      if (!VerifyErrorCode(
-              compiler, program, data, root_code, *code.failed(), err)) {
+      if (!VerifyErrorCode(compiler, program, data, root_code, *code.failed(),
+                           err)) {
         return false;
       }
     }
@@ -145,24 +146,16 @@ bool VerifyErrorCode(bpf_dsl::PolicyCompiler* compiler,
 
       // Arbitrary 64-bit values should be rejected.
       data->args[code.argno()] = 1ULL << 32;
-      if (!VerifyErrorCode(compiler,
-                           program,
-                           data,
-                           root_code,
-                           compiler->Unexpected64bitArgument(),
-                           err)) {
+      if (!VerifyErrorCode(compiler, program, data, root_code,
+                           compiler->Unexpected64bitArgument(), err)) {
         return false;
       }
 
       // Upper 32-bits set without the MSB of the lower 32-bits set should be
       // rejected too.
       data->args[code.argno()] = kUpper32Bits;
-      if (!VerifyErrorCode(compiler,
-                           program,
-                           data,
-                           root_code,
-                           compiler->Unexpected64bitArgument(),
-                           err)) {
+      if (!VerifyErrorCode(compiler, program, data, root_code,
+                           compiler->Unexpected64bitArgument(), err)) {
         return false;
       }
     }
@@ -182,8 +175,7 @@ void Ld(State* state, const struct sock_filter& insn, const char** err) {
   if (insn.k < sizeof(struct arch_seccomp_data) && (insn.k & 3) == 0) {
     // We only allow loading of properly aligned 32bit quantities.
     memcpy(&state->accumulator,
-           reinterpret_cast<const char*>(&state->data) + insn.k,
-           4);
+           reinterpret_cast<const char*>(&state->data) + insn.k, 4);
   } else {
     *err = "Invalid operand in BPF_LD instruction";
     return;
@@ -399,4 +391,5 @@ uint32_t Verifier::EvaluateBPF(const std::vector<struct sock_filter>& program,
   return 0;
 }
 
+}  // namespace bpf_dsl
 }  // namespace sandbox
diff --git a/sandbox/linux/seccomp-bpf/verifier.h b/sandbox/linux/bpf_dsl/verifier.h
index 50efe64..b0435d1 100644
--- a/sandbox/linux/seccomp-bpf/verifier.h
+++ b/sandbox/linux/bpf_dsl/verifier.h
@@ -2,8 +2,8 @@
 // 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_VERIFIER_H__
-#define SANDBOX_LINUX_SECCOMP_BPF_VERIFIER_H__
+#ifndef SANDBOX_LINUX_BPF_DSL_VERIFIER_H__
+#define SANDBOX_LINUX_BPF_DSL_VERIFIER_H__
 
 #include <stdint.h>
 
@@ -16,10 +16,10 @@ struct sock_filter;
 
 namespace sandbox {
 struct arch_seccomp_data;
+
 namespace bpf_dsl {
 class Policy;
 class PolicyCompiler;
-}
 
 class SANDBOX_EXPORT Verifier {
  public:
@@ -51,6 +51,7 @@ class SANDBOX_EXPORT Verifier {
   DISALLOW_IMPLICIT_CONSTRUCTORS(Verifier);
 };
 
+}  // namespace bpf_dsl
 }  // namespace sandbox
 
-#endif  // SANDBOX_LINUX_SECCOMP_BPF_VERIFIER_H__
+#endif  // SANDBOX_LINUX_BPF_DSL_VERIFIER_H__
diff --git a/sandbox/linux/sandbox_linux.gypi b/sandbox/linux/sandbox_linux.gypi
index a623971..6111fd3 100644
--- a/sandbox/linux/sandbox_linux.gypi
+++ b/sandbox/linux/sandbox_linux.gypi
@@ -137,6 +137,8 @@
         'bpf_dsl/syscall_set.cc',
         'bpf_dsl/syscall_set.h',
         'bpf_dsl/trap_registry.h',
+        'bpf_dsl/verifier.cc',
+        'bpf_dsl/verifier.h',
         'seccomp-bpf/die.cc',
         'seccomp-bpf/die.h',
         'seccomp-bpf/errorcode.cc',
@@ -147,8 +149,6 @@
         'seccomp-bpf/syscall.h',
         'seccomp-bpf/trap.cc',
         'seccomp-bpf/trap.h',
-        'seccomp-bpf/verifier.cc',
-        'seccomp-bpf/verifier.h',
       ],
       'dependencies': [
         '../base/base.gyp:base',
diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
index 0e1ee4f..c96642e 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
@@ -24,16 +24,13 @@
 #include "base/posix/eintr_wrapper.h"
 #include "base/third_party/valgrind/valgrind.h"
 #include "sandbox/linux/bpf_dsl/codegen.h"
-#include "sandbox/linux/bpf_dsl/dump_bpf.h"
 #include "sandbox/linux/bpf_dsl/policy.h"
 #include "sandbox/linux/bpf_dsl/policy_compiler.h"
 #include "sandbox/linux/bpf_dsl/seccomp_macros.h"
 #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/seccomp-bpf/trap.h"
-#include "sandbox/linux/seccomp-bpf/verifier.h"
 #include "sandbox/linux/services/proc_util.h"
 #include "sandbox/linux/services/syscall_wrappers.h"
 #include "sandbox/linux/services/thread_helpers.h"
@@ -192,28 +189,12 @@ scoped_ptr<CodeGen::Program> SandboxBPF::AssembleFilter(
   force_verification = true;
 #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
-  // 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();
+  return compiler.Compile(force_verification);
 }
 
 void SandboxBPF::InstallFilter(bool must_sync_threads) {