Skip to content

Commit

Permalink
Revert of Reland: Introduce sys_sigprocmask and sys_sigaction. (patch…
Browse files Browse the repository at this point in the history
…set #5 id:160001 of https://codereview.c… (patchset #4 id:80001 of https://codereview.chromium.org/1092153005/)

Reason for revert:
Causing Linux Tests to fail:
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29%281%29/builds/41533

sandbox_linux_unittests Trap.SigSysAction failed 14
failures:
Syscall.SyntheticSixArgs
SandboxBPF.PthreadEquality
SandboxBPF.Pread64
SandboxBPF.UnsafeTrapWithErrno
SandboxBPF.ForwardSyscall
SandboxBPF.BasicBlacklistWithSigsys
SandboxBPF.SigBus
Trap.SigSysAction
SandboxBPF.VerboseAPITesting
ParameterRestrictions.sched_getparam_allowed
SandboxBPF.UnsafeTrapWithCond

[ RUN      ] SandboxBPF.VerboseAPITesting
../../sandbox/linux/tests/unit_tests.cc:234: Failure
Value of: subprocess_terminated_normally
  Actual: false
Expected: true

Original issue's description:
> Reland: Introduce sys_sigprocmask and sys_sigaction. (patchset #5 id:160001 of https://codereview.chromium.org/1077143002/)
>
> This CL is relanding the CL https://codereview.chromium.org/1077143002/,
> which was reverted due to false alarm of the memory sanitizer.
> Because the memory sanitizer does not know about direct kernel syscall
> does, so it warns the arguments of the signal handler, unexpectedly.
> This CL adds the msan unpoisoning to the arguemtns, so that the memory
> sanitizer should understand it.
>
> TEST=Ran sandbox_linux_unittests with msan=1. Ran bots.
> BUG=358465
> CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_chromium_trusty32_rel,linux_arm
>
> Committed: https://crrev.com/c1c944e1ee0b100c9717084b50bdd697c6b308d2
> Cr-Commit-Position: refs/heads/master@{#326270}

TBR=mdempsky@chromium.org,hidehiko@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=358465,479705

Review URL: https://codereview.chromium.org/1102543003

Cr-Commit-Position: refs/heads/master@{#326278}
  • Loading branch information
magjed authored and Commit bot committed Apr 22, 2015
1 parent 492f2be commit 958459a
Show file tree
Hide file tree
Showing 21 changed files with 112 additions and 356 deletions.
9 changes: 4 additions & 5 deletions sandbox/linux/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -288,16 +288,15 @@ component("sandbox_services") {

source_set("sandbox_services_headers") {
sources = [
"system_headers/android_arm64_ucontext.h",
"system_headers/android_arm_ucontext.h",
"system_headers/android_i386_ucontext.h",
"system_headers/android_ucontext.h",
"system_headers/arm64_linux_syscalls.h",
"system_headers/arm64_linux_ucontext.h",
"system_headers/arm_linux_syscalls.h",
"system_headers/arm_linux_ucontext.h",
"system_headers/i386_linux_ucontext.h",
"system_headers/linux_futex.h",
"system_headers/linux_seccomp.h",
"system_headers/linux_signal.h",
"system_headers/linux_syscalls.h",
"system_headers/linux_ucontext.h",
"system_headers/x86_32_linux_syscalls.h",
"system_headers/x86_64_linux_syscalls.h",
]
Expand Down
10 changes: 5 additions & 5 deletions sandbox/linux/sandbox_linux.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -278,18 +278,18 @@
{ 'target_name': 'sandbox_services_headers',
'type': 'none',
'sources': [
'system_headers/android_arm64_ucontext.h',
'system_headers/android_arm_ucontext.h',
'system_headers/android_i386_ucontext.h',
'system_headers/android_mips_ucontext.h',
'system_headers/android_ucontext.h',
'system_headers/arm64_linux_syscalls.h',
'system_headers/arm64_linux_ucontext.h',
'system_headers/arm_linux_syscalls.h',
'system_headers/arm_linux_ucontext.h',
'system_headers/capability.h',
'system_headers/i386_linux_ucontext.h',
'system_headers/linux_futex.h',
'system_headers/linux_seccomp.h',
'system_headers/linux_syscalls.h',
'system_headers/linux_ucontext.h',
'system_headers/mips_linux_syscalls.h',
'system_headers/mips_linux_ucontext.h',
'system_headers/x86_32_linux_syscalls.h',
'system_headers/x86_64_linux_syscalls.h',
],
Expand Down
1 change: 0 additions & 1 deletion sandbox/linux/sandbox_linux_test_sources.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@
'seccomp-bpf/errorcode_unittest.cc',
'seccomp-bpf/sandbox_bpf_unittest.cc',
'seccomp-bpf/syscall_unittest.cc',
'seccomp-bpf/trap_unittest.cc',
],
}],
[ 'compile_credentials==1', {
Expand Down
7 changes: 1 addition & 6 deletions sandbox/linux/seccomp-bpf/die.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
#include "base/logging.h"
#include "base/posix/eintr_wrapper.h"
#include "sandbox/linux/seccomp-bpf/syscall.h"
#include "sandbox/linux/services/syscall_wrappers.h"
#include "sandbox/linux/system_headers/linux_signal.h"

namespace sandbox {

Expand All @@ -34,10 +32,7 @@ void Die::ExitGroup() {
// to a defined state; but we have not way to verify whether we actually
// succeeded in doing so. Nonetheless, triggering a fatal signal could help
// us terminate.
struct sigaction sa = {};
sa.sa_handler = LINUX_SIG_DFL;
sa.sa_flags = LINUX_SA_RESTART;
sys_sigaction(LINUX_SIGSEGV, &sa, nullptr);
signal(SIGSEGV, SIG_DFL);
Syscall::Call(__NR_prctl, PR_SET_DUMPABLE, (void*)0, (void*)0, (void*)0);
if (*(volatile char*)0) {
}
Expand Down
6 changes: 5 additions & 1 deletion sandbox/linux/seccomp-bpf/syscall.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@
#include <stdint.h>

#include "base/macros.h"
#include "sandbox/linux/system_headers/linux_signal.h"
#include "sandbox/sandbox_export.h"

// Android's signal.h doesn't define ucontext etc.
#if defined(OS_ANDROID)
#include "sandbox/linux/system_headers/android_ucontext.h"
#endif

namespace sandbox {

// This purely static class can be used to perform system calls with some
Expand Down
42 changes: 17 additions & 25 deletions sandbox/linux/seccomp-bpf/trap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,17 @@
#include <algorithm>
#include <limits>

#include "base/compiler_specific.h"
#include "base/logging.h"
#include "build/build_config.h"
#include "sandbox/linux/bpf_dsl/seccomp_macros.h"
#include "sandbox/linux/seccomp-bpf/die.h"
#include "sandbox/linux/seccomp-bpf/syscall.h"
#include "sandbox/linux/services/syscall_wrappers.h"
#include "sandbox/linux/system_headers/linux_seccomp.h"
#include "sandbox/linux/system_headers/linux_signal.h"

// Android's signal.h doesn't define ucontext etc.
#if defined(OS_ANDROID)
#include "sandbox/linux/system_headers/android_ucontext.h"
#endif

namespace {

Expand Down Expand Up @@ -51,13 +53,13 @@ const char kSandboxDebuggingEnv[] = "CHROME_SANDBOX_DEBUGGING";
// possibly even worse.
bool GetIsInSigHandler(const ucontext_t* ctx) {
// Note: on Android, sigismember does not take a pointer to const.
return sigismember(const_cast<sigset_t*>(&ctx->uc_sigmask), LINUX_SIGBUS);
return sigismember(const_cast<sigset_t*>(&ctx->uc_sigmask), SIGBUS);
}

void SetIsInSigHandler() {
sigset_t mask;
if (sigemptyset(&mask) || sigaddset(&mask, LINUX_SIGBUS) ||
sandbox::sys_sigprocmask(LINUX_SIG_BLOCK, &mask, NULL)) {
if (sigemptyset(&mask) || sigaddset(&mask, SIGBUS) ||
sigprocmask(SIG_BLOCK, &mask, NULL)) {
SANDBOX_DIE("Failed to block SIGBUS");
}
}
Expand All @@ -80,13 +82,10 @@ Trap::Trap()
has_unsafe_traps_(false) {
// Set new SIGSYS handler
struct sigaction sa = {};
// In some toolchain, sa_sigaction is not declared in struct sigaction.
// So, here cast the pointer to the sa_handler's type. This works because
// |sa_handler| and |sa_sigaction| shares the same memory.
sa.sa_handler = reinterpret_cast<void (*)(int)>(SigSysAction);
sa.sa_flags = LINUX_SA_SIGINFO | LINUX_SA_NODEFER;
struct sigaction old_sa = {};
if (sys_sigaction(LINUX_SIGSYS, &sa, &old_sa) < 0) {
sa.sa_sigaction = SigSysAction;
sa.sa_flags = SA_SIGINFO | SA_NODEFER;
struct sigaction old_sa;
if (sigaction(SIGSYS, &sa, &old_sa) < 0) {
SANDBOX_DIE("Failed to configure SIGSYS handler");
}

Expand All @@ -100,8 +99,8 @@ Trap::Trap()

// Unmask SIGSYS
sigset_t mask;
if (sigemptyset(&mask) || sigaddset(&mask, LINUX_SIGSYS) ||
sys_sigprocmask(LINUX_SIG_UNBLOCK, &mask, NULL)) {
if (sigemptyset(&mask) || sigaddset(&mask, SIGSYS) ||
sigprocmask(SIG_UNBLOCK, &mask, NULL)) {
SANDBOX_DIE("Failed to configure SIGSYS handler");
}
}
Expand All @@ -121,14 +120,7 @@ bpf_dsl::TrapRegistry* Trap::Registry() {
return global_trap_;
}

void Trap::SigSysAction(int nr, LinuxSigInfo* info, void* void_context) {
if (info) {
MSAN_UNPOISON(info, sizeof(*info));
}
if (void_context) {
MSAN_UNPOISON(void_context, sizeof(ucontext_t));
}

void Trap::SigSysAction(int nr, siginfo_t* info, void* void_context) {
if (!global_trap_) {
RAW_SANDBOX_DIE(
"This can't happen. Found no global singleton instance "
Expand All @@ -137,15 +129,15 @@ void Trap::SigSysAction(int nr, LinuxSigInfo* info, void* void_context) {
global_trap_->SigSys(nr, info, void_context);
}

void Trap::SigSys(int nr, LinuxSigInfo* info, void* void_context) {
void Trap::SigSys(int nr, siginfo_t* info, void* void_context) {
// Signal handlers should always preserve "errno". Otherwise, we could
// trigger really subtle bugs.
const int old_errno = errno;

// Various sanity checks to make sure we actually received a signal
// triggered by a BPF filter. If something else triggered SIGSYS
// (e.g. kill()), there is really nothing we can do with this signal.
if (nr != LINUX_SIGSYS || info->si_code != SYS_SECCOMP || !void_context ||
if (nr != SIGSYS || info->si_code != SYS_SECCOMP || !void_context ||
info->si_errno <= 0 ||
static_cast<size_t>(info->si_errno) > trap_array_size_) {
// ATI drivers seem to send SIGSYS, so this cannot be FATAL.
Expand Down
6 changes: 3 additions & 3 deletions sandbox/linux/seccomp-bpf/trap.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
#ifndef SANDBOX_LINUX_SECCOMP_BPF_TRAP_H__
#define SANDBOX_LINUX_SECCOMP_BPF_TRAP_H__

#include <signal.h>
#include <stdint.h>

#include <map>

#include "base/macros.h"
#include "sandbox/linux/bpf_dsl/trap_registry.h"
#include "sandbox/linux/system_headers/linux_signal.h"
#include "sandbox/sandbox_export.h"

namespace sandbox {
Expand Down Expand Up @@ -57,11 +57,11 @@ class SANDBOX_EXPORT Trap : public bpf_dsl::TrapRegistry {
// break subsequent system calls that trigger a SIGSYS.
~Trap() = delete;

static void SigSysAction(int nr, LinuxSigInfo* info, void* void_context);
static void SigSysAction(int nr, siginfo_t* info, void* void_context);

// Make sure that SigSys is not inlined in order to get slightly better crash
// dumps.
void SigSys(int nr, LinuxSigInfo* info, void* void_context)
void SigSys(int nr, siginfo_t* info, void* void_context)
__attribute__((noinline));
// We have a global singleton that handles all of our SIGSYS traps. This
// variable must never be deallocated after it has been set up initially, as
Expand Down
28 changes: 0 additions & 28 deletions sandbox/linux/seccomp-bpf/trap_unittest.cc

This file was deleted.

10 changes: 7 additions & 3 deletions sandbox/linux/services/credentials.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,16 @@
#include "sandbox/linux/services/syscall_wrappers.h"
#include "sandbox/linux/services/thread_helpers.h"
#include "sandbox/linux/system_headers/capability.h"
#include "sandbox/linux/system_headers/linux_signal.h"

namespace sandbox {

namespace {

// Signal ABI for some toolchain is incompatible with Linux's. In particular,
// PNaCl toolchain defines SIGCHLD = 20. So, here, directly define Linux's
// value.
const int kLinuxSIGCHLD = 17;

bool IsRunningOnValgrind() { return RUNNING_ON_VALGRIND; }

// Checks that the set of RES-uids and the set of RES-gids have
Expand Down Expand Up @@ -92,8 +96,8 @@ bool ChrootToSafeEmptyDir() {
#endif

pid = clone(ChrootToSelfFdinfo, stack,
CLONE_VM | CLONE_VFORK | CLONE_FS | LINUX_SIGCHLD, nullptr,
nullptr, nullptr, nullptr);
CLONE_VM | CLONE_VFORK | CLONE_FS | kLinuxSIGCHLD,
nullptr, nullptr, nullptr, nullptr);
PCHECK(pid != -1);

int status = -1;
Expand Down
61 changes: 0 additions & 61 deletions sandbox/linux/services/syscall_wrappers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,12 @@
#include <sys/time.h>
#include <sys/types.h>
#include <unistd.h>
#include <cstring>

#include "base/compiler_specific.h"
#include "base/logging.h"
#include "base/third_party/valgrind/valgrind.h"
#include "build/build_config.h"
#include "sandbox/linux/system_headers/capability.h"
#include "sandbox/linux/system_headers/linux_signal.h"
#include "sandbox/linux/system_headers/linux_syscalls.h"

namespace sandbox {
Expand Down Expand Up @@ -139,63 +137,4 @@ int sys_unshare(int flags) {
return syscall(__NR_unshare, flags);
}

int sys_sigprocmask(int how, const sigset_t* set, decltype(nullptr) oldset) {
// In some toolchain (in particular Android and PNaCl toolchain),
// sigset_t is 32 bits, but Linux ABI requires 64 bits.
uint64_t linux_value = 0;
std::memcpy(&linux_value, set, std::min(sizeof(sigset_t), sizeof(uint64_t)));
return syscall(__NR_rt_sigprocmask, how, &linux_value, nullptr,
sizeof(linux_value));
}

// struct sigaction is different ABI from the Linux's.
struct KernelSigAction {
void (*kernel_handler)(int);
uint32_t sa_flags;
void (*sa_restorer)(void);
uint64_t sa_mask;
};

// On X86_64 arch, it is necessary to set sa_restorer always.
#if defined(ARCH_CPU_X86_64)
#if !defined(SA_RESTORER)
#define SA_RESTORER 0x04000000
#endif

static void sys_rt_sigreturn() {
syscall(__NR_rt_sigreturn);
}
#endif

int sys_sigaction(int signum,
const struct sigaction* act,
struct sigaction* oldact) {
KernelSigAction kernel_act = {};
if (act) {
kernel_act.kernel_handler = act->sa_handler;
std::memcpy(&kernel_act.sa_mask, &act->sa_mask,
std::min(sizeof(kernel_act.sa_mask), sizeof(act->sa_mask)));
kernel_act.sa_flags = act->sa_flags;

#if defined(ARCH_CPU_X86_64)
if (!(kernel_act.sa_flags & SA_RESTORER)) {
kernel_act.sa_flags |= SA_RESTORER;
kernel_act.sa_restorer = sys_rt_sigreturn;
}
#endif
}

KernelSigAction kernel_oldact = {};
int result = syscall(__NR_rt_sigaction, signum, act ? &kernel_act : nullptr,
oldact ? &kernel_oldact : nullptr, sizeof(uint64_t));
if (result == 0 && oldact) {
oldact->sa_handler = kernel_oldact.kernel_handler;
sigemptyset(&oldact->sa_mask);
std::memcpy(&oldact->sa_mask, &kernel_oldact.sa_mask,
std::min(sizeof(kernel_act.sa_mask), sizeof(act->sa_mask)));
oldact->sa_flags = kernel_oldact.sa_flags;
}
return result;
}

} // namespace sandbox
12 changes: 0 additions & 12 deletions sandbox/linux/services/syscall_wrappers.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#ifndef SANDBOX_LINUX_SERVICES_SYSCALL_WRAPPERS_H_
#define SANDBOX_LINUX_SERVICES_SYSCALL_WRAPPERS_H_

#include <signal.h>
#include <stdint.h>
#include <sys/types.h>

Expand Down Expand Up @@ -67,17 +66,6 @@ SANDBOX_EXPORT int sys_chroot(const char* path);
// Some libcs do not expose a unshare wrapper.
SANDBOX_EXPORT int sys_unshare(int flags);

// Some libcs do not expose a sigprocmask. Note that oldset must be a nullptr,
// because of some ABI gap between toolchain's and Linux's.
SANDBOX_EXPORT int sys_sigprocmask(int how,
const sigset_t* set,
decltype(nullptr) oldset);

// Some libcs do not expose a sigaction().
SANDBOX_EXPORT int sys_sigaction(int signum,
const struct sigaction* act,
struct sigaction* oldact);

} // namespace sandbox

#endif // SANDBOX_LINUX_SERVICES_SYSCALL_WRAPPERS_H_
Loading

0 comments on commit 958459a

Please sign in to comment.