NonSFI sandbox: restrict futex(2) operations.

NonSFI sandbox: restrict futex(2) operations.

We restrict futex operations to everything but "priority inheritance".

BUG=408847
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32

Committed: https://crrev.com/f5afc495794d31855f805a359bde5dfa6561ea0a
Cr-Commit-Position: refs/heads/master@{#293776}

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

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

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

Mark: PTAL!

This is basically a copy of what I did for the "baseline" policy. (We decided
long ago that NonSFI policies should be independant).

If we think we can further tune this done with a little bit of work, I would
rather still land this and iterate from there.

https://codereview.chromium.org/549653002/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/549653002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:103: const int
kAllowedFutexFlags = FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME;
Should I remove FUTEX_CLOCK_REALTIME?

[Mark Seaborn, 2014-09-06 00:36:47]

LGTM.  When you CQ this, please add
"CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32", otherwise this
won't really be tested.

https://codereview.chromium.org/549653002/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/549653002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:103: const int
kAllowedFutexFlags = FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME;
On 2014/09/05 23:40:52, jln wrote:
> Should I remove FUTEX_CLOCK_REALTIME?

glibc might be using it.  I'm not sure.

It would be a very useful flag, except that the kernel seems to arbitrarily
disallow its use on the plain FUTEX_WAIT, which is unfortunate.

https://codereview.chromium.org/549653002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:106: FUTEX_WAIT,       
FUTEX_WAKE,    FUTEX_FD,          FUTEX_REQUEUE,
Don'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/549653002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:111: BoolExpr IsAllowedOp = (op
& kOperationMask) == kAllowedFutexOperations[0];
Style nit: shouldn't this be "is_allowed_op"?

https://codereview.chromium.org/549653002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:278: // TODO(hamaji): Allow only
FUTEX_PRIVATE_FLAG.
Move this comment into RestrictFutexOperation()?

https://codereview.chromium.org/549653002/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/549653002/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:330: base::Thread
thread("sandbox_tests");
Nit: fix indentation.

You might want to comment that the dtor joins the thread.  It's not obvious.

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

Thanks Mark!

https://chromiumcodereview.appspot.com/549653002/diff/1/components/nacl/loade...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://chromiumcodereview.appspot.com/549653002/diff/1/components/nacl/loade...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:106: FUTEX_WAIT,       
FUTEX_WAKE,    FUTEX_FD,          FUTEX_REQUEUE,
On 2014/09/06 00:36:47, Mark Seaborn wrote:
> Don't need FUTEX_FD.  The man page says "Because it was inherently racy,
> FUTEX_FD has been removed from Linux 2.6.26 onward."

Done.

https://chromiumcodereview.appspot.com/549653002/diff/1/components/nacl/loade...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:111: BoolExpr IsAllowedOp = (op
& kOperationMask) == kAllowedFutexOperations[0];
On 2014/09/06 00:36:47, Mark Seaborn wrote:
> Style nit: shouldn't this be "is_allowed_op"?

Done.

https://chromiumcodereview.appspot.com/549653002/diff/1/components/nacl/loade...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:278: // TODO(hamaji): Allow only
FUTEX_PRIVATE_FLAG.
On 2014/09/06 00:36:47, Mark Seaborn wrote:
> Move this comment into RestrictFutexOperation()?

Done.

https://chromiumcodereview.appspot.com/549653002/diff/1/components/nacl/loade...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://chromiumcodereview.appspot.com/549653002/diff/1/components/nacl/loade...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:330: base::Thread
thread("sandbox_tests");
On 2014/09/06 00:36:47, Mark Seaborn wrote:
> Nit: fix indentation.
> 
> You might want to comment that the dtor joins the thread.  It's not obvious.

Done.

[jln (very slow on Chromium), 2014-09-08 19:07:12]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-09-08 19:09:00]

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

[commit-bot: I haz the power, 2014-09-08 20:16:01]

Committed patchset #3 (id:40001) as 1fd2fcfe1a196091dfebb76a62c1514af0e2adbe

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

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