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..856c60f2e5ea645d29d164b59b5bf0d0559500fb 100644
--- a/content/common/sandbox_seccomp_bpf_linux.cc
+++ b/content/common/sandbox_seccomp_bpf_linux.cc
@@ -1269,11 +1269,11 @@ 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);
 }
 
-// x86_64/i386 for now. Needs to be adapted and tested for ARM.
+// x86_64/i386.
 ErrorCode GpuProcessPolicy(Sandbox *sandbox, int sysno,
                            void *broker_process) {
   switch(sysno) {
@@ -1283,7 +1283,7 @@ ErrorCode GpuProcessPolicy(Sandbox *sandbox, int sysno,
       return ErrorCode(ErrorCode::ERR_ALLOWED);
     case __NR_open:
     case __NR_openat:
-        return sandbox->Trap(GpuOpenSIGSYS_Handler, broker_process);
+      return sandbox->Trap(GpuOpenSIGSYS_Handler, broker_process);
     default:
 #if defined(__x86_64__) || defined(__arm__)
       if (IsSystemVSharedMemory(sysno))
@@ -1297,7 +1297,7 @@ ErrorCode GpuProcessPolicy(Sandbox *sandbox, int sysno,
   }
 }
 
-// x86_64/i386 for now. Needs to be adapted and tested for ARM.
+// x86_64/i386.
 // A GPU broker policy is the same as a GPU policy with open and
 // openat allowed.
 ErrorCode GpuBrokerProcessPolicy(Sandbox *sandbox, int sysno, void *aux) {
@@ -1312,6 +1312,57 @@ ErrorCode GpuBrokerProcessPolicy(Sandbox *sandbox, int sysno, void *aux) {
   }
 }
 
+// ARM Mali GPU process sandbox.
+ErrorCode ArmMaliGpuProcessPolicy(Sandbox *sandbox, int sysno,
+                                  void *broker_process) {
+  switch(sysno) {
+    case __NR_ioctl:
+#if defined(__arm__)
+    // ARM GPU sandbox is started earlier so we need to allow more stuff.

[jln (very slow on Chromium), 2013/04/23 02:22:47]

Maybe add a clear comment: this means that network access will be allowed in the
GPU sandbox.

It's clearly not ideal, but we can take care of this later. Do you know what's
happening ?

Does it need actual network access or just local sockets? No way we could
restrict with parameter inspection ?

[Jorge Lucangeli Obes, 2013/04/23 17:20:41]

I think we might be able to, but I'd rather get FS lockdown first and then
tighten this up. I'll update the comment.

+    case __NR_access:

[jln (very slow on Chromium), 2013/04/23 02:22:47]

As discussed, let's get rid of that once you can ;)

[Jorge Lucangeli Obes, 2013/04/23 17:20:41]

Added TODO. Since this CL does *not* enable the sandbox, I can make sure I only
enable the sandbox once the broker changes land.

+    case __NR_socket:
+    case __NR_socketpair:
+    case __NR_connect:
+    case __NR_getpeername:

[jln (very slow on Chromium), 2013/04/23 02:22:47]

Please sort!

[Jorge Lucangeli Obes, 2013/04/23 17:20:41]

Done.

+    case __NR_getsockname:
+    case __NR_sysinfo:
+    case __NR_uname:
+#endif  // defined(__arm__)
+      return ErrorCode(ErrorCode::ERR_ALLOWED);
+    case __NR_open:
+    case __NR_openat:
+      return sandbox->Trap(GpuOpenSIGSYS_Handler, broker_process);
+    default:
+#if defined(__arm__)
+      if (IsSystemVSharedMemory(sysno))
+        return ErrorCode(EACCES);
+
+      if (IsAdvancedScheduler(sysno))

[jln (very slow on Chromium), 2013/04/23 02:22:47]

This one should compile on all architectures (I'm pretty adamant  about avoiding
#ifdef like the plague).

Added benefit: you can put this one and the next one in a single if.

[Jorge Lucangeli Obes, 2013/04/23 17:20:41]

Done.

+        return ErrorCode(ErrorCode::ERR_ALLOWED);
+#endif
+      if (IsEventFd(sysno))
+        return ErrorCode(ErrorCode::ERR_ALLOWED);
+
+      // Default on the baseline policy.
+      return BaselinePolicy(sandbox, sysno);
+  }
+}
+
+// A GPU broker policy is the same as a GPU policy with open and
+// openat allowed.
+ErrorCode ArmMaliGpuBrokerProcessPolicy(Sandbox *sandbox,
+                                        int sysno, void *aux) {
+  // "aux" would typically be NULL, when called from
+  // "EnableGpuBrokerPolicyCallBack"
+  switch(sysno) {
+    case __NR_open:
+    case __NR_openat:
+      return ErrorCode(ErrorCode::ERR_ALLOWED);
+    default:
+      return ArmMaliGpuProcessPolicy(sandbox, sysno, aux);
+  }
+}
+
 // Allow clone for threads, crash if anything else is attempted.
 // Don't restrict on ASAN.
 ErrorCode RestrictCloneToThreads(Sandbox *sandbox) {
@@ -1453,28 +1504,69 @@ ErrorCode AllowAllPolicy(Sandbox *, int sysno, void *) {
   }
 }
 
-bool EnableGpuBrokerPolicyCallBack() {
+bool EnableGpuBrokerPolicyCallback() {
   StartSandboxWithPolicy(GpuBrokerProcessPolicy, NULL);
   return true;
 }
 
+bool EnableArmMaliGpuBrokerPolicyCallback() {
+  StartSandboxWithPolicy(ArmMaliGpuBrokerProcessPolicy, NULL);
+  return true;
+}
+
+void AddArmMaliGpuWhitelist(std::vector<std::string>* read_whitelist,
+                            std::vector<std::string>* write_whitelist) {
+  // On ARM we're enabling the sandbox before the X connection is made,
+  // 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";
+
+  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);
+
+  write_whitelist->push_back(kMali0Path);
+  write_whitelist->push_back(kDevMfcDecPath);
+  write_whitelist->push_back(kDevGsc1Path);
+}
+
 // Start a broker process to handle open() inside the sandbox.
-void InitGpuBrokerProcess(BrokerProcess** broker_process) {
+void InitGpuBrokerProcess(Sandbox::EvaluateSyscall gpu_policy,
+                          BrokerProcess** broker_process) {
   static const char kDriRcPath[] = "/etc/drirc";
   static const char kDriCard0Path[] = "/dev/dri/card0";
 
   CHECK(broker_process);
   CHECK(*broker_process == NULL);
 
+  bool (*sandbox_callback)(void) = EnableGpuBrokerPolicyCallback;

[jln (very slow on Chromium), 2013/04/23 02:22:47]

It's confusing, I'd just initialize it to NULL, and there should be a clear
switch-like if statement that sets it to right value depending on the policy.

[Jorge Lucangeli Obes, 2013/04/23 17:20:41]

Done.

+
   std::vector<std::string> read_whitelist;
   read_whitelist.push_back(kDriCard0Path);

[jln (very slow on Chromium), 2013/04/23 02:22:47]

Are these needed for Mali ? If not, put them clearly in the if gpu_policy ==
GpuProcessPolicy (see comment below).

[Jorge Lucangeli Obes, 2013/04/23 17:20:41]

Yep, they're needed for both.

   read_whitelist.push_back(kDriRcPath);
+
   std::vector<std::string> write_whitelist;
   write_whitelist.push_back(kDriCard0Path);
 
+  if (IsArchitectureArm() && gpu_policy == ArmMaliGpuProcessPolicy) {

[jln (very slow on Chromium), 2013/04/23 02:22:47]

I think the right choice is if(gpu_policy == XX) { CHECK(IsArchitecture..());
... }

I would put an else if (gpu_policy == GpuProcessPolicy) right after (or maybe
better: before), so that it's clear and in one place how things diverge.

[Jorge Lucangeli Obes, 2013/04/23 17:20:41]

Done.

+    AddArmMaliGpuWhitelist(&read_whitelist, &write_whitelist);
+    sandbox_callback = EnableArmMaliGpuBrokerPolicyCallback;
+  }
+
   *broker_process = new BrokerProcess(read_whitelist, write_whitelist);
-  // Initialize the broker process and give it a sandbox call back.
-  CHECK((*broker_process)->Init(EnableGpuBrokerPolicyCallBack));
+  // Initialize the broker process and give it a sandbox callback.
+  CHECK((*broker_process)->Init(sandbox_callback));
 }
 
 // Warms up/preloads resources needed by the policies.
@@ -1482,10 +1574,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(policy, broker_process);
 
+    if (IsArchitectureX86_64() || IsArchitectureI386()) {
       // Accelerated video decode dlopen()'s a shared object
       // inside the sandbox, so preload it now.
       if (IsAcceleratedVideoDecodeEnabled()) {
@@ -1500,6 +1592,9 @@ void WarmupPolicy(Sandbox::EvaluateSyscall policy,
         dlopen(I965DrvVideoPath, RTLD_NOW|RTLD_GLOBAL|RTLD_NODELETE);
       }
     }
+  } else if (policy == ArmMaliGpuProcessPolicy) {
+    // Create a new broker process.
+    InitGpuBrokerProcess(policy, broker_process);
   }
 }
 
@@ -1508,11 +1603,11 @@ Sandbox::EvaluateSyscall GetProcessSyscallPolicy(
     const std::string& process_type) {
   if (process_type == switches::kGpuProcess) {
     // On Chrome OS, --enable-gpu-sandbox enables the more restrictive policy.
-    // However, we don't yet enable the more restrictive GPU process policy
-    // on ARM.
-    if (IsArchitectureArm() ||
-        (IsChromeOS() && !command_line.HasSwitch(switches::kEnableGpuSandbox)))
+    if (IsChromeOS() && !command_line.HasSwitch(switches::kEnableGpuSandbox))
       return BlacklistDebugAndNumaPolicy;
+    // On Chrome OS ARM, we need a specific GPU process policy.
+    else if (IsChromeOS() && IsArchitectureArm())
+      return ArmMaliGpuProcessPolicy;
     else
       return GpuProcessPolicy;
   }