You can not select more than 25 topics Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
chromium/chromium-105-Trap-raw_ptr.p...

162 lines
6.6 KiB

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