Linux sandbox: whitelist allowed Futex operations.

sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc

Index: sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc
diff --git a/sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc b/sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc
index 2f7578586ed8640ffc79be80cb428b646aafa816..799c529390791e3df6dacd8b5e7e6d9e1056cbcb 100644
--- a/sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc
+++ b/sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc
@@ -25,13 +25,16 @@
 #include "sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h"
 #include "sandbox/linux/seccomp-bpf/linux_seccomp.h"
 #include "sandbox/linux/seccomp-bpf/sandbox_bpf.h"
-#include "sandbox/linux/services/android_futex.h"
 
 #if defined(OS_ANDROID)
+
+#include "sandbox/linux/services/android_futex.h"
+
 #if !defined(F_DUPFD_CLOEXEC)
 #define F_DUPFD_CLOEXEC (F_LINUX_SPECIFIC_BASE + 6)
 #endif
-#endif
+
+#endif  // defined(OS_ANDROID)
 
 #if defined(__arm__) && !defined(MAP_STACK)
 #define MAP_STACK 0x20000  // Daisy build environment has old headers.
@@ -207,19 +210,20 @@ ResultExpr RestrictKillTarget(pid_t target_pid, int sysno) {
 }
 
 ResultExpr RestrictFutex() {
-  // In futex.c, the kernel does "int cmd = op & FUTEX_CMD_MASK;". We need to
-  // make sure that the combination below will cover every way to get
-  // FUTEX_CMP_REQUEUE_PI.
-  const int kBannedFutexBits =
-      ~(FUTEX_CMD_MASK | FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME);
-  COMPILE_ASSERT(0 == kBannedFutexBits,
-                 need_to_explicitly_blacklist_more_bits);
+  const int kAllowedFutexFlags = FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME;
+  const int kOperationMask = ~kAllowedFutexFlags;
+  const int kAllowedFutexOperations[] = {
+      FUTEX_WAIT,        FUTEX_WAKE,    FUTEX_FD,          FUTEX_REQUEUE,

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

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

+      FUTEX_CMP_REQUEUE, FUTEX_WAKE_OP, FUTEX_WAIT_BITSET, FUTEX_WAKE_BITSET};
 
   const Arg<int> op(1);
-  return If(op == FUTEX_CMP_REQUEUE_PI || op == FUTEX_CMP_REQUEUE_PI_PRIVATE ||
-                op == (FUTEX_CMP_REQUEUE_PI | FUTEX_CLOCK_REALTIME) ||
-                op == (FUTEX_CMP_REQUEUE_PI_PRIVATE | FUTEX_CLOCK_REALTIME),
-            CrashSIGSYSFutex()).Else(Allow());
+
+  BoolExpr IsAllowedOp = (op & kOperationMask) == kAllowedFutexOperations[0];
+  for (size_t i = 1; i < arraysize(kAllowedFutexOperations); ++i) {
+    IsAllowedOp =
+        IsAllowedOp || ((op & kOperationMask) == kAllowedFutexOperations[i]);

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

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:48]

Yep. mdempsky is implementing better support in the compiler
(https://chromiumcodereview.appspot.com/438683004/), but v1 won't optimize
anything.
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]

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

+  }
+  return If(IsAllowedOp, Allow()).Else(CrashSIGSYSFutex());
 }
 
 }  // namespace sandbox.