Linux sandbox: whitelist allowed Futex operations.

Linux sandbox: whitelist allowed Futex operations.

Remove the existing blacklist and whitelist allowed futex operations instead.
The semantics change a little bit as none of the "priority inheritance" operations
are allowed.

BUG=408847
R=mdempsky@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/a1d175fe57f21101a1a6ad94cd2ee5c450781337

[jln (very slow on Chromium), 2014-09-05 21:30:47]

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

[jln (very slow on Chromium), 2014-09-05 21:30:52]

Matthew: PTAL!

[mdempsky, 2014-09-05 21:37:18]

LGTM

Once https://codereview.chromium.org/438683004/ is submitted, we can write this
even more simply as:

    Switch(op & kOperationMask)
      .CASES((FUTEX_WAIT, FUTEX_WAKE, ...), Allow())
      .Default(CrashSIGSYSFutex());

[jln (very slow on Chromium), 2014-09-05 22:24:22]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-09-05 22:25:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jln@chromium.org/550473002/40001

[jln (very slow on Chromium), 2014-09-05 23:01:58]

Committed patchset #3 (id:40001) manually as a1d175f (presubmit successful).

[Mark Seaborn, 2014-09-06 00:29:28]

mseaborn@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2014-09-06 00:29:28]

https://codereview.chromium.org/550473002/diff/40001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc
(right):

https://codereview.chromium.org/550473002/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:216:
FUTEX_WAIT,        FUTEX_WAKE,    FUTEX_FD,          FUTEX_REQUEUE,
You shouldn't need FUTEX_FD.  The man page says "Because it was inherently racy,
FUTEX_FD has been removed from Linux 2.6.26 onward."

https://codereview.chromium.org/550473002/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:224:
IsAllowedOp || ((op & kOperationMask) == kAllowedFutexOperations[i]);
Does this end up calculating "op & kOperationMask" multiple times unnecessarily,
BTW?  I'm not sure what the generated BPF is like for this.

An alternative might be:

  int kOpMask = 31;
  IsAllowed =
    (op & ~(kAllowedFutexFlags | kOpMask)) == 0 &&  // check for disallowed flag
    ((1 << (op & kOpMask)) & kAllowedFutexOpsMask) != 0;  // check futex op

where kAllowedFutexOpsMask =
  (1 << FUTEX_WAIT) |
  (1 << FUTEX_WAKE) | ...;

This probably isn't worth micro-optimising, though. :-)

[jln (very slow on Chromium), 2014-09-06 00:36:49]

https://codereview.chromium.org/550473002/diff/40001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc
(right):

https://codereview.chromium.org/550473002/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:224:
IsAllowedOp || ((op & kOperationMask) == kAllowedFutexOperations[i]);
On 2014/09/06 00:29:28, Mark Seaborn wrote:
> Does this end up calculating "op & kOperationMask" multiple times
unnecessarily,
> BTW?  I'm not sure what the generated BPF is like for this.

Yep. mdempsky is implementing better support in the compiler
(https://chromiumcodereview.appspot.com/438683004/), but v1 won't optimize
anything.


> An alternative might be:
> 
>   int kOpMask = 31;
>   IsAllowed =
>     (op & ~(kAllowedFutexFlags | kOpMask)) == 0 &&  // check for disallowed
flag
>     ((1 << (op & kOpMask)) & kAllowedFutexOpsMask) != 0;  // check futex op
> 
> where kAllowedFutexOpsMask =
>   (1 << FUTEX_WAIT) |
>   (1 << FUTEX_WAKE) | ...;
> 
> This probably isn't worth micro-optimising, though. :-)

It won't work, the compiler wouldn't allow the "1<< op" operation in the current
version unfortunately.

[mdempsky, 2014-09-06 00:38:10]

https://codereview.chromium.org/550473002/diff/40001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc
(right):

https://codereview.chromium.org/550473002/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:224:
IsAllowedOp || ((op & kOperationMask) == kAllowedFutexOperations[i]);
On 2014/09/06 00:29:28, Mark Seaborn wrote:
> Does this end up calculating "op & kOperationMask" multiple times
unnecessarily,
> BTW?

Currently, yes.  I'm hoping we can make the compiler optimize cases like this in
the future (and this should be easier after
https://codereview.chromium.org/438683004/).

> I'm not sure what the generated BPF is like for this.

Currently, it'll look like

    LDW  [arg1_lower32]
    AND  kOperationMask
    JEQ  FUTEX_WAIT, passed, (next)
    LDW  [arg1_lower32]
    AND  kOperationMask
    JEQ  FUTEX_WAKE, passed, (next)
    ...

(Actually, I think it'll be even worse and re-check the upper 32-bits each time
too.)

> An alternative might be:
> 
>   int kOpMask = 31;
>   IsAllowed =
>     (op & ~(kAllowedFutexFlags | kOpMask)) == 0 &&  // check for disallowed
flag
>     ((1 << (op & kOpMask)) & kAllowedFutexOpsMask) != 0;  // check futex op

bpf_dsl doesn't currently support "1 << arg" expressions, but it seems like
something I could look into implementing.

I might prefer to implement that as an optimization for when bpf_dsl notices
that the Switch/CASES is matching on a bunch of small integer values, so policy
code can stay simple.

[commit-bot: I haz the power, 2014-09-10 03:41:47]

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