From 632aad0141fe0008fa9babba4f1f514222fa2cda Mon Sep 17 00:00:00 2001 From: Matthew Denton 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 Reviewed-by: Tom Sepez 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 #include +#include #include #include #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 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(SECCOMP_SYSCALL(ctx)), SECCOMP_ARCH, - reinterpret_cast(sigsys.ip.get()), + reinterpret_cast(sigsys.ip), {static_cast(SECCOMP_PARM1(ctx)), static_cast(SECCOMP_PARM2(ctx)), static_cast(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 -#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 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