Decouple Trap from ErrorCode

Decouple Trap from ErrorCode

This CL makes a few changes in the direction of removing dependencies
on ErrorCode by pushing them up a layer:

- Trap now simply uses TrapKeys directly to store trap function and
metadata instead of needlessly encoding them as ErrorCode.

- MakeTrap returns a bare trap ID and it's instead the caller's
responsibility to encode in an ErrorCode if needed, which is now
handled by ErrorCode's trap constructor.

- Also, ErrorCodeFromTrapId() is replaced by IsSafeTrapId() to answer
the one question that SandboxBPF was interested in knowing about
existing traps.

- Change a few SandboxBPF trap-constructing functions into static
functions since they don't depend on any SandboxBPF instance member
variables.

BUG=414363
Committed: https://crrev.com/af240444899ad7ff5c7f0209c5865ce36ac9c92a
Cr-Commit-Position: refs/heads/master@{#295121}

[mdempsky, 2014-09-15 20:15:23]

mdempsky@chromium.org changed reviewers:
+ jln@chromium.org, leecam@chromium.org

[leecam, 2014-09-16 12:36:45]

LGTM - This is so much cleaner!
One thing I did use was ErrorCodeFromTrapId in the logging stuff to get the
function that would be called for a trap. I guess I'll need to expose something
to return me the TrapFunc from the id?

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
File sandbox/linux/seccomp-bpf/trap.cc (right):

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
sandbox/linux/seccomp-bpf/trap.cc:320: // events.
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.

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
sandbox/linux/seccomp-bpf/trap.cc:350: trap_array_size_++;
why don't we just make this an atomic and fix the TODO?

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
File sandbox/linux/seccomp-bpf/trap.h (right):

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
sandbox/linux/seccomp-bpf/trap.h:115: size_t volatile trap_array_size_;  //
Currently used size of array
why are these volatile?

[jln (very slow on Chromium), 2014-09-16 17:47:46]

lgtm, nice cleanup!

https://codereview.chromium.org/572753002/diff/20001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/trap.cc (right):

https://codereview.chromium.org/572753002/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/trap.cc:339: // TODO(mdempsky): Try to clean this up
using base/atomicops or C++11
It doesn't change much here, but we should also stop assuming being
mono-threaded at this point.

At some point we may want to be able to refine the sandbox in a multi-threaded
environment (with TSYNC), it could prove very useful.

[mdempsky, 2014-09-16 18:48:20]

> One thing I did use was ErrorCodeFromTrapId in the logging stuff to get the
> function that would be called for a trap. I guess I'll need to expose
something
> to return me the TrapFunc from the id?

Ah, good point.  Looking at your logging CL, I suspect adding a function like
this to Trap should do the trick:

  static intptr_t InvokeTrap(uint16_t id, const struct arch_seccomp_data& args);

but I'll followup with some more thoughts in the crbug.

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
File sandbox/linux/seccomp-bpf/trap.cc (right):

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
sandbox/linux/seccomp-bpf/trap.cc:320: // events.
On 2014/09/16 12:36:44, leecam wrote:
> 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?

Yeah, more or less.

> 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.

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.

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
sandbox/linux/seccomp-bpf/trap.cc:350: trap_array_size_++;
On 2014/09/16 12:36:44, leecam wrote:
> why don't we just make this an atomic and fix the TODO?

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. :)

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
File sandbox/linux/seccomp-bpf/trap.h (right):

https://codereview.chromium.org/572753002/diff/1/sandbox/linux/seccomp-bpf/tr...
sandbox/linux/seccomp-bpf/trap.h:115: size_t volatile trap_array_size_;  //
Currently used size of array
On 2014/09/16 12:36:44, leecam wrote:
> why are these volatile?

Reverted.

My rationale was roughly that async signal handlers are only allowed to access
global variables by storing to volatile sig_atomic_t values. These variables are
modified by normal code while the SIGSYS handler is unblocked, so theoretically
they need some stricter access patterns than normal variables.

But since we currently require(?) single-threaded execution and SIGSYS is
actually only triggered synchronously (by calling a signal handler), the async
concerns don't quite apply.

I've reverted these changes for now in favor of a simple "TODO" in trap.cc, and
I'll update the issue with some longer term thoughts on how to fix it.

https://codereview.chromium.org/572753002/diff/20001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/trap.cc (right):

https://codereview.chromium.org/572753002/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/trap.cc:339: // TODO(mdempsky): Try to clean this up
using base/atomicops or C++11
On 2014/09/16 17:47:46, jln (late on IPC reviews) wrote:
> It doesn't change much here, but we should also stop assuming being
> mono-threaded at this point.
> 
> At some point we may want to be able to refine the sandbox in a multi-threaded
> environment (with TSYNC), it could prove very useful.

Yep, I'll followup on the crbug with more details.

[mdempsky, 2014-09-16 18:48:27]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-09-16 18:49:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/572753002/20001

[commit-bot: I haz the power, 2014-09-16 19:16:38]

Committed patchset #2 (id:20001) as 01c27cbe0b693cb5a0b274a32a4cf88a5eafeea1

[commit-bot: I haz the power, 2014-09-16 19:18:23]

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