Consolidate syscall broker policies and signal handling.

No functional change intended, just duplicate code reduction.
Adds the common signal handler to broker_process.cc, since all
it does is call back into BrokerProcess methods.

Change-Id: Ie8f1604888465fb9996c34a7f0f42811b8cddb30
Reviewed-on: https://chromium-review.googlesource.com/773109
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517454}

sandbox/linux/syscall_broker/DEPS

@@ -1,3 +1,4 @@
 include_rules = [
   "+sandbox/linux/system_headers",
+  "+sandbox/linux/bpf_dsl",
 ]

sandbox/linux/syscall_broker/broker_process.cc

@@ -114,6 +114,45 @@ int BrokerProcess::Open(const char* pathname, int flags) const {
   return broker_client_->Open(pathname, flags);
 }
 
-}  // namespace syscall_broker
+// static
+intptr_t BrokerProcess::SIGSYS_Handler(const sandbox::arch_seccomp_data& args,
+                                       void* aux_broker_process) {
+  RAW_CHECK(aux_broker_process);
+  auto* broker_process = static_cast<BrokerProcess*>(aux_broker_process);
+  switch (args.nr) {
+#if !defined(__aarch64__)
+    case __NR_access:
+      return broker_process->Access(reinterpret_cast<const char*>(args.args[0]),
+                                    static_cast<int>(args.args[1]));
+    case __NR_open:
+#if defined(MEMORY_SANITIZER)
+      // http://crbug.com/372840
+      __msan_unpoison_string(reinterpret_cast<const char*>(args.args[0]));
+#endif
+      return broker_process->Open(reinterpret_cast<const char*>(args.args[0]),
+                                  static_cast<int>(args.args[1]));
+#endif  // !defined(__aarch64__)
+    case __NR_faccessat:
+      if (static_cast<int>(args.args[0]) == AT_FDCWD) {
+        return broker_process->Access(
+            reinterpret_cast<const char*>(args.args[1]),
+            static_cast<int>(args.args[2]));
+      } else {
+        return -EPERM;
+      }
+    case __NR_openat:
+      // Allow using openat() as open().
+      if (static_cast<int>(args.args[0]) == AT_FDCWD) {
+        return broker_process->Open(reinterpret_cast<const char*>(args.args[1]),
+                                    static_cast<int>(args.args[2]));
+      } else {
+        return -EPERM;
+      }
+    default:
+      RAW_CHECK(false);
+      return -ENOSYS;
+  }
+}
 
+}  // namespace syscall_broker
 }  // namespace sandbox.

sandbox/linux/syscall_broker/broker_process.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/pickle.h"
 #include "base/process/process.h"
+#include "sandbox/linux/bpf_dsl/trap_registry.h"
 #include "sandbox/linux/syscall_broker/broker_policy.h"
 #include "sandbox/sandbox_export.h"
 
