sandbox/linux/bpf_dsl: remove ErrorCode intermediary representation

sandbox/linux/bpf_dsl/policy_compiler.cc

Index: sandbox/linux/bpf_dsl/policy_compiler.cc
diff --git a/sandbox/linux/bpf_dsl/policy_compiler.cc b/sandbox/linux/bpf_dsl/policy_compiler.cc
index ad9ccbab61170df5e54733e096297844b5d5e7fa..cb6f00c114587b5e366bd2341ec4c3e5d4a64b41 100644
--- a/sandbox/linux/bpf_dsl/policy_compiler.cc
+++ b/sandbox/linux/bpf_dsl/policy_compiler.cc
@@ -14,7 +14,6 @@
 #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/errorcode.h"
 #include "sandbox/linux/bpf_dsl/policy.h"
 #include "sandbox/linux/bpf_dsl/seccomp_macros.h"
 #include "sandbox/linux/bpf_dsl/syscall_set.h"
@@ -91,7 +90,6 @@ PolicyCompiler::PolicyCompiler(const Policy* policy, TrapRegistry* registry)
       registry_(registry),
       escapepc_(0),
       panic_func_(DefaultPanic),
-      conds_(),
       gen_(),
       has_unsafe_traps_(HasUnsafeTraps(policy_)) {
   DCHECK(policy);
@@ -185,7 +183,7 @@ CodeGen::Node PolicyCompiler::MaybeAddEscapeHatch(CodeGen::Node rest) {
 }
 
 CodeGen::Node PolicyCompiler::DispatchSyscall() {
-  // Evaluate all possible system calls and group their ErrorCodes into
+  // Evaluate all possible system calls and group their Nodes into
   // ranges of identical codes.
   Ranges ranges;
   FindRanges(&ranges);
@@ -225,7 +223,7 @@ void PolicyCompiler::FindRanges(Ranges* ranges) {
   // int32_t, but BPF instructions always operate on unsigned quantities. We
   // deal with this disparity by enumerating from MIN_SYSCALL to MAX_SYSCALL,
   // and then verifying that the rest of the number range (both positive and
-  // negative) all return the same ErrorCode.
+  // negative) all return the same Node.
   const CodeGen::Node invalid_node = CompileResult(policy_->InvalidSyscall());
   uint32_t old_sysnum = 0;
   CodeGen::Node old_node =
@@ -277,68 +275,54 @@ CodeGen::Node PolicyCompiler::AssembleJumpTable(Ranges::const_iterator start,
 }
 
 CodeGen::Node PolicyCompiler::CompileResult(const ResultExpr& res) {
-  return RetExpression(res->Compile(this));
+  return res->Compile(this);
 }
 
-CodeGen::Node PolicyCompiler::RetExpression(const ErrorCode& err) {
-  switch (err.error_type()) {
-    case ErrorCode::ET_COND:
-      return CondExpression(err);
-    case ErrorCode::ET_SIMPLE:
-    case ErrorCode::ET_TRAP:
-      return gen_.MakeInstruction(BPF_RET + BPF_K, err.err());
-    default:
-      LOG(FATAL)
-          << "ErrorCode is not suitable for returning from a BPF program";
-      return CodeGen::kNullNode;
-  }
-}
-
-CodeGen::Node PolicyCompiler::CondExpression(const ErrorCode& cond) {
-  // Sanity check that |cond| makes sense.
-  CHECK(cond.argno_ >= 0 && cond.argno_ < 6) << "Invalid argument number "
-                                             << cond.argno_;
-  CHECK(cond.width_ == ErrorCode::TP_32BIT ||
-        cond.width_ == ErrorCode::TP_64BIT)
-      << "Invalid argument width " << cond.width_;
-  CHECK_NE(0U, cond.mask_) << "Zero mask is invalid";
-  CHECK_EQ(cond.value_, cond.value_ & cond.mask_)
-      << "Value contains masked out bits";
+CodeGen::Node PolicyCompiler::MaskedEqual(int argno,
+                                          size_t width,
+                                          uint64_t mask,
+                                          uint64_t value,
+                                          CodeGen::Node passed,
+                                          CodeGen::Node failed) {
+  // Sanity check that arguments make sense.
+  CHECK(argno >= 0 && argno < 6) << "Invalid argument number " << argno;
+  CHECK(width == 4 || width == 8) << "Invalid argument width " << width;
+  CHECK_NE(0U, mask) << "Zero mask is invalid";
+  CHECK_EQ(value, value & mask) << "Value contains masked out bits";
   if (sizeof(void*) == 4) {

[rickyz (no longer on Chrome), 2015/09/03 23:34:11]

(Question unrelated to your change): Is there ever likely to be a use case for
compiling policies for a 32-bit process from a 64-bit process (or is the
assumption that the build env is the same as the target env already baked in
elsewhere)? After seeing cross compilation with clang vs. gcc, I've become a fan
of supporting it in one build whenever possible :-)

[mdempsky, 2015/09/04 18:32:09]

Agreed that Clang's cross-compiler model is nicer, but at the moment for bpf_dsl
I expect it would require some extra complexity but wouldn't yield any immediate
benefits.

There are a few limitations that make it impractical to compile a policy within
one process and reuse it for a different process (e.g., trap registration is the
major one; but embedding PIDs is another), which limits the value of supporting
cross-compilation for "production" usage.

jln and I have also discussed that it might be nice for unit testing to be able
to more easily verify the 32-bit mode behavior on 64-bit.  However, the limiting
factor there is the bpf_dsl unit tests are still written in a very
integration-testy manner.

So, it's on my mind to remove these host-dependent limitations, just not a high
priority at the moment. :)

-    CHECK_EQ(ErrorCode::TP_32BIT, cond.width_)
-        << "Invalid width on 32-bit platform";
+    CHECK_EQ(4U, width) << "Invalid width on 32-bit platform";
   }
-  if (cond.width_ == ErrorCode::TP_32BIT) {
-    CHECK_EQ(0U, cond.mask_ >> 32) << "Mask exceeds argument size";
-    CHECK_EQ(0U, cond.value_ >> 32) << "Value exceeds argument size";
+  if (width == 4) {
+    CHECK_EQ(0U, mask >> 32) << "Mask exceeds argument size";
+    CHECK_EQ(0U, value >> 32) << "Value exceeds argument size";
   }
 
-  CodeGen::Node passed = RetExpression(*cond.passed_);
-  CodeGen::Node failed = RetExpression(*cond.failed_);
-
   // We want to emit code to check "(arg & mask) == value" where arg, mask, and
   // value are 64-bit values, but the BPF machine is only 32-bit. We implement
   // this by independently testing the upper and lower 32-bits and continuing to
   // |passed| if both evaluate true, or to |failed| if either evaluate false.
-  return CondExpressionHalf(cond,
-                            UpperHalf,
-                            CondExpressionHalf(cond, LowerHalf, passed, failed),
-                            failed);
+  return MaskedEqualHalf(argno, width, mask, value, ArgHalf::UPPER,
+                         MaskedEqualHalf(argno, width, mask, value,
+                                         ArgHalf::LOWER, passed, failed),
+                         failed);
 }
 
-CodeGen::Node PolicyCompiler::CondExpressionHalf(const ErrorCode& cond,
-                                                 ArgHalf half,
-                                                 CodeGen::Node passed,
-                                                 CodeGen::Node failed) {
-  if (cond.width_ == ErrorCode::TP_32BIT && half == UpperHalf) {
+CodeGen::Node PolicyCompiler::MaskedEqualHalf(int argno,
+                                              size_t width,
+                                              uint64_t full_mask,
+                                              uint64_t full_value,
+                                              ArgHalf half,
+                                              CodeGen::Node passed,
+                                              CodeGen::Node failed) {
+  if (width == 4 && half == ArgHalf::UPPER) {
     // Special logic for sanity checking the upper 32-bits of 32-bit system
     // call arguments.
 
     // TODO(mdempsky): Compile Unexpected64bitArgument() just per program.
-    CodeGen::Node invalid_64bit = RetExpression(Unexpected64bitArgument());
+    CodeGen::Node invalid_64bit = Unexpected64bitArgument();
 
-    const uint32_t upper = SECCOMP_ARG_MSB_IDX(cond.argno_);
-    const uint32_t lower = SECCOMP_ARG_LSB_IDX(cond.argno_);
+    const uint32_t upper = SECCOMP_ARG_MSB_IDX(argno);
+    const uint32_t lower = SECCOMP_ARG_LSB_IDX(argno);
 
     if (sizeof(void*) == 4) {
       // On 32-bit platforms, the upper 32-bits should always be 0:
@@ -381,10 +365,11 @@ CodeGen::Node PolicyCompiler::CondExpressionHalf(const ErrorCode& cond,
                 invalid_64bit)));
   }
 
-  const uint32_t idx = (half == UpperHalf) ? SECCOMP_ARG_MSB_IDX(cond.argno_)
-                                           : SECCOMP_ARG_LSB_IDX(cond.argno_);
-  const uint32_t mask = (half == UpperHalf) ? cond.mask_ >> 32 : cond.mask_;
-  const uint32_t value = (half == UpperHalf) ? cond.value_ >> 32 : cond.value_;
+  const uint32_t idx = (half == ArgHalf::UPPER) ? SECCOMP_ARG_MSB_IDX(argno)
+                                                : SECCOMP_ARG_LSB_IDX(argno);
+  const uint32_t mask = (half == ArgHalf::UPPER) ? full_mask >> 32 : full_mask;
+  const uint32_t value =
+      (half == ArgHalf::UPPER) ? full_value >> 32 : full_value;
 
   // Emit a suitable instruction sequence for (arg & mask) == value.
 
@@ -439,12 +424,12 @@ CodeGen::Node PolicyCompiler::CondExpressionHalf(const ErrorCode& cond,
               BPF_JMP + BPF_JEQ + BPF_K, value, passed, failed)));
 }
 
-ErrorCode PolicyCompiler::Unexpected64bitArgument() {
-  return panic_func_("Unexpected 64bit argument detected")->Compile(this);
+CodeGen::Node PolicyCompiler::Unexpected64bitArgument() {
+  return CompileResult(panic_func_("Unexpected 64bit argument detected"));
 }
 
-ErrorCode PolicyCompiler::Error(int err) {
-  if (has_unsafe_traps_) {
+CodeGen::Node PolicyCompiler::Return(uint32_t ret) {
+  if (has_unsafe_traps_ && (ret & SECCOMP_RET_ACTION) == SECCOMP_RET_ERRNO) {
     // When inside an UnsafeTrap() callback, we want to allow all system calls.
     // This means, we must conditionally disable the sandbox -- and that's not
     // something that kernel-side BPF filters can do, as they cannot inspect
@@ -454,17 +439,18 @@ ErrorCode PolicyCompiler::Error(int err) {
     // The performance penalty for this extra round-trip to user-space is not
     // actually that bad, as we only ever pay it for denied system calls; and a
     // typical program has very few of these.
-    return Trap(ReturnErrno, reinterpret_cast<void*>(err), true);
+    return Trap(ReturnErrno, reinterpret_cast<void*>(ret & SECCOMP_RET_DATA),
+                true);
   }
 
-  return ErrorCode(err);
+  return gen_.MakeInstruction(BPF_RET + BPF_K, ret);
 }
 
-ErrorCode PolicyCompiler::Trap(TrapRegistry::TrapFnc fnc,
-                               const void* aux,
-                               bool safe) {
+CodeGen::Node PolicyCompiler::Trap(TrapRegistry::TrapFnc fnc,
+                                   const void* aux,
+                                   bool safe) {
   uint16_t trap_id = registry_->Add(fnc, aux, safe);
-  return ErrorCode(trap_id, fnc, aux, safe);
+  return gen_.MakeInstruction(BPF_RET + BPF_K, SECCOMP_RET_TRAP + trap_id);
 }
 
 bool PolicyCompiler::IsRequiredForUnsafeTrap(int sysno) {
@@ -476,19 +462,5 @@ bool PolicyCompiler::IsRequiredForUnsafeTrap(int sysno) {
   return false;
 }
 
-ErrorCode PolicyCompiler::CondMaskedEqual(int argno,
-                                          ErrorCode::ArgType width,
-                                          uint64_t mask,
-                                          uint64_t value,
-                                          const ErrorCode& passed,
-                                          const ErrorCode& failed) {
-  return ErrorCode(argno,
-                   width,
-                   mask,
-                   value,
-                   &*conds_.insert(passed).first,
-                   &*conds_.insert(failed).first);
-}
-
 }  // namespace bpf_dsl
 }  // namespace sandbox