ARM GPU process Seccomp-BPF policy.

content/common/sandbox_seccomp_bpf_linux.cc

Index: content/common/sandbox_seccomp_bpf_linux.cc
diff --git a/content/common/sandbox_seccomp_bpf_linux.cc b/content/common/sandbox_seccomp_bpf_linux.cc
index 2f3f0efa449c5308268f543f97e946cdee1cab24..4d0b3842cbdd102f70b4068cf887aea73c1acd10 100644
--- a/content/common/sandbox_seccomp_bpf_linux.cc
+++ b/content/common/sandbox_seccomp_bpf_linux.cc
@@ -1269,7 +1269,7 @@ ErrorCode BaselinePolicy(Sandbox *sandbox, int sysno) {
     // be denied gracefully right away.
     return sandbox->Trap(CrashSIGSYS_Handler, NULL);
   }
-  // In any other case crash the program with our SIGSYS handler
+  // In any other case crash the program with our SIGSYS handler.
   return sandbox->Trap(CrashSIGSYS_Handler, NULL);
 }
 
@@ -1280,6 +1280,22 @@ ErrorCode GpuProcessPolicy(Sandbox *sandbox, int sysno,
     case __NR_ioctl:
     case __NR_sched_getaffinity:
     case __NR_sched_setaffinity:
+#if defined(__arm__)

[jln (very slow on Chromium), 2013/04/19 21:28:27]

Let's make another GPU process policy instead. Name GpuArmIntelDriverPolicy or
something along these lines.
Make it clear in a comment that the if (IsArchitectureArm()) is just a
convenient shortcut to detect that configuration, but that the policy is
specific to one driver.

From that policy, you can inherit the current, generic GPU policy.

I also think you should just allow all of IsAdvancedScheduler(), it's not much
more attack surface than what this allows. If we're concerned about the IO
Scheduler, let's split it  up in its own function.

For access(), if we need to allow it, we should add it to the broker process
that allows open(). Add it as a TODO on the existing bug for NONBLOCK / CLOEXEC.

Also remember: use IseArchitectureArm() for anything that doesn't trigger a
compile error, and #ifdef as last resort.

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

Thinking about it a bit more, perhaps the policies should just be fully separate
(and inherit on the baseline policy).

Inheritance from an intermediate GPU policy would create more complexity for
little gain. Inheritance from the generic GPU policy is probably just wrong.

Let me know what you think.

[Jorge Lucangeli Obes, 2013/04/23 00:13:20]

Done by inheriting from Baseline, agreed that makes more sense.

+    // ARM GPU sandbox is started earlier so we need to allow more stuff.
+    case __NR_access:
+    case __NR_socket:
+    case __NR_socketpair:
+    case __NR_connect:
+    case __NR_getpeername:
+    case __NR_getsockname:
+    case __NR_sched_get_priority_min:
+    case __NR_sched_get_priority_max:
+    case __NR_sched_getparam:
+    case __NR_sched_getscheduler:
+    case __NR_sched_setscheduler:
+    case __NR_sysinfo:
+    case __NR_uname:
+#endif  // defined(__arm__)
       return ErrorCode(ErrorCode::ERR_ALLOWED);
     case __NR_open:
     case __NR_openat:
@@ -1463,13 +1479,40 @@ void InitGpuBrokerProcess(BrokerProcess** broker_process) {
   static const char kDriRcPath[] = "/etc/drirc";
   static const char kDriCard0Path[] = "/dev/dri/card0";
 
+  // On ARM we're enabling the sandbox before the X connection is made,

[jln (very slow on Chromium), 2013/04/19 21:28:27]

Similarly, let's cleanly split this as its own ARM-with-intel-GPU function.

You could make helper functions such as:
AddX86IntGpuWhiteListedFiled(std::.....* read_whitelist,...* write_whitelist);
that you call from here.

InitGpuBrokerProcess() can take the policy as an argument to determine which one
of the helpers to call.

[Jorge Lucangeli Obes, 2013/04/23 00:13:20]

Split ARM portion into its own function.

I ended up needing different "sandbox enabling" callbacks. I could have added a
|policy| parameter to BrokerProcess::Init, but decided against it to keep
coupling to a minimum. We could eventually do some base::Bind magic to have
proper currying in Enable*GpuBrokerPolicyCallback.

+  // so we need to allow access to |.Xauthority|.
+  static const char kXAutorityPath[] = "/home/chronos/.Xauthority";
+
+  // Devices and files needed by the ARM GPU userspace.
+  static const char kMali0Path[] = "/dev/mali0";
+  static const char kLibGlesPath[] = "/usr/lib/libGLESv2.so.2";
+  static const char kLibEglPath[] = "/usr/lib/libEGL.so.1";
+
+  // Devices needed for video decode acceleration on ARM.
+  static const char kDevMfcDecPath[] = "/dev/mfc-dec";
+  static const char kDevGsc1Path[] = "/dev/gsc1";
+
   CHECK(broker_process);
   CHECK(*broker_process == NULL);
 
   std::vector<std::string> read_whitelist;
+  if (IsArchitectureArm()) {
+    read_whitelist.push_back(kXAutorityPath);
+    read_whitelist.push_back(kMali0Path);
+    read_whitelist.push_back(kLibGlesPath);
+    read_whitelist.push_back(kLibEglPath);
+    read_whitelist.push_back(kDevMfcDecPath);
+    read_whitelist.push_back(kDevGsc1Path);
+  }
   read_whitelist.push_back(kDriCard0Path);
   read_whitelist.push_back(kDriRcPath);
+
   std::vector<std::string> write_whitelist;
+  if (IsArchitectureArm()) {
+    write_whitelist.push_back(kMali0Path);
+    write_whitelist.push_back(kDevMfcDecPath);
+    write_whitelist.push_back(kDevGsc1Path);
+  }
   write_whitelist.push_back(kDriCard0Path);
 
   *broker_process = new BrokerProcess(read_whitelist, write_whitelist);
@@ -1482,10 +1525,10 @@ void InitGpuBrokerProcess(BrokerProcess** broker_process) {
 void WarmupPolicy(Sandbox::EvaluateSyscall policy,
                   BrokerProcess** broker_process) {
   if (policy == GpuProcessPolicy) {
-    if (IsArchitectureX86_64() || IsArchitectureI386()) {
-      // Create a new broker process.
-      InitGpuBrokerProcess(broker_process);
+    // Create a new broker process.
+    InitGpuBrokerProcess(broker_process);
 
+    if (IsArchitectureX86_64() || IsArchitectureI386()) {
       // Accelerated video decode dlopen()'s a shared object
       // inside the sandbox, so preload it now.
       if (IsAcceleratedVideoDecodeEnabled()) {