@@ -70,6 +71,11 @@ class SANDBOX_EXPORT BrokerProcess {
 
   int broker_pid() const { return broker_pid_; }
 
+  // Handler to be used with a bpf_dsl Trap() function to forward system calls
+  // to the methods above.
+  static intptr_t SIGSYS_Handler(const arch_seccomp_data& args,
+                                 void* aux_broker_process);
+
  private:
   friend class BrokerProcessTestHelper;
 

services/service_manager/sandbox/BUILD.gn

@@ -27,6 +27,8 @@ component("sandbox") {
     sources += [
       "linux/bpf_base_policy_linux.cc",
       "linux/bpf_base_policy_linux.h",
+      "linux/bpf_broker_policy_linux.cc",
+      "linux/bpf_broker_policy_linux.h",
       "linux/bpf_cdm_policy_linux.cc",
       "linux/bpf_cdm_policy_linux.h",
       "linux/bpf_cros_amd_gpu_policy_linux.cc",

services/service_manager/sandbox/linux/bpf_base_policy_linux.cc

@@ -35,10 +35,6 @@ ResultExpr BPFBasePolicy::InvalidSyscall() const {
   return baseline_policy_->InvalidSyscall();
 }
 
-std::unique_ptr<BPFBasePolicy> BPFBasePolicy::GetBrokerSandboxPolicy() {
-  return nullptr;
-}
-
 int BPFBasePolicy::GetFSDeniedErrno() {
   return kFSDeniedErrno;
 }

services/service_manager/sandbox/linux/bpf_base_policy_linux.h

@@ -30,11 +30,6 @@ class SERVICE_MANAGER_SANDBOX_EXPORT BPFBasePolicy
       int system_call_number) const override;
   sandbox::bpf_dsl::ResultExpr InvalidSyscall() const override;
 
-  // If the syscall handler for this policy requires a broker process,
-  // return the corresponding (less restrictive) sandbox policy to apply
-  // to the broker. If a broker is not required, nullptr is returned.
-  virtual std::unique_ptr<BPFBasePolicy> GetBrokerSandboxPolicy();
-
   // Get the errno(3) to return for filesystem errors.
   static int GetFSDeniedErrno();
 

services/service_manager/sandbox/linux/bpf_broker_policy_linux.cc

@@ -0,0 +1,38 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/service_manager/sandbox/linux/bpf_broker_policy_linux.h"
+
+#include "sandbox/linux/bpf_dsl/bpf_dsl.h"
+#include "sandbox/linux/system_headers/linux_syscalls.h"
+
+using sandbox::bpf_dsl::Allow;
+using sandbox::bpf_dsl::ResultExpr;
+
+namespace service_manager {
+
+BrokerProcessPolicy::BrokerProcessPolicy() {}
+
+BrokerProcessPolicy::~BrokerProcessPolicy() {}
+
+ResultExpr BrokerProcessPolicy::EvaluateSyscall(int sysno) const {
+  switch (sysno) {
+#if !defined(__aarch64__)
+    case __NR_access:
+    case __NR_open:
+#endif  // !defined(__aarch64__)
+    case __NR_faccessat:
+    case __NR_openat:
+#if !defined(OS_CHROMEOS) && !defined(__aarch64__)
+    // The broker process needs to able to unlink the temporary
+    // files that it may create.
+    case __NR_unlink:
+#endif
+      return Allow();
+    default:
+      return GpuProcessPolicy::EvaluateSyscall(sysno);
+  }
+}
+
+}  // namespace service_manager

services/service_manager/sandbox/linux/bpf_broker_policy_linux.h

@@ -0,0 +1,33 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_BROKER_POLICY_LINUX_H_
+#define SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_BROKER_POLICY_LINUX_H_
+
+#include "sandbox/linux/bpf_dsl/bpf_dsl.h"
+#include "services/service_manager/sandbox/export.h"
+#include "services/service_manager/sandbox/linux/bpf_gpu_policy_linux.h"
+
+namespace service_manager {
+
+// A broker policy is one for a privileged syscall broker that allows
+// access, open, openat, and (in the non-Chrome OS case) unlink.
+// TODO(tsepez): probably should not inherit from any other process policy,
+// since that may include random syscalls that this does not need.
+class SERVICE_MANAGER_SANDBOX_EXPORT BrokerProcessPolicy
+    : public GpuProcessPolicy {
+ public:
+  BrokerProcessPolicy();
+  ~BrokerProcessPolicy() override;
+
+  sandbox::bpf_dsl::ResultExpr EvaluateSyscall(
+      int system_call_number) const override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BrokerProcessPolicy);
+};
+
+}  // namespace service_manager
+
+#endif  // SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_BROKER_POLICY_LINUX_H_

services/service_manager/sandbox/linux/bpf_cros_amd_gpu_policy_linux.cc

@@ -4,23 +4,15 @@
 
 #include "services/service_manager/sandbox/linux/bpf_cros_amd_gpu_policy_linux.h"
 
-#include <dlfcn.h>
-#include <errno.h>
 #include <fcntl.h>
 #include <sys/socket.h>
-#include <sys/stat.h>
-#include <sys/types.h>
 
 // Some arch's (arm64 for instance) unistd.h don't pull in symbols used here
 // unless these are defined.
 #define __ARCH_WANT_SYSCALL_NO_AT
 #define __ARCH_WANT_SYSCALL_DEPRECATED
 #include <unistd.h>
 
-#include <memory>
-#include <string>
-#include <vector>
-
 #include "base/logging.h"
 #include "sandbox/linux/bpf_dsl/bpf_dsl.h"
 #include "sandbox/linux/system_headers/linux_syscalls.h"
@@ -63,34 +55,4 @@ ResultExpr CrosAmdGpuProcessPolicy::EvaluateSyscall(int sysno) const {
   }
 }
 
