Restrict mmap(2) flags for x64.

content/common/sandbox_seccomp_bpf_linux.cc

Index: content/common/sandbox_seccomp_bpf_linux.cc
===================================================================
--- content/common/sandbox_seccomp_bpf_linux.cc	(revision 200134)
+++ content/common/sandbox_seccomp_bpf_linux.cc	(working copy)
@@ -100,6 +100,7 @@
   // likelihood of collision with mapped pages.
   syscall |= ((args.args[0] & 0xffUL) << 12);
   syscall |= ((args.args[1] & 0xffUL) << 20);
+syscall = args.args[3] & 0xffffffffUL;
   // Purposefully dereference the syscall as an address so it'll show up very
   // clearly and easily in crash dumps.
   volatile char* addr = reinterpret_cast<volatile char*>(syscall);
@@ -658,16 +659,16 @@
   switch (sysno) {
     case __NR_brk:
     case __NR_mlock:
+    case __NR_mprotect:
+    case __NR_munlock:
+    case __NR_munmap:
+      return true;
 #if defined(__i386__) || defined(__x86_64__)
-    case __NR_mmap:   // TODO(jln): to restrict flags.
+    case __NR_mmap:

[jln (very slow on Chromium), 2013/05/15 20:01:05]

Nit: if we keep it this way, let's move mmap and mmap2 down to keep alphabetical
ordering!

 #endif
 #if defined(__i386__) || defined(__arm__)
     case __NR_mmap2:
 #endif
-    case __NR_mprotect:
-    case __NR_munlock:
-    case __NR_munmap:
-      return true;
     case __NR_madvise:
     case __NR_mincore:
     case __NR_mlockall:
@@ -1236,6 +1237,16 @@
   }
 }
 
+ErrorCode RestrictMmapFlags(Sandbox *sandbox) {
+  // These flags are allowed. Significantly, we don't permit MAP_HUGETLB, or
+  // the newer flags such as MAP_POPULATE.
+  uint32_t mask = ~(MAP_SHARED | MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK |

[jln (very slow on Chromium), 2013/05/15 20:01:05]

want to rename that variable denied_mask to make things more clear?

+                    MAP_NORESERVE | MAP_FIXED);
+  return sandbox->Cond(3, ErrorCode::TP_32BIT, ErrorCode::OP_HAS_ANY_BITS,

[jln (very slow on Chromium), 2013/05/15 20:01:05]

Nit:

I guess we should have provided a OP_HAS_NO_BITS helper, it's a bit confusing
that we're inverting the order of what's allowed / denied in comparison to the
other similar functions.

Can you try to make things a little be more clear by having denied_mask on the
previous line and the two Trap and ERR_ALLOWED each on their own line ?

+                       mask, sandbox->Trap(CrashSIGSYS_Handler, NULL),
+                       ErrorCode(ErrorCode::ERR_ALLOWED));
+}
+
 ErrorCode BaselinePolicy(Sandbox *sandbox, int sysno) {
 #if defined(__x86_64__) || defined(__arm__)
   if (sysno == __NR_socketpair) {
@@ -1246,12 +1257,24 @@
                          sandbox->Trap(CrashSIGSYS_Handler, NULL));
   }
 #endif
-  if (sysno == __NR_madvise) {
+  switch (sysno) {

[jln (very slow on Chromium), 2013/05/15 20:01:05]

If you want to make this a switch, make sure you start the switch on top.

Then you'd need a default: case as per style guide requirements.

I wonder if it's worth it though, I found the if () return logic fairly
readable. Your call.

+  case __NR_madvise:
     // Only allow MADV_DONTNEED (aka MADV_FREE).
     return sandbox->Cond(2, ErrorCode::TP_32BIT,
                          ErrorCode::OP_EQUAL, MADV_DONTNEED,
                          ErrorCode(ErrorCode::ERR_ALLOWED),
                          ErrorCode(EPERM));
+#if defined(__x86_64__)
+  case __NR_mmap:
+    return RestrictMmapFlags(sandbox);
+#elif defined(__i386__)
+  case __NR_mmap:
+    return ErrorCode(ErrorCode::ERR_ALLOWED);
+#endif
+#if defined(__i386__) || defined(__arm__)
+  case __NR_mmap2:
+    return ErrorCode(ErrorCode::ERR_ALLOWED);
+#endif
   }
 
   if (IsBaselinePolicyAllowed(sysno)) {

[jln (DO NOT USE THIS), 2013/05/15 20:10:14]

Another remark:

The goal of putting things *above* IsBaseLinePolicyAllowed() is that we do deny
thing that would otherwise be allowed. You can put all of these (which are now
defined by default in the baseline policy) below.