NonSFI sandbox: restrict futex(2) operations.

components/nacl/loader/nonsfi/nonsfi_sandbox.cc

Index: components/nacl/loader/nonsfi/nonsfi_sandbox.cc
diff --git a/components/nacl/loader/nonsfi/nonsfi_sandbox.cc b/components/nacl/loader/nonsfi/nonsfi_sandbox.cc
index b5fe899746c3bc7a70b62193ccc693766e917216..87e04085628ee8acaaf4a59a79885c689d651b5f 100644
--- a/components/nacl/loader/nonsfi/nonsfi_sandbox.cc
+++ b/components/nacl/loader/nonsfi/nonsfi_sandbox.cc
@@ -6,12 +6,14 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <linux/futex.h>
 #include <linux/net.h>
+#include <sys/mman.h>
 #include <sys/prctl.h>
 #include <sys/ptrace.h>
-#include <sys/mman.h>
 #include <sys/socket.h>
 #include <sys/syscall.h>
+#include <sys/time.h>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
@@ -30,9 +32,11 @@
 
 using sandbox::CrashSIGSYS;
 using sandbox::CrashSIGSYSClone;
+using sandbox::CrashSIGSYSFutex;
 using sandbox::CrashSIGSYSPrctl;
 using sandbox::bpf_dsl::Allow;
 using sandbox::bpf_dsl::Arg;
+using sandbox::bpf_dsl::BoolExpr;
 using sandbox::bpf_dsl::Error;
 using sandbox::bpf_dsl::If;
 using sandbox::bpf_dsl::ResultExpr;
@@ -95,6 +99,23 @@ ResultExpr RestrictClone() {
             Allow()).Else(CrashSIGSYSClone());
 }
 
+ResultExpr RestrictFutexOperation() {
+  const int kAllowedFutexFlags = FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME;

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

Should I remove FUTEX_CLOCK_REALTIME?

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

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.

+  const int kOperationMask = ~kAllowedFutexFlags;
+  const int kAllowedFutexOperations[] = {
+      FUTEX_WAIT,        FUTEX_WAKE,    FUTEX_FD,          FUTEX_REQUEUE,

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

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

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

Done.

+      FUTEX_CMP_REQUEUE, FUTEX_WAKE_OP, FUTEX_WAIT_BITSET, FUTEX_WAKE_BITSET};
+
+  const Arg<int> op(1);
+
+  BoolExpr IsAllowedOp = (op & kOperationMask) == kAllowedFutexOperations[0];

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

Style nit: shouldn't this be "is_allowed_op"?

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

Done.

+  for (size_t i = 1; i < arraysize(kAllowedFutexOperations); ++i) {
+    IsAllowedOp =
+        IsAllowedOp || ((op & kOperationMask) == kAllowedFutexOperations[i]);
+  }
+  return If(IsAllowedOp, Allow()).Else(CrashSIGSYSFutex());
+}
+
 ResultExpr RestrictPrctl() {
   // base::PlatformThread::SetName() uses PR_SET_NAME so we return
   // EPERM for it. Otherwise, we will raise SIGSYS.
@@ -214,8 +235,6 @@ ResultExpr NaClNonSfiBPFSandboxPolicy::EvaluateSyscall(int sysno) const {
 #elif defined(__x86_64__)
     case __NR_fstat:
 #endif
-    // TODO(hamaji): Allow only FUTEX_PRIVATE_FLAG.
-    case __NR_futex:
     // TODO(hamaji): Remove the need of gettid. Currently, this is
     // called from PlatformThread::CurrentId().
     case __NR_gettid:
@@ -256,6 +275,10 @@ ResultExpr NaClNonSfiBPFSandboxPolicy::EvaluateSyscall(int sysno) const {
 #endif
       return RestrictFcntlCommands();
 
+    // TODO(hamaji): Allow only FUTEX_PRIVATE_FLAG.

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

Move this comment into RestrictFutexOperation()?

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

Done.

+    case __NR_futex:
+      return RestrictFutexOperation();
+
 #if defined(__x86_64__)
     case __NR_mmap:
 #endif