-std::unique_ptr<BPFBasePolicy>
-CrosAmdGpuProcessPolicy::GetBrokerSandboxPolicy() {
-  return std::make_unique<CrosAmdGpuBrokerProcessPolicy>();
-}
-
-CrosAmdGpuBrokerProcessPolicy::CrosAmdGpuBrokerProcessPolicy() {}
-
-CrosAmdGpuBrokerProcessPolicy::~CrosAmdGpuBrokerProcessPolicy() {}
-
-// A GPU broker policy is the same as a GPU policy with access, open,
-// openat and in the non-Chrome OS case unlink allowed.
-ResultExpr CrosAmdGpuBrokerProcessPolicy::EvaluateSyscall(int sysno) const {
-  switch (sysno) {
-    case __NR_faccessat:
-    case __NR_openat:
-#if !defined(__aarch64__)
-    case __NR_access:
-    case __NR_open:
-#if !defined(OS_CHROMEOS)
-    // The broker process needs to able to unlink the temporary
-    // files that it may create. This is used by DRI3.
-    case __NR_unlink:
-#endif  // !defined(OS_CHROMEOS)
-#endif  // !define(__aarch64__)
-      return Allow();
-    default:
-      return CrosAmdGpuProcessPolicy::EvaluateSyscall(sysno);
-  }
-}
-
 }  // namespace service_manager

services/service_manager/sandbox/linux/bpf_cros_amd_gpu_policy_linux.h

@@ -5,9 +5,7 @@
 #ifndef SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_CROS_AMD_GPU_POLICY_LINUX_H_
 #define SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_CROS_AMD_GPU_POLICY_LINUX_H_
 
-#include <memory>
-
-#include "base/macros.h"
+#include "sandbox/linux/bpf_dsl/bpf_dsl.h"
 #include "services/service_manager/sandbox/export.h"
 #include "services/service_manager/sandbox/linux/bpf_gpu_policy_linux.h"
 
@@ -23,25 +21,10 @@ class SERVICE_MANAGER_SANDBOX_EXPORT CrosAmdGpuProcessPolicy
   sandbox::bpf_dsl::ResultExpr EvaluateSyscall(
       int system_call_number) const override;
 
-  std::unique_ptr<BPFBasePolicy> GetBrokerSandboxPolicy() override;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(CrosAmdGpuProcessPolicy);
 };
 
-class SERVICE_MANAGER_SANDBOX_EXPORT CrosAmdGpuBrokerProcessPolicy
-    : public CrosAmdGpuProcessPolicy {
- public:
-  CrosAmdGpuBrokerProcessPolicy();
-  ~CrosAmdGpuBrokerProcessPolicy() override;
-
-  sandbox::bpf_dsl::ResultExpr EvaluateSyscall(
-      int system_call_number) const override;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(CrosAmdGpuBrokerProcessPolicy);
-};
-
 }  // namespace service_manager
 
 #endif  // SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_CROS_AMD_GPU_POLICY_LINUX_H_

