Decouple Trap from ErrorCode

sandbox/linux/seccomp-bpf/trap.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // 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/trap.h"
 
 #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.
 #if defined(OS_ANDROID)
 #include "sandbox/linux/services/android_ucontext.h"
 #endif
 
 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
 // by setting the CHROME_SANDBOX_DEBUGGING environment variable.
 const char kSandboxDebuggingEnv[] = "CHROME_SANDBOX_DEBUGGING";
 
 // We need to tell whether we are performing a "normal" callback, or
 // whether we were called recursively from within a UnsafeTrap() callback.
 // This is a little tricky to do, because we need to somehow get access to
 // per-thread data from within a signal context. Normal TLS storage is not
@@ skipping 156 matching lines @@
 #else
     rc = Syscall::Call(SECCOMP_SYSCALL(ctx),
                        SECCOMP_PARM1(ctx),
                        SECCOMP_PARM2(ctx),
                        SECCOMP_PARM3(ctx),
                        SECCOMP_PARM4(ctx),
                        SECCOMP_PARM5(ctx),
                        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();
     }
 
     // Copy the seccomp-specific data into a arch_seccomp_data structure. This
     // is what we are showing to TrapFnc callbacks that the system call
     // evaluator registered with the sandbox.
     struct arch_seccomp_data data = {
         static_cast<int>(SECCOMP_SYSCALL(ctx)),
         SECCOMP_ARCH,
         reinterpret_cast<uint64_t>(sigsys.ip),
         {static_cast<uint64_t>(SECCOMP_PARM1(ctx)),
          static_cast<uint64_t>(SECCOMP_PARM2(ctx)),
          static_cast<uint64_t>(SECCOMP_PARM3(ctx)),
          static_cast<uint64_t>(SECCOMP_PARM4(ctx)),
          static_cast<uint64_t>(SECCOMP_PARM5(ctx)),
          static_cast<uint64_t>(SECCOMP_PARM6(ctx))}};
 
     // 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
   // that we just handled, and restore "errno" to the value that it had
   // before entering the signal handler.
   Syscall::PutValueInUcontext(rc, ctx);
   errno = old_errno;
 
   return;
 }
 
 bool Trap::TrapKey::operator<(const TrapKey& o) const {
   if (fnc != o.fnc) {
     return fnc < o.fnc;
   } else if (aux != o.aux) {
     return aux < o.aux;
   } else {
     return safe < o.safe;
   }
 }
 
-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
     // means, the BPF compiler will never emit code that allow unsafe system
     // calls to by-pass the filter (because they use the magic return address
     // from Syscall::Call(-1)).
 
     // 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
   // identifiers in the range from 0 to SECCOMP_RET_DATA (0xFFFF). We want to
   // avoid 0, as it could be confused for a trap without any specific id.
   // The nice thing about sequentially numbered identifiers is that we can also
   // 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.
     // Instead, we allocate a new array, copy the values, and then switch the
     // pointer. We only really care about the pointer being updated atomically
     // and the data that is pointed to being valid, as these are the only
     // values accessed from the signal handler. It is OK if trap_array_size_
     // is inconsistent with the pointer, as it is monotonously increasing.
     // Also, we only care about compiler barriers, as the signal handler is
     // triggered synchronously from a system call. We don't have to protect
     // against issues with the memory model or with completely asynchronous
     // 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,
       // iff the compiler believes that "delete[]" doesn't have any other
       // global side-effects.
       // We insert optimization barriers to prevent this from happening.
       // The first barrier is probably not needed, but better be explicit in
       // what we want to tell the compiler.
       // The clang developer mailing list couldn't answer whether this is a
       // 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;
     }
+
+    uint16_t id = trap_array_size_ + 1;
     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);
+    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 {
   const char* debug_flag = getenv(kSandboxDebuggingEnv);
   return debug_flag && *debug_flag;
 }
 
 bool Trap::EnableUnsafeTrapsInSigSysHandler() {
   Trap* trap = GetInstance();
   if (!trap->has_unsafe_traps_) {
@@ skipping 10 matching lines @@
     } else {
       SANDBOX_INFO(
           "Cannot disable sandbox and use unsafe traps unless "
           "CHROME_SANDBOX_DEBUGGING is turned on first");
     }
   }
   // Returns the, possibly updated, value of has_unsafe_traps_.
   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_;
 
 }  // namespace sandbox