Non-SFI mode: Sandbox support for NaCl async-signals.

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 475c28c28c637f4975287402af9c28b65132117c..9587010e02298f49e2afa127ee84eb410b6d4284 100644
--- a/components/nacl/loader/nonsfi/nonsfi_sandbox.cc
+++ b/components/nacl/loader/nonsfi/nonsfi_sandbox.cc
@@ -22,6 +22,7 @@
 #include "sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.h"
 #include "sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.h"
 #include "sandbox/linux/system_headers/linux_futex.h"
+#include "sandbox/linux/system_headers/linux_signal.h"
 #include "sandbox/linux/system_headers/linux_syscalls.h"
 
 // Chrome OS Daisy (ARM) build environment and PNaCl toolchain do not define
@@ -83,7 +84,9 @@ ResultExpr RestrictClone() {
   clone_flags |= CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID;
 #endif
   const Arg<int> flags(0);
-  return If(flags == clone_flags, Allow()).Else(CrashSIGSYSClone());
+  return If(flags == clone_flags ||

[Mark Seaborn, 2015/08/13 23:38:10]

How about adding a TODO to remove the variant without CLONE_PARENT_SETTID after
the NaCl-side change is rolled in?

[Luis Héctor Chávez, 2015/08/14 00:29:32]

Done.

+                flags == (clone_flags | CLONE_PARENT_SETTID),
+            Allow()).Else(CrashSIGSYSClone());
 }
 
 ResultExpr RestrictFutexOperation() {
@@ -146,6 +149,13 @@ ResultExpr RestrictMmap() {
             Allow()).Else(CrashSIGSYS());
 }
 
+ResultExpr RestrictTgkill() {
+  const Arg<int> tgid(0), signum(2);
+  // Only sending SIGUSR1 to a thread in the same process is allowed.
+  return If(tgid == getpid() && signum == LINUX_SIGUSR1,

[Mark Seaborn, 2015/08/13 23:38:10]

Should we check that the thread ID is positive too, just to be on the safe side?

The Linux man page for tgkill() doesn't document any special behaviour for 0 or
negative values, but the Linux man pages are notoriously incomplete.

[Luis Héctor Chávez, 2015/08/14 00:29:32]

Arg<int> does not support greater-than comparison :( (although a mask operation
with an Arg<uint32_t> could work in a pinch).

Looks like Linux does the lookup for the thread ID using equality (kernel/pid.c,
find_pid_ns()) and does not seem to give any special treatment to negative
values. Furthermore, negative thread IDs are invalid since alloc_pidmap()
returns -1 on failure. We should be safe with the code as-is.

[Mark Seaborn, 2015/08/14 20:12:41]

I suppose you can check that:

 * arg != 0
 * (arg & (1 << 31)) == 0  // arg is non-negative

And comment about the lack of a greater-than op.

Actually, can you add a test for this in
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc, please?  I forgot
about that before.
That may be true for current versions, but we're slightly paranoid that future
Linux versions might introduce weird corner cases. :-)

+            Allow()).Else(CrashSIGSYS());
+}
+
 #if !defined(OS_NACL_NONSFI) && (defined(__x86_64__) || defined(__arm__))
 ResultExpr RestrictSocketpair() {
   // Only allow AF_UNIX, PF_UNIX. Crash if anything else is seen.
@@ -304,6 +314,9 @@ ResultExpr NaClNonSfiBPFSandboxPolicy::EvaluateSyscall(int sysno) const {
 #endif
 #endif
 
+    case __NR_tgkill:
+      return RestrictTgkill();
+
     case __NR_brk:
       // The behavior of brk on Linux is different from other system
       // calls. It does not return errno but the current break on