Revert of bpf_dsl: decouple PolicyCompiler from Syscall

Revert of bpf_dsl: decouple PolicyCompiler from Syscall (patchset #3 id:40001 of https://codereview.chromium.org/939943002/)

Reason for revert:
This CL seem to increase nacl_helper size and results in Linux_x64 failure.

Original issue's description:
> bpf_dsl: decouple PolicyCompiler from Syscall
>
> 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
>
> Committed: https://crrev.com/242ad8711d9f0d58fb45b134468ad68b0835fad2
> Cr-Commit-Position: refs/heads/master@{#317237}

TBR=jln@chromium.org,mdempsky@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=449357
Committed: https://crrev.com/e83e901c290d599fcbd82da8a436d520aca94879
Cr-Commit-Position: refs/heads/master@{#317295}

[kaliamoorthi, 2015-02-20 11:06:03]

Created Revert of bpf_dsl: decouple PolicyCompiler from Syscall

[commit-bot: I haz the power, 2015-02-20 11:07:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/937303005/1

[commit-bot: I haz the power, 2015-02-20 11:08:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-20 11:08:32]

Failed to apply patch for sandbox/linux/bpf_dsl/policy_compiler.cc:
While running git apply --index -3 -p1;
  error: patch failed: sandbox/linux/bpf_dsl/policy_compiler.cc:20
  Falling back to three-way merge...
  Applied patch to 'sandbox/linux/bpf_dsl/policy_compiler.cc' with conflicts.
  U sandbox/linux/bpf_dsl/policy_compiler.cc

Patch:       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
bcfd21b77658d9abc8bb1abd4260b66b7f12fd23..18ed7be582a363367c23a39c10230aa42b4acd39
100644
--- a/sandbox/linux/bpf_dsl/policy_compiler.cc
+++ b/sandbox/linux/bpf_dsl/policy_compiler.cc
@@ -20,6 +20,7 @@
 #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 {
@@ -85,7 +86,6 @@
 PolicyCompiler::PolicyCompiler(const Policy* policy, TrapRegistry* registry)
     : policy_(policy),
       registry_(registry),
-      escapepc_(0),
       conds_(),
       gen_(),
       has_unsafe_traps_(HasUnsafeTraps(policy_)) {
@@ -106,8 +106,11 @@
     // 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.
-
-    CHECK_NE(0U, escapepc_) << "UnsafeTrap() requires a valid escape PC";
+    if (Syscall::Call(-1) == -1 && errno == ENOSYS) {
+      SANDBOX_DIE(
+          "Support for UnsafeTrap() has not yet been ported to this "
+          "architecture");
+    }
 
     for (int sysnum : kSyscallsRequiredForUnsafeTraps) {
       if (!policy_->EvaluateSyscall(sysnum)->IsAllow()) {
@@ -131,10 +134,6 @@
   scoped_ptr<CodeGen::Program> program(new CodeGen::Program());
   gen_.Compile(AssemblePolicy(), program.get());
   return program.Pass();
-}
-
-void PolicyCompiler::DangerousSetEscapePC(uint64_t escapepc) {
-  escapepc_ = escapepc;
 }
 
 CodeGen::Node PolicyCompiler::AssemblePolicy() {
@@ -163,13 +162,12 @@
     return rest;
   }
 
-  // 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);
+  // 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);
 
   // BPF cannot do native 64-bit comparisons, so we have to compare
   // both 32-bit halves of the instruction pointer. If they match what
@@ -181,10 +179,10 @@
   return gen_.MakeInstruction(
       BPF_LD + BPF_W + BPF_ABS, SECCOMP_IP_LSB_IDX,
       gen_.MakeInstruction(
-          BPF_JMP + BPF_JEQ + BPF_K, lopc,
+          BPF_JMP + BPF_JEQ + BPF_K, low,
           gen_.MakeInstruction(
               BPF_LD + BPF_W + BPF_ABS, SECCOMP_IP_MSB_IDX,
-              gen_.MakeInstruction(BPF_JMP + BPF_JEQ + BPF_K, hipc,
+              gen_.MakeInstruction(BPF_JMP + BPF_JEQ + BPF_K, hi,
                                    CompileResult(Allow()), rest)),
           rest));
 }

[commit-bot: I haz the power, 2015-02-20 11:08:34]

The CQ bit was unchecked by commit-bot@chromium.org

[mdempsky, 2015-02-20 11:10:09]

How much larger does this make nacl_helper?

[kaliamoorthi, 2015-02-20 11:12:31]

On 2015/02/20 11:10:09, mdempsky wrote:
> How much larger does this make nacl_helper?

Please check
http://build.chromium.org/p/chromium/builders/Linux%20x64?numbuilds=50

I believe 3%

[mdempsky, 2015-02-20 11:33:20]

The CQ bit was checked by mdempsky@chromium.org

[mdempsky, 2015-02-20 11:33:20]

lgtm

[commit-bot: I haz the power, 2015-02-20 11:34:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/937303005/1

[commit-bot: I haz the power, 2015-02-20 11:34:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-20 11:34:56]

Failed to apply patch for sandbox/linux/bpf_dsl/policy_compiler.cc:
While running git apply --index -3 -p1;
  error: patch failed: sandbox/linux/bpf_dsl/policy_compiler.cc:20
  Falling back to three-way merge...
  Applied patch to 'sandbox/linux/bpf_dsl/policy_compiler.cc' with conflicts.
  U sandbox/linux/bpf_dsl/policy_compiler.cc

Patch:       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
bcfd21b77658d9abc8bb1abd4260b66b7f12fd23..18ed7be582a363367c23a39c10230aa42b4acd39
100644
--- a/sandbox/linux/bpf_dsl/policy_compiler.cc
+++ b/sandbox/linux/bpf_dsl/policy_compiler.cc
@@ -20,6 +20,7 @@
 #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 {
@@ -85,7 +86,6 @@
 PolicyCompiler::PolicyCompiler(const Policy* policy, TrapRegistry* registry)
     : policy_(policy),
       registry_(registry),
-      escapepc_(0),
       conds_(),
       gen_(),
       has_unsafe_traps_(HasUnsafeTraps(policy_)) {
@@ -106,8 +106,11 @@
     // 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.
-
-    CHECK_NE(0U, escapepc_) << "UnsafeTrap() requires a valid escape PC";
+    if (Syscall::Call(-1) == -1 && errno == ENOSYS) {
+      SANDBOX_DIE(
+          "Support for UnsafeTrap() has not yet been ported to this "
+          "architecture");
+    }
 
     for (int sysnum : kSyscallsRequiredForUnsafeTraps) {
       if (!policy_->EvaluateSyscall(sysnum)->IsAllow()) {
@@ -131,10 +134,6 @@
   scoped_ptr<CodeGen::Program> program(new CodeGen::Program());
   gen_.Compile(AssemblePolicy(), program.get());
   return program.Pass();
-}
-
-void PolicyCompiler::DangerousSetEscapePC(uint64_t escapepc) {
-  escapepc_ = escapepc;
 }
 
 CodeGen::Node PolicyCompiler::AssemblePolicy() {
@@ -163,13 +162,12 @@
     return rest;
   }
 
-  // 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);
+  // 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);
 
   // BPF cannot do native 64-bit comparisons, so we have to compare
   // both 32-bit halves of the instruction pointer. If they match what
@@ -181,10 +179,10 @@
   return gen_.MakeInstruction(
       BPF_LD + BPF_W + BPF_ABS, SECCOMP_IP_LSB_IDX,
       gen_.MakeInstruction(
-          BPF_JMP + BPF_JEQ + BPF_K, lopc,
+          BPF_JMP + BPF_JEQ + BPF_K, low,
           gen_.MakeInstruction(
               BPF_LD + BPF_W + BPF_ABS, SECCOMP_IP_MSB_IDX,
-              gen_.MakeInstruction(BPF_JMP + BPF_JEQ + BPF_K, hipc,
+              gen_.MakeInstruction(BPF_JMP + BPF_JEQ + BPF_K, hi,
                                    CompileResult(Allow()), rest)),
           rest));
 }

[commit-bot: I haz the power, 2015-02-20 11:35:03]

The CQ bit was unchecked by commit-bot@chromium.org

[mdempsky, 2015-02-20 11:35:08]

On 2015/02/20 11:12:31, kaliamoorthi wrote:
> On 2015/02/20 11:10:09, mdempsky wrote:
> > How much larger does this make nacl_helper?
> 
> Please check
> http://build.chromium.org/p/chromium/builders/Linux%20x64?numbuilds=50
> 
> I believe 3%

Looks like 0.03%.  Seems silly to have to revert these for now, but okay.

[mdempsky, 2015-02-20 11:36:39]

On 2015/02/20 11:34:56, I haz the power (commit-bot) wrote:
> Failed to apply patch for sandbox/linux/bpf_dsl/policy_compiler.cc:

Probably because of https://codereview.chromium.org/944763004/

[mdempsky, 2015-02-20 11:37:06]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2015-02-20 11:37:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/937303005/1

[commit-bot: I haz the power, 2015-02-20 11:38:06]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-20 11:38:45]

Patchset 1 (id:??) landed as
https://crrev.com/e83e901c290d599fcbd82da8a436d520aca94879
Cr-Commit-Position: refs/heads/master@{#317295}

[kaliamoorthi, 2015-02-20 12:18:29]

On 2015/02/20 11:36:39, mdempsky wrote:
> On 2015/02/20 11:34:56, I haz the power (commit-bot) wrote:
> > Failed to apply patch for sandbox/linux/bpf_dsl/policy_compiler.cc:
> 
> Probably because of https://codereview.chromium.org/944763004/

No revert of this one solves the problem.