From 7549149bb5aaca535f5d64fc5aeffdfc9834626f Mon Sep 17 00:00:00 2001
From: Alyssa Ross <hi@alyssa.is>
Date: Thu, 2 Sep 2021 19:16:45 +0000
Subject: [PATCH] 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>
---
 src/main.rs                | 8 ++++++++
 vmm/src/seccomp_filters.rs | 1 +
 vmm/src/vm.rs              | 9 ++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/main.rs b/src/main.rs
index 92217ee8a..afcaa1a49 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -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)?;
diff --git a/vmm/src/seccomp_filters.rs b/vmm/src/seccomp_filters.rs
index 308c3cb96..f6069caf6 100644
--- a/vmm/src/seccomp_filters.rs
+++ b/vmm/src/seccomp_filters.rs
@@ -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![]),
diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs
index ed3557e3b..21d04dd3e 100644
--- a/vmm/src/vm.rs
+++ b/vmm/src/vm.rs
@@ -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());