From 537c3d8edfdcb00016b27a937b050086edcb0031 Mon Sep 17 00:00:00 2001 From: Alexandra Iordache Date: Thu, 9 May 2019 19:23:36 +0300 Subject: [PATCH 1/9] vmm: fix test_signal_handler This test fails intermittently when linked against glibc. Glibc's getpid() caches the result, so it doesn't always do the syscall. To avoid GNU-related complications, the test now forces a different syscall (mkdir) - as getpid will be needed in a later commit. Signed-off-by: Alexandra Iordache --- vmm/src/sigsys_handler.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/vmm/src/sigsys_handler.rs b/vmm/src/sigsys_handler.rs index a313d882451..54a21021860 100644 --- a/vmm/src/sigsys_handler.rs +++ b/vmm/src/sigsys_handler.rs @@ -88,9 +88,8 @@ mod tests { use super::*; use std::mem; - use std::process; - use libc::cpu_set_t; + use libc::{cpu_set_t, syscall}; use seccomp::{allow_syscall, SeccompAction, SeccompFilter}; @@ -144,8 +143,8 @@ mod tests { assert!(filter.apply().is_ok()); assert_eq!(METRICS.seccomp.num_faults.count(), 0); - // Calls the blacklisted SYS_getpid. - let _pid = process::id(); + // Call the blacklisted `SYS_mkdir`. + unsafe { syscall(libc::SYS_mkdir, "/foo/bar\0") }; assert!(cpu_count() > 0); From e277be101f6d5c2e6820737716be4297b15c284d Mon Sep 17 00:00:00 2001 From: Alexandra Iordache Date: Tue, 7 May 2019 14:37:54 +0300 Subject: [PATCH 2/9] vmm: rename sigsys_handler to signal_handler This step is in preparation to a refactoring that moves generic signal handling utilities to the sys_util crate, leaving only Firecracker specifics (read: custom signal handlers themselves) in vmm. Signed-off-by: Alexandra Iordache --- vmm/src/lib.rs | 6 +++--- vmm/src/{sigsys_handler.rs => signal_handler.rs} | 0 2 files changed, 3 insertions(+), 3 deletions(-) rename vmm/src/{sigsys_handler.rs => signal_handler.rs} (100%) diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index 0f759036e63..5f2e47fcfe1 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -39,8 +39,8 @@ extern crate sys_util; /// Syscalls allowed through the seccomp filter. pub mod default_syscalls; mod device_manager; -/// Signal handling utilities for seccomp violations. -mod sigsys_handler; +/// Signal handling utilities. +mod signal_handler; /// Wrappers over structures used to configure the VMM. pub mod vmm_config; mod vstate; @@ -76,7 +76,7 @@ use memory_model::{GuestAddress, GuestMemory}; use net_util::TapError; #[cfg(target_arch = "aarch64")] use serde_json::Value; -pub use sigsys_handler::setup_sigsys_handler; +pub use signal_handler::setup_sigsys_handler; use sys_util::{EventFd, Terminal}; use vmm_config::boot_source::{BootSourceConfig, BootSourceConfigError}; use vmm_config::drive::{BlockDeviceConfig, BlockDeviceConfigs, DriveError}; diff --git a/vmm/src/sigsys_handler.rs b/vmm/src/signal_handler.rs similarity index 100% rename from vmm/src/sigsys_handler.rs rename to vmm/src/signal_handler.rs From 8aefa724b3ff2a25eb192a611d452c718bf048b2 Mon Sep 17 00:00:00 2001 From: Alexandra Iordache Date: Tue, 7 May 2019 16:35:31 +0300 Subject: [PATCH 3/9] vmm/sys_util: refactor signal handling * added a single function in vmm that installs all relevant signal handlers; * left signal handler functions in vmm (currently only for SIGSYS); * moved signal handling logic to sys_util; * split sys_util's register_signal_handler into 2 separate functions, one to be called exclusively for vCPUs and a second general purpose one. The vCPU-specific one doen't change the signal mask and alters the signal number, forcing it between SIGRTMIN and SIGRTMAX. The general purpose one is setup_sigsys_handler renamed, and will morph into a generic one in a following commit. Signed-off-by: Alexandra Iordache --- src/main.rs | 6 +-- sys_util/src/signal.rs | 109 +++++++++++++++----------------------- vmm/src/lib.rs | 3 +- vmm/src/signal_handler.rs | 52 ++++++------------ vmm/src/vstate.rs | 4 +- 5 files changed, 64 insertions(+), 110 deletions(-) diff --git a/src/main.rs b/src/main.rs index 80849593237..d1849ccacf4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -30,6 +30,7 @@ use api_server::{ApiServer, Error}; use fc_util::validators::validate_instance_id; use logger::{Metric, LOGGER, METRICS}; use mmds::MMDS; +use vmm::signal_handler::register_signal_handlers; use vmm::vmm_config::instance_info::{InstanceInfo, InstanceState}; const DEFAULT_API_SOCK_PATH: &str = "/tmp/firecracker.socket"; @@ -40,11 +41,10 @@ fn main() { .preinit(Some(DEFAULT_INSTANCE_ID.to_string())) .expect("Failed to register logger"); - if let Err(e) = vmm::setup_sigsys_handler() { - error!("Failed to register signal handler: {}", e); + if let Err(e) = register_signal_handlers() { + error!("Failed to register signal handlers: {}", e); process::exit(i32::from(vmm::FC_EXIT_CODE_GENERIC_ERROR)); } - // Start firecracker by setting up a panic hook, which will be called before // terminating as we're building with panic = "abort". // It's worth noting that the abort is caused by sending a SIG_ABORT signal to the process. diff --git a/sys_util/src/signal.rs b/sys_util/src/signal.rs index 474693ae73b..6723acbb1cc 100644 --- a/sys_util/src/signal.rs +++ b/sys_util/src/signal.rs @@ -7,34 +7,17 @@ use super::SyscallReturnCode; use libc::{ - c_int, c_void, pthread_kill, pthread_t, sigaction, siginfo_t, EINVAL, SA_SIGINFO, SIGHUP, - SIGSYS, + c_int, c_void, pthread_kill, pthread_t, sigaction, sigfillset, siginfo_t, sigset_t, EINVAL, }; use std::io; use std::mem; use std::os::unix::thread::JoinHandleExt; +use std::ptr::null_mut; use std::thread::JoinHandle; -type SiginfoHandler = extern "C" fn(num: c_int, info: *mut siginfo_t, _unused: *mut c_void) -> (); - -pub enum SignalHandler { - Siginfo(SiginfoHandler), - // TODO add a`SimpleHandler` when `libc` adds `sa_handler` support to `sigaction`. -} - -/// Fills a `sigaction` structure from of the signal handler. -impl Into for SignalHandler { - fn into(self) -> sigaction { - let mut act: sigaction = unsafe { mem::zeroed() }; - match self { - SignalHandler::Siginfo(function) => { - act.sa_flags = SA_SIGINFO; - act.sa_sigaction = function as *const () as usize; - } - } - act - } -} +/// Type that represents a signal handler function. +pub type SignalHandler = + extern "C" fn(num: c_int, info: *mut siginfo_t, _unused: *mut c_void) -> (); extern "C" { fn __libc_current_sigrtmin() -> c_int; @@ -55,36 +38,47 @@ fn SIGRTMAX() -> c_int { /// Verifies that a signal number is valid. /// -/// VCPU signals need to have values enclosed within the OS limits for realtime signals, -/// and the remaining ones need to be between the minimum (SIGHUP) and maximum (SIGSYS) values. +/// VCPU signals need to have values enclosed within the OS limits for realtime signals. /// /// Will return Ok(num) or Err(EINVAL). -pub fn validate_signal_num(num: c_int, for_vcpu: bool) -> io::Result { - if for_vcpu { - let actual_num = num + SIGRTMIN(); - if actual_num <= SIGRTMAX() { - return Ok(actual_num); - } - } else if SIGHUP <= num && num <= SIGSYS { - return Ok(num); +pub fn validate_vcpu_signal_num(num: c_int) -> io::Result { + let actual_num = num + SIGRTMIN(); + if actual_num <= SIGRTMAX() { + Ok(actual_num) + } else { + Err(io::Error::from_raw_os_error(EINVAL)) } - Err(io::Error::from_raw_os_error(EINVAL)) } -/// Registers `handler` as the signal handler of signum `num`. -/// -/// Uses `sigaction` to register the handler. +/// Registers `handler` as the vCPU's signal handler of signum `num`. /// /// This is considered unsafe because the given handler will be called asynchronously, interrupting /// whatever the thread was doing and therefore must only do async-signal-safe operations. -pub unsafe fn register_signal_handler( - num: i32, - handler: SignalHandler, - for_vcpu: bool, -) -> io::Result<()> { - let num = validate_signal_num(num, for_vcpu)?; - let act: sigaction = handler.into(); - SyscallReturnCode(sigaction(num, &act, ::std::ptr::null_mut())).into_empty_result() +pub unsafe fn register_vcpu_signal_handler(num: i32, handler: SignalHandler) -> io::Result<()> { + let num = validate_vcpu_signal_num(num)?; + // Safe, because this is a POD struct. + let mut sigact: sigaction = mem::zeroed(); + sigact.sa_flags = libc::SA_SIGINFO; + sigact.sa_sigaction = handler as usize; + SyscallReturnCode(sigaction(num, &sigact, null_mut())).into_empty_result() +} + +/// Registers the specified signal handler for `SIGSYS`. +pub fn register_sigsys_handler(sigsys_handler: SignalHandler) -> Result<(), io::Error> { + // Safe, because this is a POD struct. + let mut sigact: sigaction = unsafe { mem::zeroed() }; + sigact.sa_flags = libc::SA_SIGINFO; + sigact.sa_sigaction = sigsys_handler as usize; + + // We set all the bits of sa_mask, so all signals are blocked on the current thread while the + // SIGSYS handler is executing. Safe because the parameter is valid and we check the return + // value. + if unsafe { sigfillset(&mut sigact.sa_mask as *mut sigset_t) } < 0 { + return Err(io::Error::last_os_error()); + } + + // Safe because the parameters are valid and we check the return value. + unsafe { SyscallReturnCode(sigaction(libc::SIGSYS, &sigact, null_mut())).into_empty_result() } } /// Trait for threads that can be signalled via `pthread_kill`. @@ -101,7 +95,7 @@ pub unsafe trait Killable { /// /// The value of `num + SIGRTMIN` must not exceed `SIGRTMAX`. fn kill(&self, num: i32) -> io::Result<()> { - let num = validate_signal_num(num, true)?; + let num = validate_vcpu_signal_num(num)?; // Safe because we ensure we are using a valid pthread handle, a valid signal number, and // check the return result. @@ -119,7 +113,6 @@ unsafe impl Killable for JoinHandle { #[cfg(test)] mod tests { use super::*; - use libc; use std::thread; use std::time::Duration; @@ -135,25 +128,9 @@ mod tests { fn test_register_signal_handler() { unsafe { // testing bad value - assert!(register_signal_handler( - SIGRTMAX(), - SignalHandler::Siginfo(handle_signal), - true - ) - .is_err()); - format!( - "{:?}", - register_signal_handler(SIGRTMAX(), SignalHandler::Siginfo(handle_signal), true) - ); - assert!( - register_signal_handler(0, SignalHandler::Siginfo(handle_signal), true).is_ok() - ); - assert!(register_signal_handler( - libc::SIGSYS, - SignalHandler::Siginfo(handle_signal), - false - ) - .is_ok()); + assert!(register_vcpu_signal_handler(SIGRTMAX(), handle_signal).is_err()); + assert!(register_vcpu_signal_handler(0, handle_signal).is_ok()); + assert!(register_sigsys_handler(handle_signal).is_ok()); } } @@ -168,7 +145,7 @@ mod tests { // be brought down when the signal is received, as part of the default behaviour. Signal // handlers are global, so we install this before starting the thread. unsafe { - register_signal_handler(0, SignalHandler::Siginfo(handle_signal), true) + register_vcpu_signal_handler(0, handle_signal) .expect("failed to register vcpu signal handler"); } diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index 5f2e47fcfe1..e83db95ffe4 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -40,7 +40,7 @@ extern crate sys_util; pub mod default_syscalls; mod device_manager; /// Signal handling utilities. -mod signal_handler; +pub mod signal_handler; /// Wrappers over structures used to configure the VMM. pub mod vmm_config; mod vstate; @@ -76,7 +76,6 @@ use memory_model::{GuestAddress, GuestMemory}; use net_util::TapError; #[cfg(target_arch = "aarch64")] use serde_json::Value; -pub use signal_handler::setup_sigsys_handler; use sys_util::{EventFd, Terminal}; use vmm_config::boot_source::{BootSourceConfig, BootSourceConfigError}; use vmm_config::drive::{BlockDeviceConfig, BlockDeviceConfigs, DriveError}; diff --git a/vmm/src/signal_handler.rs b/vmm/src/signal_handler.rs index 54a21021860..2d78c238796 100644 --- a/vmm/src/signal_handler.rs +++ b/vmm/src/signal_handler.rs @@ -5,13 +5,12 @@ extern crate logger; extern crate sys_util; use std::io; -use std::mem; -use std::ptr::null_mut; use std::result::Result; -use libc::{sigaction, sigfillset, sigset_t}; +use libc::{_exit, c_int, c_void, siginfo_t, SIGSYS}; use logger::{Metric, LOGGER, METRICS}; +use sys_util::register_sigsys_handler; // The offset of `si_syscall` (offending syscall identifier) within the siginfo structure // expressed as an `(u)int*`. @@ -22,44 +21,15 @@ const SI_OFF_SYSCALL: isize = 6; const SYS_SECCOMP_CODE: i32 = 1; -// This no longer relies on sys_util::register_signal_handler(), which is a lot weirder than it -// should be (at least for this use case). Also, we want to change the sa_mask field of the -// sigaction struct. -/// Sets up the specified signal handler for `SIGSYS`. -pub fn setup_sigsys_handler() -> Result<(), io::Error> { - // Safe, because this is a POD struct. - let mut sigact: sigaction = unsafe { mem::zeroed() }; - sigact.sa_flags = libc::SA_SIGINFO; - sigact.sa_sigaction = sigsys_handler as usize; - - // We set all the bits of sa_mask, so all signals are blocked on the current thread while the - // SIGSYS handler is executing. Safe because the parameter is valid and we check the return - // value. - if unsafe { sigfillset(&mut sigact.sa_mask as *mut sigset_t) } < 0 { - return Err(io::Error::last_os_error()); - } - - // Safe because the parameters are valid and we check the return value. - if unsafe { sigaction(libc::SIGSYS, &sigact, null_mut()) } < 0 { - return Err(io::Error::last_os_error()); - } - - Ok(()) -} - -extern "C" fn sigsys_handler( - num: libc::c_int, - info: *mut libc::siginfo_t, - _unused: *mut libc::c_void, -) { +extern "C" fn sigsys_handler(num: c_int, info: *mut siginfo_t, _unused: *mut c_void) { // Safe because we're just reading some fields from a supposedly valid argument. let si_signo = unsafe { (*info).si_signo }; let si_code = unsafe { (*info).si_code }; // Sanity check. The condition should never be true. - if num != si_signo || num != libc::SIGSYS || si_code != SYS_SECCOMP_CODE as i32 { + if num != si_signo || num != SIGSYS || si_code != SYS_SECCOMP_CODE as i32 { // Safe because we're terminating the process anyway. - unsafe { libc::_exit(i32::from(super::FC_EXIT_CODE_UNEXPECTED_ERROR)) }; + unsafe { _exit(i32::from(super::FC_EXIT_CODE_UNEXPECTED_ERROR)) }; } // Other signals which might do async unsafe things incompatible with the rest of this @@ -79,10 +49,18 @@ extern "C" fn sigsys_handler( // running unit tests. #[cfg(not(test))] unsafe { - libc::_exit(i32::from(super::FC_EXIT_CODE_BAD_SYSCALL)) + _exit(i32::from(super::FC_EXIT_CODE_BAD_SYSCALL)) }; } +/// Registers all the required signal handlers. +/// +/// Custom handlers are installed for: `SIGSYS`. +/// +pub fn register_signal_handlers() -> Result<(), io::Error> { + register_sigsys_handler(sigsys_handler) +} + #[cfg(test)] mod tests { use super::*; @@ -119,7 +97,7 @@ mod tests { #[test] fn test_signal_handler() { - assert!(setup_sigsys_handler().is_ok()); + assert!(register_signal_handlers().is_ok()); let filter = SeccompFilter::new( vec![ diff --git a/vmm/src/vstate.rs b/vmm/src/vstate.rs index 46cf0e523bc..4e368d3ffc6 100644 --- a/vmm/src/vstate.rs +++ b/vmm/src/vstate.rs @@ -440,7 +440,7 @@ mod tests { use super::*; use libc::{c_int, c_void, siginfo_t}; - use sys_util::{register_signal_handler, Killable, SignalHandler}; + use sys_util::{register_vcpu_signal_handler, Killable}; // Auxiliary function being used throughout the tests. fn setup_vcpu() -> (Vm, Vcpu) { @@ -646,7 +646,7 @@ mod tests { // be brought down when the signal is received, as part of the default behaviour. Signal // handlers are global, so we install this before starting the thread. unsafe { - register_signal_handler(signum, SignalHandler::Siginfo(handle_signal), true) + register_vcpu_signal_handler(signum, handle_signal) .expect("failed to register vcpu signal handler"); } From 65dc516420d5b085156949d43e3dd02de0f689ce Mon Sep 17 00:00:00 2001 From: Alexandra Iordache Date: Tue, 7 May 2019 16:49:36 +0300 Subject: [PATCH 4/9] sys_util: generic register_signal_handler Renamed register_sigsys_handler to register_signal_handler, enabling the installation of custom signal handlers for any signal. Signed-off-by: Alexandra Iordache --- sys_util/src/signal.rs | 41 +++++++++++++++++++++++++++++---------- vmm/src/signal_handler.rs | 4 ++-- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/sys_util/src/signal.rs b/sys_util/src/signal.rs index 6723acbb1cc..eeaea353d0a 100644 --- a/sys_util/src/signal.rs +++ b/sys_util/src/signal.rs @@ -8,6 +8,7 @@ use super::SyscallReturnCode; use libc::{ c_int, c_void, pthread_kill, pthread_t, sigaction, sigfillset, siginfo_t, sigset_t, EINVAL, + SIGHUP, SIGSYS, }; use std::io; use std::mem; @@ -36,12 +37,12 @@ fn SIGRTMAX() -> c_int { unsafe { __libc_current_sigrtmax() } } -/// Verifies that a signal number is valid. +/// Verifies that a signal number is valid when sent to a vCPU. /// /// VCPU signals need to have values enclosed within the OS limits for realtime signals. /// /// Will return Ok(num) or Err(EINVAL). -pub fn validate_vcpu_signal_num(num: c_int) -> io::Result { +fn validate_vcpu_signal_num(num: c_int) -> io::Result { let actual_num = num + SIGRTMIN(); if actual_num <= SIGRTMAX() { Ok(actual_num) @@ -50,12 +51,28 @@ pub fn validate_vcpu_signal_num(num: c_int) -> io::Result { } } -/// Registers `handler` as the vCPU's signal handler of signum `num`. +/// Verifies that a signal number is valid when sent to the process. +/// +/// Signals can take values between `SIGHUB` and `SIGSYS`. +/// +/// Will return Ok(num) or Err(EINVAL). +fn validate_signal_num(num: c_int) -> io::Result { + if num >= SIGHUP && num <= SIGSYS { + Ok(num) + } else { + Err(io::Error::from_raw_os_error(EINVAL)) + } +} + +/// Registers `handler` as the vCPU's signal handler of `signum`. /// /// This is considered unsafe because the given handler will be called asynchronously, interrupting /// whatever the thread was doing and therefore must only do async-signal-safe operations. -pub unsafe fn register_vcpu_signal_handler(num: i32, handler: SignalHandler) -> io::Result<()> { - let num = validate_vcpu_signal_num(num)?; +pub unsafe fn register_vcpu_signal_handler( + signum: c_int, + handler: SignalHandler, +) -> io::Result<()> { + let num = validate_vcpu_signal_num(signum)?; // Safe, because this is a POD struct. let mut sigact: sigaction = mem::zeroed(); sigact.sa_flags = libc::SA_SIGINFO; @@ -63,12 +80,13 @@ pub unsafe fn register_vcpu_signal_handler(num: i32, handler: SignalHandler) -> SyscallReturnCode(sigaction(num, &sigact, null_mut())).into_empty_result() } -/// Registers the specified signal handler for `SIGSYS`. -pub fn register_sigsys_handler(sigsys_handler: SignalHandler) -> Result<(), io::Error> { +/// Registers `handler` as the process' signal handler of `signum`. +pub fn register_signal_handler(signum: c_int, handler: SignalHandler) -> Result<(), io::Error> { + let num = validate_signal_num(signum)?; // Safe, because this is a POD struct. let mut sigact: sigaction = unsafe { mem::zeroed() }; sigact.sa_flags = libc::SA_SIGINFO; - sigact.sa_sigaction = sigsys_handler as usize; + sigact.sa_sigaction = handler as usize; // We set all the bits of sa_mask, so all signals are blocked on the current thread while the // SIGSYS handler is executing. Safe because the parameter is valid and we check the return @@ -78,7 +96,7 @@ pub fn register_sigsys_handler(sigsys_handler: SignalHandler) -> Result<(), io:: } // Safe because the parameters are valid and we check the return value. - unsafe { SyscallReturnCode(sigaction(libc::SIGSYS, &sigact, null_mut())).into_empty_result() } + unsafe { SyscallReturnCode(sigaction(num, &sigact, null_mut())).into_empty_result() } } /// Trait for threads that can be signalled via `pthread_kill`. @@ -116,6 +134,8 @@ mod tests { use std::thread; use std::time::Duration; + use libc::SIGSYS; + static mut SIGNAL_HANDLER_CALLED: bool = false; extern "C" fn handle_signal(_: c_int, _: *mut siginfo_t, _: *mut c_void) { @@ -130,7 +150,8 @@ mod tests { // testing bad value assert!(register_vcpu_signal_handler(SIGRTMAX(), handle_signal).is_err()); assert!(register_vcpu_signal_handler(0, handle_signal).is_ok()); - assert!(register_sigsys_handler(handle_signal).is_ok()); + assert!(register_signal_handler(SIGSYS, handle_signal).is_ok()); + assert!(register_signal_handler(SIGSYS + 1, handle_signal).is_err()); } } diff --git a/vmm/src/signal_handler.rs b/vmm/src/signal_handler.rs index 2d78c238796..3eed034ebea 100644 --- a/vmm/src/signal_handler.rs +++ b/vmm/src/signal_handler.rs @@ -10,7 +10,7 @@ use std::result::Result; use libc::{_exit, c_int, c_void, siginfo_t, SIGSYS}; use logger::{Metric, LOGGER, METRICS}; -use sys_util::register_sigsys_handler; +use sys_util::register_signal_handler; // The offset of `si_syscall` (offending syscall identifier) within the siginfo structure // expressed as an `(u)int*`. @@ -58,7 +58,7 @@ extern "C" fn sigsys_handler(num: c_int, info: *mut siginfo_t, _unused: *mut c_v /// Custom handlers are installed for: `SIGSYS`. /// pub fn register_signal_handlers() -> Result<(), io::Error> { - register_sigsys_handler(sigsys_handler) + register_signal_handler(SIGSYS, sigsys_handler) } #[cfg(test)] From f533bf11de7c455144b313a0496ff0f659978b27 Mon Sep 17 00:00:00 2001 From: Alexandra Iordache Date: Tue, 7 May 2019 17:25:20 +0300 Subject: [PATCH 5/9] vmm: handle SIGBUS & SIGSEGV Log a message and exit with a specific exit code upon intercepting SIGBUS/SIGSEGV. Signed-off-by: Alexandra Iordache --- vmm/src/lib.rs | 4 +++ vmm/src/signal_handler.rs | 52 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index e83db95ffe4..6b0cccd5044 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -112,6 +112,10 @@ pub const FC_EXIT_CODE_GENERIC_ERROR: u8 = 1; pub const FC_EXIT_CODE_UNEXPECTED_ERROR: u8 = 2; /// Firecracker was shut down after intercepting a restricted system call. pub const FC_EXIT_CODE_BAD_SYSCALL: u8 = 148; +/// Firecracker was shut down after intercepting `SIGBUS`. +pub const FC_EXIT_CODE_SIGBUS: u8 = 149; +/// Firecracker was shut down after intercepting `SIGSEGV`. +pub const FC_EXIT_CODE_SIGSEGV: u8 = 150; /// Errors associated with the VMM internal logic. These errors cannot be generated by direct user /// input, but can result from bad configuration of the host (for example if Firecracker doesn't diff --git a/vmm/src/signal_handler.rs b/vmm/src/signal_handler.rs index 3eed034ebea..ba7a138f64c 100644 --- a/vmm/src/signal_handler.rs +++ b/vmm/src/signal_handler.rs @@ -7,7 +7,7 @@ extern crate sys_util; use std::io; use std::result::Result; -use libc::{_exit, c_int, c_void, siginfo_t, SIGSYS}; +use libc::{_exit, c_int, c_void, siginfo_t, SIGBUS, SIGSEGV, SIGSYS}; use logger::{Metric, LOGGER, METRICS}; use sys_util::register_signal_handler; @@ -53,12 +53,49 @@ extern "C" fn sigsys_handler(num: c_int, info: *mut siginfo_t, _unused: *mut c_v }; } +extern "C" fn sigbus_sigsegv_handler(num: c_int, info: *mut siginfo_t, _unused: *mut c_void) { + // Safe because we're just reading some fields from a supposedly valid argument. + let si_signo = unsafe { (*info).si_signo }; + let si_code = unsafe { (*info).si_code }; + + // Sanity check. The condition should never be true. + if num != si_signo || (num != SIGBUS && num != SIGSEGV) { + // Safe because we're terminating the process anyway. + unsafe { _exit(i32::from(super::FC_EXIT_CODE_UNEXPECTED_ERROR)) }; + } + + // Other signals which might do async unsafe things incompatible with the rest of this + // function are blocked due to the sa_mask used when registering the signal handler. + error!( + "Shutting down VM after intercepting signal {}, code {}.", + si_signo, si_code + ); + // Log the metrics before exiting. + if let Err(e) = LOGGER.log_metrics() { + error!("Failed to log metrics while stopping: {}", e); + } + + // Safe because we're terminating the process anyway. We don't actually do anything when + // running unit tests. + #[cfg(not(test))] + unsafe { + _exit(i32::from(match si_signo { + SIGBUS => super::FC_EXIT_CODE_SIGBUS, + SIGSEGV => super::FC_EXIT_CODE_SIGSEGV, + _ => super::FC_EXIT_CODE_UNEXPECTED_ERROR, + })) + }; +} + /// Registers all the required signal handlers. /// -/// Custom handlers are installed for: `SIGSYS`. +/// Custom handlers are installed for: `SIGBUS`, `SIGSEGV`, `SIGSYS`. /// pub fn register_signal_handlers() -> Result<(), io::Error> { - register_signal_handler(SIGSYS, sigsys_handler) + register_signal_handler(SIGSYS, sigsys_handler)?; + register_signal_handler(SIGBUS, sigbus_sigsegv_handler)?; + register_signal_handler(SIGSEGV, sigbus_sigsegv_handler)?; + Ok(()) } #[cfg(test)] @@ -66,6 +103,7 @@ mod tests { use super::*; use std::mem; + use std::process; use libc::{cpu_set_t, syscall}; @@ -104,6 +142,8 @@ mod tests { allow_syscall(libc::SYS_brk), allow_syscall(libc::SYS_exit), allow_syscall(libc::SYS_futex), + allow_syscall(libc::SYS_getpid), + allow_syscall(libc::SYS_kill), allow_syscall(libc::SYS_munmap), allow_syscall(libc::SYS_rt_sigprocmask), allow_syscall(libc::SYS_rt_sigreturn), @@ -136,5 +176,11 @@ mod tests { // The signal handler should let the program continue during unit tests. assert_eq!(METRICS.seccomp.num_faults.count(), 1); } + + // Assert that the SIGBUS handler left the process alive. + unsafe { + syscall(libc::SYS_kill, process::id(), SIGBUS); + } + assert!(true); } } From 95f5d7ee97d2991a385fb00fe1c40e3231105b5f Mon Sep 17 00:00:00 2001 From: Alexandra Iordache Date: Thu, 9 May 2019 15:21:27 +0300 Subject: [PATCH 6/9] documentation & doctests for signal handling utils Signed-off-by: Alexandra Iordache --- sys_util/src/signal.rs | 72 ++++++++++++++++++++++++++++++++++++--- vmm/src/signal_handler.rs | 9 +++++ 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/sys_util/src/signal.rs b/sys_util/src/signal.rs index eeaea353d0a..f433ea21eb0 100644 --- a/sys_util/src/signal.rs +++ b/sys_util/src/signal.rs @@ -40,10 +40,14 @@ fn SIGRTMAX() -> c_int { /// Verifies that a signal number is valid when sent to a vCPU. /// /// VCPU signals need to have values enclosed within the OS limits for realtime signals. +/// Returns either `Ok(num)` or `Err(EINVAL)`. /// -/// Will return Ok(num) or Err(EINVAL). -fn validate_vcpu_signal_num(num: c_int) -> io::Result { - let actual_num = num + SIGRTMIN(); +/// # Arguments +/// +/// * `signum`: signal number. +/// +fn validate_vcpu_signal_num(signum: c_int) -> io::Result { + let actual_num = signum + SIGRTMIN(); if actual_num <= SIGRTMAX() { Ok(actual_num) } else { @@ -54,8 +58,12 @@ fn validate_vcpu_signal_num(num: c_int) -> io::Result { /// Verifies that a signal number is valid when sent to the process. /// /// Signals can take values between `SIGHUB` and `SIGSYS`. +/// Returns either `Ok(num)` or `Err(EINVAL)`. +/// +/// # Arguments +/// +/// * `signum`: signal number. /// -/// Will return Ok(num) or Err(EINVAL). fn validate_signal_num(num: c_int) -> io::Result { if num >= SIGHUP && num <= SIGSYS { Ok(num) @@ -68,6 +76,34 @@ fn validate_signal_num(num: c_int) -> io::Result { /// /// This is considered unsafe because the given handler will be called asynchronously, interrupting /// whatever the thread was doing and therefore must only do async-signal-safe operations. +/// +/// # Arguments +/// +/// * `signum`: signal number. +/// * `handler`: signal handler functor. +/// +/// # Example +/// +/// ``` +/// extern crate libc; +/// extern crate sys_util; +/// +/// use libc::{c_int, c_void, raise, siginfo_t}; +/// use sys_util::register_vcpu_signal_handler; +/// +/// extern "C" fn handle_signal(_: c_int, _: *mut siginfo_t, _: *mut c_void) {} +/// extern "C" { fn __libc_current_sigrtmin() -> c_int; } +/// +/// fn main() { +/// // Register dummy signal handler for `SIGRTMIN`. +/// assert!(unsafe { register_vcpu_signal_handler(0, handle_signal).is_ok() }); +/// // Raise `SIGRTMIN`. +/// unsafe { raise(__libc_current_sigrtmin()); } +/// // Assert that the process is still alive. +/// assert!(true); +/// } +/// ``` +/// pub unsafe fn register_vcpu_signal_handler( signum: c_int, handler: SignalHandler, @@ -81,6 +117,34 @@ pub unsafe fn register_vcpu_signal_handler( } /// Registers `handler` as the process' signal handler of `signum`. +/// +/// # Arguments +/// +/// * `signum`: signal number. +/// * `handler`: signal handler functor. +/// +/// # Example +/// +/// ``` +/// extern crate libc; +/// extern crate sys_util; +/// +/// use std::sync::atomic::{AtomicBool, Ordering, ATOMIC_BOOL_INIT}; +/// use libc::{c_int, c_void, raise, siginfo_t, SIGUSR1}; +/// use sys_util::register_signal_handler; +/// +/// static HANDLER_CALLED: AtomicBool = ATOMIC_BOOL_INIT; +/// extern "C" fn handle_signal(_: c_int, _: *mut siginfo_t, _: *mut c_void) { +/// HANDLER_CALLED.store(true, Ordering::SeqCst); +/// } +/// +/// fn main() { +/// assert!(unsafe { register_signal_handler(SIGUSR1, handle_signal).is_ok() }); +/// unsafe { raise(SIGUSR1); } +/// assert!(HANDLER_CALLED.load(Ordering::SeqCst)); +/// } +/// ``` +/// pub fn register_signal_handler(signum: c_int, handler: SignalHandler) -> Result<(), io::Error> { let num = validate_signal_num(signum)?; // Safe, because this is a POD struct. diff --git a/vmm/src/signal_handler.rs b/vmm/src/signal_handler.rs index ba7a138f64c..0bb3ba9964a 100644 --- a/vmm/src/signal_handler.rs +++ b/vmm/src/signal_handler.rs @@ -21,6 +21,11 @@ const SI_OFF_SYSCALL: isize = 6; const SYS_SECCOMP_CODE: i32 = 1; +/// Signal handler for `SIGSYS`. +/// +/// Increments the `seccomp.num_faults` metric, logs an error message and terminates the process +/// with a specific exit code. +/// extern "C" fn sigsys_handler(num: c_int, info: *mut siginfo_t, _unused: *mut c_void) { // Safe because we're just reading some fields from a supposedly valid argument. let si_signo = unsafe { (*info).si_signo }; @@ -53,6 +58,10 @@ extern "C" fn sigsys_handler(num: c_int, info: *mut siginfo_t, _unused: *mut c_v }; } +/// Signal handler for `SIGBUS` and `SIGSEGV`. +/// +/// Logs an error message and terminates the process with a specific exit code. +/// extern "C" fn sigbus_sigsegv_handler(num: c_int, info: *mut siginfo_t, _unused: *mut c_void) { // Safe because we're just reading some fields from a supposedly valid argument. let si_signo = unsafe { (*info).si_signo }; From 1f722e25295a065808bf490684594923aad04722 Mon Sep 17 00:00:00 2001 From: Alexandra Iordache Date: Thu, 9 May 2019 15:29:11 +0300 Subject: [PATCH 7/9] tests: update licenses test The license checker now accepts files licensed in 2018 and 2019. Signed-off-by: Alexandra Iordache --- tests/integration_tests/build/test_licenses.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/integration_tests/build/test_licenses.py b/tests/integration_tests/build/test_licenses.py index 20c52772d2d..05e789045f6 100644 --- a/tests/integration_tests/build/test_licenses.py +++ b/tests/integration_tests/build/test_licenses.py @@ -6,8 +6,9 @@ import pytest +AMAZON_COPYRIGHT_YEARS = (2018, 2019) AMAZON_COPYRIGHT = ( - "Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved." + "Copyright {} Amazon.com, Inc. or its affiliates. All Rights Reserved." ) AMAZON_LICENSE = ( "SPDX-License-Identifier: Apache-2.0" @@ -28,7 +29,14 @@ EXCLUDED_DIRECTORIES = ((os.path.join(os.getcwd(), 'build'))) -def validate_license(filename): +def _has_amazon_copyright(string): + for year in AMAZON_COPYRIGHT_YEARS: + if AMAZON_COPYRIGHT.format(year) in string: + return True + return False + + +def _validate_license(filename): """Validate licenses in all .rs, .py. and .sh file. Python and Rust files should have the licenses on the first 2 lines @@ -45,7 +53,7 @@ def validate_license(filename): copy = file.readline() local_license = file.readline() has_amazon_copyright = ( - AMAZON_COPYRIGHT in copy and + _has_amazon_copyright(copy) and AMAZON_LICENSE in local_license ) has_chromium_copyright = ( @@ -70,5 +78,5 @@ def test_for_valid_licenses(): for subdir, _, files in os.walk(os.getcwd()): for file in files: filepath = os.path.join(subdir, file) - assert validate_license(filepath) is True, \ + assert _validate_license(filepath) is True, \ "%s has invalid license" % filepath From 0bf17d8055a052c6eb8aed24ba98be42a4dc8099 Mon Sep 17 00:00:00 2001 From: Alexandra Iordache Date: Thu, 9 May 2019 14:07:29 +0300 Subject: [PATCH 8/9] tests: add integ test for SIGBUS/SIGSEGV handling The test checks that Firecracker logs the appropriate mesage when intercepting the signal. Exit code is not checked because, as the jailer clones into a new pid namespace, it's no longer in the test process' tree and its pid cannot be waited on. Signed-off-by: Alexandra Iordache --- .../functional/test_signals.py | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 tests/integration_tests/functional/test_signals.py diff --git a/tests/integration_tests/functional/test_signals.py b/tests/integration_tests/functional/test_signals.py new file mode 100644 index 00000000000..6ce35ef8d7c --- /dev/null +++ b/tests/integration_tests/functional/test_signals.py @@ -0,0 +1,56 @@ +# Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Tests scenarios for Firecracker signal handling.""" + +import os +from signal import SIGBUS, SIGSEGV +from time import sleep + +import pytest + +import host_tools.logging as log_tools + + +@pytest.mark.parametrize( + "signum", + [SIGBUS, SIGSEGV] +) +def test_sigbus_sigsegv(test_microvm_with_api, signum): + """Test signal handling for `SIGBUS` and `SIGSEGV`.""" + test_microvm = test_microvm_with_api + test_microvm.spawn() + + # We don't need to monitor the memory for this test. + test_microvm.memory_events_queue = None + + test_microvm.basic_config() + + # Configure logging. + log_fifo_path = os.path.join(test_microvm.path, 'log_fifo') + metrics_fifo_path = os.path.join(test_microvm.path, 'metrics_fifo') + log_fifo = log_tools.Fifo(log_fifo_path) + metrics_fifo = log_tools.Fifo(metrics_fifo_path) + + response = test_microvm.logger.put( + log_fifo=test_microvm.create_jailed_resource(log_fifo.path), + metrics_fifo=test_microvm.create_jailed_resource(metrics_fifo.path), + level='Error', + show_level=False, + show_log_origin=False + ) + assert test_microvm.api_session.is_status_no_content(response.status_code) + + test_microvm.start() + firecracker_pid = int(test_microvm.jailer_clone_pid) + + sleep(0.5) + os.kill(firecracker_pid, signum) + + msg = 'Shutting down VM after intercepting signal {}'.format(signum) + lines = log_fifo.sequential_reader(5) + msg_found = False + for line in lines: + if msg in line: + msg_found = True + break + assert msg_found From 333869bb8b2309621b496307fc6557b94611fbd5 Mon Sep 17 00:00:00 2001 From: Alexandra Iordache Date: Thu, 9 May 2019 14:40:30 +0300 Subject: [PATCH 9/9] changelog: add note on new signal handler Signed-off-by: Alexandra Iordache --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9455b6d7d5..cb139dcedf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ - Added an experimental swagger definition that includes the specification for the vsock API call. +- Added a signal handler for `SIGBUS` and `SIGSEGV` that immediately terminates + the process upon intercepting the signal. ## [0.16.0]