bpf_dsl: decouple PolicyCompiler from Syscall

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}

[mdempsky, 2015-02-19 10:19:41]

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

[jln (very slow on Chromium), 2015-02-19 19:49:14]

This is a good cleanup!

I'm only worried about "burying" the backdoor a little too much.

Do you mind spraying some more checks for DangerousSetBackdoorEscapePC() around?

Only relying on the non-existence of unsafetraps seems a little too brittle.

https://chromiumcodereview.appspot.com/939943002/diff/1/sandbox/linux/bpf_dsl...
File sandbox/linux/bpf_dsl/policy_compiler.h (right):

https://chromiumcodereview.appspot.com/939943002/diff/1/sandbox/linux/bpf_dsl...
sandbox/linux/bpf_dsl/policy_compiler.h:32: uint64_t escapepc = 0);
I'm worried about making it too easy to inject a backdoor.

How about:

1. Instead of a constructor parameter, we require calling a clear
DangerousSetBackdoorEscapePC() API?

2. CHECK that kSandboxDebuggingEnv is set before calling
DangerousSetBackdoorEscapePC().

3. I would even add the same CHECK to MaybeAddEscapeHatch() for good measure.

[mdempsky, 2015-02-20 03:20:32]

https://codereview.chromium.org/939943002/diff/1/sandbox/linux/bpf_dsl/policy...
File sandbox/linux/bpf_dsl/policy_compiler.h (right):

https://codereview.chromium.org/939943002/diff/1/sandbox/linux/bpf_dsl/policy...
sandbox/linux/bpf_dsl/policy_compiler.h:32: uint64_t escapepc = 0);
On 2015/02/19 19:49:14, jln wrote:
> I'm worried about making it too easy to inject a backdoor.

FWIW, this just lets the "user" control what PC is used for the escape hatch. 
All of the same safe guards to prevent unsafe traps in production still existed,
even if someone accidentally passes a bogus PC value here.

> How about:
> 
> 1. Instead of a constructor parameter, we require calling a clear
> DangerousSetBackdoorEscapePC() API?

Done.

> 2. CHECK that kSandboxDebuggingEnv is set before calling
> DangerousSetBackdoorEscapePC().

Done.

> 3. I would even add the same CHECK to MaybeAddEscapeHatch() for good measure.

I added an extra call to registry_->EnableUnsafeTraps(), since I don't want
bpf_dsl::PolicyCompiler to depend on the specifics of kSandboxDebuggingEnv.

[jln (very slow on Chromium), 2015-02-20 03:28:24]

lgtm

You could also replace reading has_unsafe_trap_ with HasUnsafeTraps() which
would return the former with an additional CHECK() in the true case. But we're
probably at a decent paranoia level already.

[mdempsky, 2015-02-20 03:34:32]

The CQ bit was checked by mdempsky@chromium.org

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

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

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

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-20 04:11:52]

Patchset 3 (id:??) landed as
https://crrev.com/242ad8711d9f0d58fb45b134468ad68b0835fad2
Cr-Commit-Position: refs/heads/master@{#317237}

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

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/937303005/ by kaliamoorthi@chromium.org.

The reason for reverting is: This CL seem to increase nacl_helper size and
results in Linux_x64 failure..