SECCOMP-BPF: Refactor the BPF sandbox API to use fewer "static" fields and methods.

sandbox/linux/seccomp-bpf/verifier.cc

Index: sandbox/linux/seccomp-bpf/verifier.cc
diff --git a/sandbox/linux/seccomp-bpf/verifier.cc b/sandbox/linux/seccomp-bpf/verifier.cc
index 06143b9c20e5ac45723f23e9718295b911627c0c..e25c369e0855d17937252849c35cb89931f86716 100644
--- a/sandbox/linux/seccomp-bpf/verifier.cc
+++ b/sandbox/linux/seccomp-bpf/verifier.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string.h>
+
 #include "sandbox/linux/seccomp-bpf/sandbox_bpf.h"
 #include "sandbox/linux/seccomp-bpf/syscall_iterator.h"
 #include "sandbox/linux/seccomp-bpf/verifier.h"
@@ -9,79 +11,64 @@
 
 namespace playground2 {
 
-bool Verifier::VerifyBPF(const std::vector<struct sock_filter>& program,
-                         const Sandbox::Evaluators& evaluators,
-                         const char **err) {
-  *err = NULL;
-  if (evaluators.size() != 1) {
-    *err = "Not implemented";
-    return false;
-  }
-  Sandbox::EvaluateSyscall evaluate_syscall = evaluators.begin()->first;
-  void *aux                                 = evaluators.begin()->second;
-  for (SyscallIterator iter(false); !iter.Done(); ) {
-    uint32_t sysnum = iter.Next();
-    // We ideally want to iterate over the full system call range and values
-    // just above and just below this range. This gives us the full result set
-    // of the "evaluators".
-    // On Intel systems, this can fail in a surprising way, as a cleared bit 30
-    // indicates either i386 or x86-64; and a set bit 30 indicates x32. And
-    // unless we pay attention to setting this bit correctly, an early check in
-    // our BPF program will make us fail with a misleading error code.
-    struct arch_seccomp_data data = { static_cast<int>(sysnum),
-                                      static_cast<uint32_t>(SECCOMP_ARCH) };
-#if defined(__i386__) || defined(__x86_64__)
-#if defined(__x86_64__) && defined(__ILP32__)
-    if (!(sysnum & 0x40000000u)) {
-      continue;
-    }
-#else
-    if (sysnum & 0x40000000u) {
-      continue;
-    }
-#endif
-#endif
-    ErrorCode code = evaluate_syscall(sysnum, aux);
-    if (!VerifyErrorCode(program, &data, code, code, err)) {
-      return false;
-    }
+namespace {

[jln (very slow on Chromium), 2013/02/15 20:58:25]

I really would rather not nest the anonymous namespace inside of playground2.
But I understand why you do it in that case.

Please continute to make sure that the anonymous namespace is the first thing in
the other namespace if you do that, as it is now.

+
+struct State {
+  State(const std::vector<struct sock_filter>& p,
+        const struct arch_seccomp_data& d) :
+    program(p),
+    data(d),
+    ip(0),
+    accumulator(0),
+    acc_is_valid(false) {
   }
-  return true;
-}
+  const std::vector<struct sock_filter>& program;
+  const struct arch_seccomp_data&        data;
+  unsigned int                           ip;
+  uint32_t                               accumulator;
+  bool                                   acc_is_valid;
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(State);
+};
 
-uint32_t Verifier::EvaluateErrorCode(const ErrorCode& code,
-                                     const struct arch_seccomp_data& data) {
-  if (code.error_type_ == ErrorCode::ET_SIMPLE ||
-      code.error_type_ == ErrorCode::ET_TRAP) {
-    return code.err_;
-  } else if (code.error_type_ == ErrorCode::ET_COND) {
-    if (code.width_ == ErrorCode::TP_32BIT &&
-        (data.args[code.argno_] >> 32) &&
-        (data.args[code.argno_]&0xFFFFFFFF80000000ull)!=0xFFFFFFFF80000000ull){
-      return Sandbox::Unexpected64bitArgument().err();
+uint32_t EvaluateErrorCode(Sandbox *sandbox, const ErrorCode& code,
+                           const struct arch_seccomp_data& data) {
+  if (code.error_type() == ErrorCode::ET_SIMPLE ||
+      code.error_type() == ErrorCode::ET_TRAP) {
+    return code.err();
+  } else if (code.error_type() == ErrorCode::ET_COND) {
+    if (code.width() == ErrorCode::TP_32BIT &&
+        (data.args[code.argno()] >> 32) &&
+        (data.args[code.argno()] & 0xFFFFFFFF80000000ull) !=
+        0xFFFFFFFF80000000ull) {
+      return sandbox->Unexpected64bitArgument().err();
     }
-    switch (code.op_) {
+    switch (code.op()) {
     case ErrorCode::OP_EQUAL:
-      return EvaluateErrorCode((code.width_ == ErrorCode::TP_32BIT
-                                ? uint32_t(data.args[code.argno_])
-                                : data.args[code.argno_]) == code.value_
-                               ? *code.passed_
-                               : *code.failed_,
+      return EvaluateErrorCode(sandbox,
+                               (code.width() == ErrorCode::TP_32BIT
+                                ? uint32_t(data.args[code.argno()])
+                                : data.args[code.argno()]) == code.value()
+                               ? *code.passed()
+                               : *code.failed(),
                                data);
     case ErrorCode::OP_HAS_ALL_BITS:
-      return EvaluateErrorCode(((code.width_ == ErrorCode::TP_32BIT
-                                 ? uint32_t(data.args[code.argno_])
-                                 : data.args[code.argno_]) & code.value_)
-                               == code.value_
-                               ? *code.passed_
-                               : *code.failed_,
+      return EvaluateErrorCode(sandbox,
+                               ((code.width() == ErrorCode::TP_32BIT
+                                 ? uint32_t(data.args[code.argno()])
+                                 : data.args[code.argno()]) & code.value())
+                               == code.value()
+                               ? *code.passed()
+                               : *code.failed(),
                                data);
     case ErrorCode::OP_HAS_ANY_BITS:
-      return EvaluateErrorCode((code.width_ == ErrorCode::TP_32BIT
-                                ? uint32_t(data.args[code.argno_])
-                                : data.args[code.argno_]) & code.value_
-                               ? *code.passed_
-                               : *code.failed_,
+      return EvaluateErrorCode(sandbox,
+                               (code.width() == ErrorCode::TP_32BIT
+                                ? uint32_t(data.args[code.argno()])
+                                : data.args[code.argno()]) & code.value()
+                               ? *code.passed()
+                               : *code.failed(),
                                data);
     default:
       return SECCOMP_RET_INVALID;
@@ -91,19 +78,20 @@ uint32_t Verifier::EvaluateErrorCode(const ErrorCode& code,
   }
 }
 
-bool Verifier::VerifyErrorCode(const std::vector<struct sock_filter>& program,
-                               struct arch_seccomp_data *data,
-                               const ErrorCode& root_code,
-                               const ErrorCode& code,
-                               const char **err) {
-  if (code.error_type_ == ErrorCode::ET_SIMPLE ||
-      code.error_type_ == ErrorCode::ET_TRAP) {
-    uint32_t computed_ret = EvaluateBPF(program, *data, err);
+bool VerifyErrorCode(Sandbox *sandbox,
+                     const std::vector<struct sock_filter>& program,
+                     struct arch_seccomp_data *data,
+                     const ErrorCode& root_code,
+                     const ErrorCode& code,
+                     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);
     if (*err) {
       return false;
-    } else if (computed_ret != EvaluateErrorCode(root_code, *data)) {
+    } else if (computed_ret != EvaluateErrorCode(sandbox, root_code, *data)) {
       // For efficiency's sake, we'd much rather compare "computed_ret"
-      // against "code.err_". This works most of the time, but it doesn't
+      // against "code.err()". This works most of the time, but it doesn't
       // always work for nested conditional expressions. The test values
       // that we generate on the fly to probe expressions can trigger
       // code flow decisions in multiple nodes of the decision tree, and the
@@ -112,32 +100,34 @@ bool Verifier::VerifyErrorCode(const std::vector<struct sock_filter>& program,
       *err = "Exit code from BPF program doesn't match";
       return false;
     }
-  } else if (code.error_type_ == ErrorCode::ET_COND) {
-    if (code.argno_ < 0 || code.argno_ >= 6) {
+  } else if (code.error_type() == ErrorCode::ET_COND) {
+    if (code.argno() < 0 || code.argno() >= 6) {
       *err = "Invalid argument number in error code";
       return false;
     }
-    switch (code.op_) {
+    switch (code.op()) {
     case ErrorCode::OP_EQUAL:
       // Verify that we can check a 32bit value (or the LSB of a 64bit value)
       // for equality.
-      data->args[code.argno_] = code.value_;
-      if (!VerifyErrorCode(program, data, root_code, *code.passed_, err)) {
+      data->args[code.argno()] = code.value();
+      if (!VerifyErrorCode(sandbox, program, data, root_code,
+                           *code.passed(), err)) {
         return false;
       }
 
       // Change the value to no longer match and verify that this is detected
       // as an inequality.
-      data->args[code.argno_] = code.value_ ^ 0x55AA55AA;
-      if (!VerifyErrorCode(program, data, root_code, *code.failed_, err)) {
+      data->args[code.argno()] = code.value() ^ 0x55AA55AA;
+      if (!VerifyErrorCode(sandbox, program, data, root_code,
+                           *code.failed(), err)) {
         return false;
       }
 
       // BPF programs can only ever operate on 32bit values. So, we have
       // generated additional BPF instructions that inspect the MSB. Verify
       // that they behave as intended.
-      if (code.width_ == ErrorCode::TP_32BIT) {
-        if (code.value_ >> 32) {
+      if (code.width() == ErrorCode::TP_32BIT) {
+        if (code.value() >> 32) {
           SANDBOX_DIE("Invalid comparison of a 32bit system call argument "
                       "against a 64bit constant; this test is always false.");
         }
@@ -145,9 +135,10 @@ bool Verifier::VerifyErrorCode(const std::vector<struct sock_filter>& program,
         // If the system call argument was intended to be a 32bit parameter,
         // verify that it is a fatal error if a 64bit value is ever passed
         // here.
-        data->args[code.argno_] = 0x100000000ull;
-        if (!VerifyErrorCode(program, data, root_code,
-                             Sandbox::Unexpected64bitArgument(), err)) {
+        data->args[code.argno()] = 0x100000000ull;
+        if (!VerifyErrorCode(sandbox, program, data, root_code,
+                             sandbox->Unexpected64bitArgument(),
+                             err)) {
           return false;
         }
       } else {
@@ -157,8 +148,9 @@ bool Verifier::VerifyErrorCode(const std::vector<struct sock_filter>& program,
         // We only need to verify the behavior of the inequality test. We
         // know that the equality test already passed, as unlike the kernel
         // the Verifier does operate on 64bit quantities.
-        data->args[code.argno_] = code.value_ ^ 0x55AA55AA00000000ull;
-        if (!VerifyErrorCode(program, data, root_code, *code.failed_, err)) {
+        data->args[code.argno()] = code.value() ^ 0x55AA55AA00000000ull;
+        if (!VerifyErrorCode(sandbox, program, data, root_code,
+                             *code.failed(), err)) {
           return false;
         }
       }
@@ -172,37 +164,37 @@ bool Verifier::VerifyErrorCode(const std::vector<struct sock_filter>& program,
       // ones.
       {
         // Testing "any" bits against a zero mask is always false. So, there
-        // are some cases, where we expect tests to take the "failed_" branch
-        // even though this is a test that normally should take "passed_".
+        // are some cases, where we expect tests to take the "failed()" branch
+        // even though this is a test that normally should take "passed()".
         const ErrorCode& passed =
-          (!code.value_ && code.op_ == ErrorCode::OP_HAS_ANY_BITS) ||
+          (!code.value() && code.op() == ErrorCode::OP_HAS_ANY_BITS) ||
 
           // On a 32bit system, it is impossible to pass a 64bit value as a
           // system call argument. So, some additional tests always evaluate
           // as false.
-          ((code.value_ & ~uint64_t(uintptr_t(-1))) &&
-           code.op_ == ErrorCode::OP_HAS_ALL_BITS) ||
-          (code.value_ && !(code.value_ & uintptr_t(-1)) &&
-              code.op_ == ErrorCode::OP_HAS_ANY_BITS)
+          ((code.value() & ~uint64_t(uintptr_t(-1))) &&
+           code.op() == ErrorCode::OP_HAS_ALL_BITS) ||
+          (code.value() && !(code.value() & uintptr_t(-1)) &&
+           code.op() == ErrorCode::OP_HAS_ANY_BITS)
 
-          ? *code.failed_ : *code.passed_;
+          ? *code.failed() : *code.passed();
 
         // Similary, testing for "all" bits in a zero mask is always true. So,
         // some cases pass despite them normally failing.
         const ErrorCode& failed =
-          !code.value_ && code.op_ == ErrorCode::OP_HAS_ALL_BITS
-          ? *code.passed_ : *code.failed_;
+          !code.value() && code.op() == ErrorCode::OP_HAS_ALL_BITS
+          ? *code.passed() : *code.failed();
 
-        data->args[code.argno_] = code.value_ & uintptr_t(-1);
-        if (!VerifyErrorCode(program, data, root_code, passed, err)) {
+        data->args[code.argno()] = code.value() & uintptr_t(-1);
+        if (!VerifyErrorCode(sandbox, program, data, root_code, passed, err)) {
           return false;
         }
-        data->args[code.argno_] = uintptr_t(-1);
-        if (!VerifyErrorCode(program, data, root_code, passed, err)) {
+        data->args[code.argno()] = uintptr_t(-1);
+        if (!VerifyErrorCode(sandbox, program, data, root_code, passed, err)) {
           return false;
         }
-        data->args[code.argno_] = 0;
-        if (!VerifyErrorCode(program, data, root_code, failed, err)) {
+        data->args[code.argno()] = 0;
+        if (!VerifyErrorCode(sandbox, program, data, root_code, failed, err)) {
           return false;
         }
       }
@@ -218,55 +210,7 @@ bool Verifier::VerifyErrorCode(const std::vector<struct sock_filter>& program,
   return true;
 }
 
-uint32_t Verifier::EvaluateBPF(const std::vector<struct sock_filter>& program,
-                               const struct arch_seccomp_data& data,
-                               const char **err) {
-  *err = NULL;
-  if (program.size() < 1 || program.size() >= SECCOMP_MAX_PROGRAM_SIZE) {
-    *err = "Invalid program length";
-    return 0;
-  }
-  for (State state(program, data); !*err; ++state.ip) {
-    if (state.ip >= program.size()) {
-      *err = "Invalid instruction pointer in BPF program";
-      break;
-    }
-    const struct sock_filter& insn = program[state.ip];
-    switch (BPF_CLASS(insn.code)) {
-    case BPF_LD:
-      Ld(&state, insn, err);
-      break;
-    case BPF_JMP:
-      Jmp(&state, insn, err);
-      break;
-    case BPF_RET: {
-      uint32_t r = Ret(&state, insn, err);
-      switch (r & SECCOMP_RET_ACTION) {
-      case SECCOMP_RET_TRAP:
-      case SECCOMP_RET_ERRNO:
-      case SECCOMP_RET_ALLOW:
-        break;
-      case SECCOMP_RET_KILL:     // We don't ever generate this
-      case SECCOMP_RET_TRACE:    // We don't ever generate this
-      case SECCOMP_RET_INVALID:  // Should never show up in BPF program
-      default:
-        *err = "Unexpected return code found in BPF program";
-        return 0;
-      }
-      return r; }
-    case BPF_ALU:
-      Alu(&state, insn, err);
-      break;
-    default:
-      *err = "Unexpected instruction in BPF program";
-      break;
-    }
-  }
-  return 0;
-}
-
-void Verifier::Ld(State *state, const struct sock_filter& insn,
-                  const char **err) {
+void Ld(State *state, const struct sock_filter& insn, const char **err) {
   if (BPF_SIZE(insn.code) != BPF_W ||
       BPF_MODE(insn.code) != BPF_ABS) {
     *err = "Invalid BPF_LD instruction";
@@ -285,8 +229,7 @@ void Verifier::Ld(State *state, const struct sock_filter& insn,
   return;
 }
 
-void Verifier::Jmp(State *state, const struct sock_filter& insn,
-                   const char **err) {
+void Jmp(State *state, const struct sock_filter& insn, const char **err) {
   if (BPF_OP(insn.code) == BPF_JA) {
     if (state->ip + insn.k + 1 >= state->program.size() ||
         state->ip + insn.k + 1 <= state->ip) {
@@ -337,8 +280,7 @@ void Verifier::Jmp(State *state, const struct sock_filter& insn,
   }
 }
 
-uint32_t Verifier::Ret(State *, const struct sock_filter& insn,
-                       const char **err) {
+uint32_t Ret(State *, const struct sock_filter& insn, const char **err) {
   if (BPF_SRC(insn.code) != BPF_K) {
     *err = "Invalid BPF_RET instruction";
     return 0;
@@ -346,8 +288,7 @@ uint32_t Verifier::Ret(State *, const struct sock_filter& insn,
   return insn.k;
 }
 
-void Verifier::Alu(State *state, const struct sock_filter& insn,
-                   const char **err) {
+void Alu(State *state, const struct sock_filter& insn, const char **err) {
   if (BPF_OP(insn.code) == BPF_NEG) {
     state->accumulator = -state->accumulator;
     return;
@@ -410,5 +351,94 @@ void Verifier::Alu(State *state, const struct sock_filter& insn,
   }
 }
 
+}  // namespace
+
+bool Verifier::VerifyBPF(Sandbox *sandbox,
+                         const std::vector<struct sock_filter>& program,
+                         const Sandbox::Evaluators& evaluators,
+                         const char **err) {
+  *err = NULL;
+  if (evaluators.size() != 1) {
+    *err = "Not implemented";
+    return false;
+  }
+  Sandbox::EvaluateSyscall evaluate_syscall = evaluators.begin()->first;
+  void *aux                                 = evaluators.begin()->second;
+  for (SyscallIterator iter(false); !iter.Done(); ) {
+    uint32_t sysnum = iter.Next();
+    // We ideally want to iterate over the full system call range and values
+    // just above and just below this range. This gives us the full result set
+    // of the "evaluators".
+    // On Intel systems, this can fail in a surprising way, as a cleared bit 30
+    // indicates either i386 or x86-64; and a set bit 30 indicates x32. And
+    // unless we pay attention to setting this bit correctly, an early check in
+    // our BPF program will make us fail with a misleading error code.
+    struct arch_seccomp_data data = { static_cast<int>(sysnum),
+                                      static_cast<uint32_t>(SECCOMP_ARCH) };
+#if defined(__i386__) || defined(__x86_64__)
+#if defined(__x86_64__) && defined(__ILP32__)
+    if (!(sysnum & 0x40000000u)) {
+      continue;
+    }
+#else
+    if (sysnum & 0x40000000u) {
+      continue;
+    }
+#endif
+#endif
+    ErrorCode code = evaluate_syscall(sandbox, sysnum, aux);
+    if (!VerifyErrorCode(sandbox, program, &data, code, code, err)) {
+      return false;
+    }
+  }
+  return true;
+}
+
+uint32_t Verifier::EvaluateBPF(const std::vector<struct sock_filter>& program,
+                               const struct arch_seccomp_data& data,
+                               const char **err) {
+  *err = NULL;
+  if (program.size() < 1 || program.size() >= SECCOMP_MAX_PROGRAM_SIZE) {
+    *err = "Invalid program length";
+    return 0;
+  }
+  for (State state(program, data); !*err; ++state.ip) {
+    if (state.ip >= program.size()) {
+      *err = "Invalid instruction pointer in BPF program";
+      break;
+    }
+    const struct sock_filter& insn = program[state.ip];
+    switch (BPF_CLASS(insn.code)) {
+    case BPF_LD:
+      Ld(&state, insn, err);
+      break;
+    case BPF_JMP:
+      Jmp(&state, insn, err);
+      break;
+    case BPF_RET: {
+      uint32_t r = Ret(&state, insn, err);
+      switch (r & SECCOMP_RET_ACTION) {
+      case SECCOMP_RET_TRAP:
+      case SECCOMP_RET_ERRNO:
+      case SECCOMP_RET_ALLOW:
+        break;
+      case SECCOMP_RET_KILL:     // We don't ever generate this
+      case SECCOMP_RET_TRACE:    // We don't ever generate this
+      case SECCOMP_RET_INVALID:  // Should never show up in BPF program
+      default:
+        *err = "Unexpected return code found in BPF program";
+        return 0;
+      }
+      return r; }
+    case BPF_ALU:
+      Alu(&state, insn, err);
+      break;
+    default:
+      *err = "Unexpected instruction in BPF program";
+      break;
+    }
+  }
+  return 0;
+}
 
 }  // namespace

[jln (very slow on Chromium), 2013/02/15 20:58:25]

namespace playground 2