From 632aad0141fe0008fa9babba4f1f514222fa2cda Mon Sep 17 00:00:00 2001
From: Matthew Denton <mpdenton@chromium.org>
Date: Mon, 01 Aug 2022 21:45:28 +0000
Subject: [PATCH] [Linux sandbox] cleanup TrapRegistry's "atomics"

TrapRegistry uses some hacky asm statements as compiler memory barriers
to prevent a signal handler from accessing a deleted array (in the case
that the store of the pointer to the new array is reordered after the
deletion of the old array and the signal handler grabs a pointer to the
old array after it's deleted).

We have std::atomic_signal_fence for this now, so this uses it.

This also changes the |trap_array_| pointer back to a raw pointer from
a raw_ptr. Usage of raw_ptr might be awkward as it is also accessed in
a signal handler, and in fact |trap_array_| is an owning pointer
anyway so raw_ptr is unnecessary.

This came up in https://crrev.com/c/3789266 in which the use of raw_ptr
with the hacky compiler barriers was not supported by GCC.

SMALL ADDITION: This also removes raw_ptr from the arch_sigsys struct; it was a raw pointer to a code instruction and likely would not have worked. It is also never dereferenced (only its value is used).

NOTE 1: In technicality, all non-local variables accessed by the signal
handler must be either lock-free std::atomics or volatile sig_atomic_t.
None of Chrome's code does this and in fact, glibc just typedefs
sig_atomic_t to int. The std::atomic_signal_fence is enough on any
architecture.

NOTE 2: This race condition is unlikely to ever happen even without
compiler barriers. The only time we might be modifying the
|trap_array_| and also accessing it from a signal handler, we must
have already applied a seccomp sandbox that uses traps, and now be
applying another one that uses traps. And to replace the deleted object,
the second sandbox must be getting applied in a multithreaded
environment, otherwise there would be no allocations after the free.

Bug: 819294

Change-Id: I9f1cd417446dd863805a303e9b111bc862cb9ae2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3788911
Commit-Queue: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1030277}
---

diff --git a/sandbox/linux/seccomp-bpf/trap.cc b/sandbox/linux/seccomp-bpf/trap.cc
index cb71a9b..b0c0257 100644
--- a/sandbox/linux/seccomp-bpf/trap.cc
+++ b/sandbox/linux/seccomp-bpf/trap.cc
@@ -12,12 +12,13 @@
 #include <sys/syscall.h>
 
 #include <algorithm>
+#include <atomic>
 #include <limits>
 #include <tuple>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
-#include "base/memory/raw_ptr.h"
+#include "base/memory/raw_ptr_exclusion.h"
 #include "build/build_config.h"
 #include "sandbox/linux/bpf_dsl/seccomp_macros.h"
 #include "sandbox/linux/seccomp-bpf/die.h"
@@ -29,7 +30,9 @@
 namespace {
 
 struct arch_sigsys {
-  raw_ptr<void> ip;
+  // This is not raw_ptr because it is a pointer to a code address given to us
+  // by the kernel.
+  RAW_PTR_EXCLUSION void* ip;
   int nr;
   unsigned int arch;
 };
@@ -77,11 +80,7 @@
 
 namespace sandbox {
 
-Trap::Trap()
-    : trap_array_(nullptr),
-      trap_array_size_(0),
-      trap_array_capacity_(0),
-      has_unsafe_traps_(false) {
+Trap::Trap() {
   // Set new SIGSYS handler
   struct sigaction sa = {};
   // In some toolchain, sa_sigaction is not declared in struct sigaction.
@@ -239,7 +238,7 @@
     struct arch_seccomp_data data = {
         static_cast<int>(SECCOMP_SYSCALL(ctx)),
         SECCOMP_ARCH,
-        reinterpret_cast<uint64_t>(sigsys.ip.get()),
+        reinterpret_cast<uint64_t>(sigsys.ip),
         {static_cast<uint64_t>(SECCOMP_PARM1(ctx)),
          static_cast<uint64_t>(SECCOMP_PARM2(ctx)),
          static_cast<uint64_t>(SECCOMP_PARM3(ctx)),
@@ -333,24 +332,11 @@
     TrapKey* new_trap_array = new TrapKey[trap_array_capacity_];
     std::copy_n(old_trap_array, trap_array_size_, new_trap_array);
 
-    // Language specs are unclear on whether the compiler is allowed to move
-    // the "delete[]" above our preceding assignments and/or memory moves,
-    // iff the compiler believes that "delete[]" doesn't have any other
-    // global side-effects.
-    // We insert optimization barriers to prevent this from happening.
-    // The first barrier is probably not needed, but better be explicit in
-    // what we want to tell the compiler.
-    // The clang developer mailing list couldn't answer whether this is a
-    // legitimate worry; but they at least thought that the barrier is
-    // sufficient to prevent the (so far hypothetical) problem of re-ordering
-    // of instructions by the compiler.
-    //
-    // TODO(mdempsky): Try to clean this up using base/atomicops or C++11
-    // atomics; see crbug.com/414363.
-    asm volatile("" : "=r"(new_trap_array) : "0"(new_trap_array) : "memory");
     trap_array_ = new_trap_array;
-    asm volatile("" : "=r"(trap_array_) : "0"(trap_array_) : "memory");
-
+    // Prevent the compiler from moving delete[] before the store of the
+    // |new_trap_array|, otherwise a concurrent SIGSYS may see a |trap_array_|
+    // that still points to |old_trap_array| after it has been deleted.
+    std::atomic_signal_fence(std::memory_order_release);
     delete[] old_trap_array;
   }
 
diff --git a/sandbox/linux/seccomp-bpf/trap.h b/sandbox/linux/seccomp-bpf/trap.h
index cc17d26..37d2029 100644
--- a/sandbox/linux/seccomp-bpf/trap.h
+++ b/sandbox/linux/seccomp-bpf/trap.h
@@ -10,7 +10,7 @@
 
 #include <map>
 
-#include "base/memory/raw_ptr.h"
+#include "base/memory/raw_ptr_exclusion.h"
 #include "sandbox/linux/bpf_dsl/trap_registry.h"
 #include "sandbox/linux/system_headers/linux_signal.h"
 #include "sandbox/sandbox_export.h"
@@ -75,11 +75,15 @@
   // events.
   static Trap* global_trap_;
 
-  TrapIds trap_ids_;            // Maps from TrapKeys to numeric ids
-  raw_ptr<TrapKey> trap_array_;  // Array of TrapKeys indexed by ids
-  size_t trap_array_size_;      // Currently used size of array
-  size_t trap_array_capacity_;  // Currently allocated capacity of array
-  bool has_unsafe_traps_;       // Whether unsafe traps have been enabled
+  TrapIds trap_ids_;  // Maps from TrapKeys to numeric ids
+  // Array of TrapKeys indexed by ids.
+  //
+  // This is not a raw_ptr as it is an owning pointer anyway, and is meant to be
+  // used between normal code and signal handlers.
+  RAW_PTR_EXCLUSION TrapKey* trap_array_ = nullptr;
+  size_t trap_array_size_ = 0;      // Currently used size of array
+  size_t trap_array_capacity_ = 0;  // Currently allocated capacity of array
+  bool has_unsafe_traps_ = false;   // Whether unsafe traps have been enabled
 };
 
 }  // namespace sandbox