services/service_manager/sandbox/linux/bpf_cros_arm_gpu_policy_linux.cc

@@ -4,19 +4,11 @@
 
 #include "services/service_manager/sandbox/linux/bpf_cros_arm_gpu_policy_linux.h"
 
-#include <dlfcn.h>
-#include <errno.h>
 #include <fcntl.h>
 #include <sys/socket.h>
-#include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
 
-#include <memory>
-#include <string>
-#include <vector>
-
-#include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ -32,7 +24,6 @@ using sandbox::bpf_dsl::Arg;
 using sandbox::bpf_dsl::Error;
 using sandbox::bpf_dsl::If;
 using sandbox::bpf_dsl::ResultExpr;
-using sandbox::SyscallSets;
 
 namespace service_manager {
 
@@ -74,30 +65,4 @@ ResultExpr CrosArmGpuProcessPolicy::EvaluateSyscall(int sysno) const {
   }
 }
 
-std::unique_ptr<BPFBasePolicy>
-CrosArmGpuProcessPolicy::GetBrokerSandboxPolicy() {
-  return std::make_unique<CrosArmGpuBrokerProcessPolicy>();
-}
-
-CrosArmGpuBrokerProcessPolicy::CrosArmGpuBrokerProcessPolicy()
-    : CrosArmGpuProcessPolicy(false) {}
-
-CrosArmGpuBrokerProcessPolicy::~CrosArmGpuBrokerProcessPolicy() {}
-
-// A GPU broker policy is the same as a GPU policy with open and
-// openat allowed.
-ResultExpr CrosArmGpuBrokerProcessPolicy::EvaluateSyscall(int sysno) const {
-  switch (sysno) {
-#if !defined(__aarch64__)
-    case __NR_access:
-    case __NR_open:
-#endif  // !defined(__aarch64__)
-    case __NR_faccessat:
-    case __NR_openat:
-      return Allow();
-    default:
-      return CrosArmGpuProcessPolicy::EvaluateSyscall(sysno);
-  }
-}
-
 }  // namespace service_manager

services/service_manager/sandbox/linux/bpf_cros_arm_gpu_policy_linux.h

@@ -5,7 +5,7 @@
 #ifndef SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_CROS_ARM_GPU_POLICY_LINUX_H_
 #define SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_CROS_ARM_GPU_POLICY_LINUX_H_
 
-#include "base/macros.h"
+#include "sandbox/linux/bpf_dsl/bpf_dsl.h"
 #include "services/service_manager/sandbox/export.h"
 #include "services/service_manager/sandbox/linux/bpf_gpu_policy_linux.h"
 
@@ -21,28 +21,13 @@ class SERVICE_MANAGER_SANDBOX_EXPORT CrosArmGpuProcessPolicy
   sandbox::bpf_dsl::ResultExpr EvaluateSyscall(
       int system_call_number) const override;
 
-  std::unique_ptr<BPFBasePolicy> GetBrokerSandboxPolicy() override;
-
  private:
 #if defined(__arm__) || defined(__aarch64__)
   const bool allow_shmat_;  // Allow shmat(2).
 #endif
   DISALLOW_COPY_AND_ASSIGN(CrosArmGpuProcessPolicy);
 };
 
-class SERVICE_MANAGER_SANDBOX_EXPORT CrosArmGpuBrokerProcessPolicy
-    : public CrosArmGpuProcessPolicy {
- public:
-  CrosArmGpuBrokerProcessPolicy();
-  ~CrosArmGpuBrokerProcessPolicy() override;
-
-  sandbox::bpf_dsl::ResultExpr EvaluateSyscall(
-      int system_call_number) const override;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(CrosArmGpuBrokerProcessPolicy);
-};
-
 }  // namespace service_manager
 
 #endif  // SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_CROS_ARM_GPU_POLICY_LINUX_H_