vmm: ensure signal handlers run on the right thread

Despite setting up a dedicated thread for signal handling, we weren't
making sure that the signals we were listening for there were actually
dispatched to the right thread.  While the signal-hook provides an
iterator API, so we can know that we're only processing the signals
coming out of the iterator on our signal handling thread, the actual
signal handling code from signal-hook, which pushes the signals onto
the iterator, can run on any thread.  This can lead to seccomp
violations when the signal-hook signal handler does something that
isn't allowed on that thread by our seccomp policy.

To reproduce, resize a terminal running cloud-hypervisor continuously
for a few minutes.  Eventually, the kernel will deliver a SIGWINCH to
a thread with a restrictive seccomp policy, and a seccomp violation
will trigger.

As part of this change, it's also necessary to allow rt_sigreturn(2)
on the signal handling thread, so signal handlers are actually allowed
to run on it.  The fact that this didn't seem to be needed before
makes me think that signal handlers were almost _never_ actually
running on the signal handling thread.

Signed-off-by: Alyssa Ross <hi@alyssa.is>
This commit is contained in:
Alyssa Ross 2021-09-02 19:16:45 +00:00 committed by Rob Bradford
parent c2144b5690
commit 7549149bb5
3 changed files with 17 additions and 1 deletions

View File

@ -26,6 +26,7 @@ use std::thread;
use thiserror::Error;
use vmm::config;
use vmm_sys_util::eventfd::EventFd;
use vmm_sys_util::signal::block_signal;
#[derive(Error, Debug)]
enum Error {
@ -496,6 +497,13 @@ fn start_vmm(cmd_arguments: ArgMatches) -> Result<Option<String>, Error> {
.unwrap();
}
// Before we start any threads, mask the signals we'll be
// installing handlers for, to make sure they only ever run on the
// dedicated signal handling thread we'll start in a bit.
for sig in vmm::vm::HANDLED_SIGNALS {
block_signal(sig).unwrap();
}
event!("vmm", "starting");
let hypervisor = hypervisor::new().map_err(Error::CreateHypervisor)?;

View File

@ -359,6 +359,7 @@ fn signal_handler_thread_rules() -> Result<Vec<(i64, Vec<SeccompRule>)>, Backend
(libc::SYS_munmap, vec![]),
(libc::SYS_recvfrom, vec![]),
(libc::SYS_rt_sigprocmask, vec![]),
(libc::SYS_rt_sigreturn, vec![]),
(libc::SYS_sendto, vec![]),
(libc::SYS_sigaltstack, vec![]),
(libc::SYS_write, vec![]),

View File

@ -76,6 +76,7 @@ use vm_migration::{
Transportable,
};
use vmm_sys_util::eventfd::EventFd;
use vmm_sys_util::signal::unblock_signal;
use vmm_sys_util::terminal::Terminal;
#[cfg(target_arch = "aarch64")]
@ -465,6 +466,8 @@ pub fn physical_bits(max_phys_bits: Option<u8>, #[cfg(feature = "tdx")] tdx_enab
cmp::min(host_phys_bits, max_phys_bits.unwrap_or(host_phys_bits))
}
pub const HANDLED_SIGNALS: [i32; 3] = [SIGWINCH, SIGTERM, SIGINT];
pub struct Vm {
kernel: Option<File>,
initramfs: Option<File>,
@ -1591,6 +1594,10 @@ impl Vm {
on_tty: bool,
exit_evt: EventFd,
) {
for sig in HANDLED_SIGNALS {
unblock_signal(sig).unwrap();
}
for signal in signals.forever() {
match signal {
SIGWINCH => {
@ -1867,7 +1874,7 @@ impl Vm {
.input_enabled()
{
let console = self.device_manager.lock().unwrap().console().clone();
let signals = Signals::new(&[SIGWINCH, SIGINT, SIGTERM]);
let signals = Signals::new(&HANDLED_SIGNALS);
match signals {
Ok(signals) => {
self.signals = Some(signals.handle());