Decouple Trap from ErrorCode

sandbox/linux/seccomp-bpf/trap.cc

Index: sandbox/linux/seccomp-bpf/trap.cc
diff --git a/sandbox/linux/seccomp-bpf/trap.cc b/sandbox/linux/seccomp-bpf/trap.cc
index ce906fcb2e7be45d9da985871b5b61fba82fdfc2..bb0245eab07440ec3d54abd98c57638c3ee2aa42 100644
--- a/sandbox/linux/seccomp-bpf/trap.cc
+++ b/sandbox/linux/seccomp-bpf/trap.cc
@@ -7,14 +7,15 @@
 #include <errno.h>
 #include <signal.h>
 #include <string.h>
-#include <sys/prctl.h>
 #include <sys/syscall.h>
 
+#include <algorithm>
 #include <limits>
 
 #include "base/logging.h"
-#include "sandbox/linux/seccomp-bpf/codegen.h"
+#include "build/build_config.h"
 #include "sandbox/linux/seccomp-bpf/die.h"
+#include "sandbox/linux/seccomp-bpf/linux_seccomp.h"
 #include "sandbox/linux/seccomp-bpf/syscall.h"
 
 // Android's signal.h doesn't define ucontext etc.
@@ -24,6 +25,12 @@
 
 namespace {
 
+struct arch_sigsys {
+  void* ip;
+  int nr;
+  unsigned int arch;
+};
+
 const int kCapacityIncrement = 20;
 
 // Unsafe traps can only be turned on, if the user explicitly allowed them
@@ -200,8 +207,8 @@ void Trap::SigSys(int nr, siginfo_t* info, void* void_context) {
                        SECCOMP_PARM6(ctx));
 #endif  // defined(__mips__)
   } else {
-    const ErrorCode& err = trap_array_[info->si_errno - 1];
-    if (!err.safe_) {
+    const TrapKey& trap = trap_array_[info->si_errno - 1];
+    if (!trap.safe) {
       SetIsInSigHandler();
     }
 
@@ -221,7 +228,7 @@ void Trap::SigSys(int nr, siginfo_t* info, void* void_context) {
 
     // Now call the TrapFnc callback associated with this particular instance
     // of SECCOMP_RET_TRAP.
-    rc = err.fnc_(data, err.aux_);
+    rc = trap.fnc(data, const_cast<void*>(trap.aux));
   }
 
   // Update the CPU register that stores the return code of the system call
@@ -243,11 +250,11 @@ bool Trap::TrapKey::operator<(const TrapKey& o) const {
   }
 }
 
-ErrorCode Trap::MakeTrap(TrapFnc fnc, const void* aux, bool safe) {
+uint16_t Trap::MakeTrap(TrapFnc fnc, const void* aux, bool safe) {
   return GetInstance()->MakeTrapImpl(fnc, aux, safe);
 }
 
-ErrorCode Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
+uint16_t Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
   if (!safe && !SandboxDebuggingAllowedByUser()) {
     // Unless the user set the CHROME_SANDBOX_DEBUGGING environment variable,
     // we never return an ErrorCode that is marked as "unsafe". This also
@@ -257,20 +264,19 @@ ErrorCode Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
 
     // This SANDBOX_DIE() can optionally be removed. It won't break security,
     // but it might make error messages from the BPF compiler a little harder
-    // to understand. Removing the SANDBOX_DIE() allows callers to easyly check
+    // to understand. Removing the SANDBOX_DIE() allows callers to easily check
     // whether unsafe traps are supported (by checking whether the returned
     // ErrorCode is ET_INVALID).
     SANDBOX_DIE(
         "Cannot use unsafe traps unless CHROME_SANDBOX_DEBUGGING "
         "is enabled");
 
-    return ErrorCode();
+    return 0;
   }
 
   // Each unique pair of TrapFnc and auxiliary data make up a distinct instance
   // of a SECCOMP_RET_TRAP.
   TrapKey key(fnc, aux, safe);
-  TrapIds::const_iterator iter = trap_ids_.find(key);
 
   // We return unique identifiers together with SECCOMP_RET_TRAP. This allows
   // us to associate trap with the appropriate handler. The kernel allows us
@@ -280,25 +286,26 @@ ErrorCode Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
   // trivially look them up from our signal handler without making any system
   // calls that might be async-signal-unsafe.
   // In order to do so, we store all of our traps in a C-style trap_array_.
-  uint16_t id;
+
+  TrapIds::const_iterator iter = trap_ids_.find(key);
   if (iter != trap_ids_.end()) {
     // We have seen this pair before. Return the same id that we assigned
     // earlier.
-    id = iter->second;
-  } else {
-    // This is a new pair. Remember it and assign a new id.
-    if (trap_array_size_ >= SECCOMP_RET_DATA /* 0xFFFF */ ||
-        trap_array_size_ >= std::numeric_limits<typeof(id)>::max()) {
+    return iter->second;
+  }
+
+  // This is a new pair. Remember it and assign a new id.
+  if (trap_array_size_ >= SECCOMP_RET_DATA /* 0xFFFF */ ||
+      trap_array_size_ >= std::numeric_limits<uint16_t>::max()) {
       // In practice, this is pretty much impossible to trigger, as there
       // are other kernel limitations that restrict overall BPF program sizes.
       SANDBOX_DIE("Too many SECCOMP_RET_TRAP callback instances");
     }
-    id = trap_array_size_ + 1;
 
     // Our callers ensure that there are no other threads accessing trap_array_
     // concurrently (typically this is done by ensuring that we are single-
     // threaded while the sandbox is being set up). But we nonetheless are
-    // modifying a life data structure that could be accessed any time a
+    // modifying a live data structure that could be accessed any time a
     // system call is made; as system calls could be triggering SIGSYS.
     // So, we have to be extra careful that we update trap_array_ atomically.
     // In particular, this means we shouldn't be using realloc() to resize it.
@@ -313,8 +320,9 @@ ErrorCode Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
     // events.

[leecam, 2014/09/16 12:36:44]

This code is to ensure that we can add new Traps atomically in case a bpf policy
is currently in place and using these structures during a SIGSYS?

As our base policy now blocks the seccomp syscall and we should lock down the
prctl so it couldnt do seccomp either it would not make sense to allow for
layered sandboxes where we allow users to set another sandbox policy. If we
could test in StartSandbox if one had already been applied to the process and
fail we could make this code much simpler. i.e we could ensure all traps are
added before we register the signal handler.

 

[mdempsky, 2014/09/16 18:48:19]

Yeah, more or less.
I think jln@ mentioned he discussed this with you, but for the codereview
record: we expect in the future we might have practical uses for stacked sandbox
policies, so it's not clear we want to disallow that just yet.

     if (trap_array_size_ >= trap_array_capacity_) {
       trap_array_capacity_ += kCapacityIncrement;
-      ErrorCode* old_trap_array = trap_array_;
-      ErrorCode* new_trap_array = new ErrorCode[trap_array_capacity_];
+      TrapKey* old_trap_array = trap_array_;
+      TrapKey* new_trap_array = new TrapKey[trap_array_capacity_];
+      std::copy_n(old_trap_array, trap_array_size_, new_trap_array);
 
       // Language specs are unclear on whether the compiler is allowed to move
       // the "delete[]" above our preceding assignments and/or memory moves,
@@ -327,19 +335,20 @@ ErrorCode Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
       // legitimate worry; but they at least thought that the barrier is
       // sufficient to prevent the (so far hypothetical) problem of re-ordering
       // of instructions by the compiler.
-      memcpy(new_trap_array, trap_array_, trap_array_size_ * sizeof(ErrorCode));
+      // TODO(mdempsky): Replace with sequentially-consistent atomic store.
       asm volatile("" : "=r"(new_trap_array) : "0"(new_trap_array) : "memory");
       trap_array_ = new_trap_array;
       asm volatile("" : "=r"(trap_array_) : "0"(trap_array_) : "memory");
 
       delete[] old_trap_array;
     }
-    trap_ids_[key] = id;
-    trap_array_[trap_array_size_] = ErrorCode(fnc, aux, safe, id);
-    return trap_array_[trap_array_size_++];
-  }
 
-  return ErrorCode(fnc, aux, safe, id);
+    uint16_t id = trap_array_size_ + 1;
+    trap_ids_[key] = id;
+    trap_array_[trap_array_size_] = key;
+    // TODO(mdempsky): Atomic increment.
+    trap_array_size_++;

[leecam, 2014/09/16 12:36:44]

why don't we just make this an atomic and fix the TODO?

[mdempsky, 2014/09/16 18:48:19]

Just because I think doing that correctly is actually a bit of non-trivial work
(and thus requires some more thought and then probably more back-and-forth
during code reviews) and can be done independently of other things, whereas
removing dependencies on ErrorCode is necessary for me to later delete ErrorCode
all together. :)

+    return id;
 }
 
 bool Trap::SandboxDebuggingAllowedByUser() const {
@@ -370,12 +379,11 @@ bool Trap::EnableUnsafeTrapsInSigSysHandler() {
   return trap->has_unsafe_traps_;
 }
 
-ErrorCode Trap::ErrorCodeFromTrapId(uint16_t id) {
+bool Trap::IsSafeTrapId(uint16_t id) {
   if (global_trap_ && id > 0 && id <= global_trap_->trap_array_size_) {
-    return global_trap_->trap_array_[id - 1];
-  } else {
-    return ErrorCode();
+    return global_trap_->trap_array_[id - 1].safe;
   }
+  return false;
 }
 
 Trap* Trap::global_trap_;