Fix bug where stale signals can interrupt wrong sandbox, and allow preventing host functions to return control to guest after being interrupted

Signed-off-by: Ludvig Liljenberg <[email protected]>

Author: ludfjig

src/hyperlight_host/src/hypervisor/hyperv_linux.rs

@@ -395,6 +395,7 @@ impl HypervLinuxDriver {
             orig_rsp: rsp_ptr,
             interrupt_handle: Arc::new(LinuxInterruptHandle {
                 running: AtomicBool::new(false),
+                cancel_requested: AtomicBool::new(false),
                 tid: AtomicU64::new(unsafe { libc::pthread_self() }),
                 dropped: AtomicBool::new(false),
             }),
@@ -584,37 +585,52 @@ impl Hypervisor for HypervLinuxDriver {
         self.interrupt_handle
             .tid
             .store(unsafe { libc::pthread_self() as u64 }, Ordering::Relaxed);
-        // Note: if a `InterruptHandle::kill()` signal is delivered to this thread **here**
-        // - before we've set the running to true,
-        // Then the signal does not have any effect, because the signal handler is a no-op.
+        // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+        // Then this is fine since `cancel_requested` is set to true, so we will skip the `VcpuFd::run()` call
         self.interrupt_handle.running.store(true, Ordering::Relaxed);
-        // Note: if a `InterruptHandle::kill()` signal is delivered to this thread **here**
-        // - after we've set the running to true,
-        // - before we've called `VcpuFd::run()`
-        // Then the individual signal is lost, because the signal is only processed after we've left userspace.
-        // However, for this reason, we keep sending the signal again and again until we see that the atomic `running` is set to false.
-        #[cfg(mshv2)]
-        let run_result = {
-            let hv_message: hv_message = Default::default();
-            self.vcpu_fd.run(hv_message)
+        // Don't run the vcpu is `cancel_requested` is true
+        //
+        // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+        // Then this is fine since `cancel_requested` is set to true, so we will skip the `VcpuFd::run()` call
+        let exit_reason = if self
+            .interrupt_handle
+            .cancel_requested
+            .swap(false, Ordering::Relaxed)
+        {
+            return Ok(HyperlightExit::Cancelled());
+        } else {
+            // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+            // Then the vcpu will run, but we will keep sending signals to this thread
+            // to interrupt it until `running` is set to false. The `vcpu_fd::run()` call will
+            // return either normally with an exit reason, or from being "kicked" by out signal handler, with an EINTR error,
+            // both of which are fine.
+            #[cfg(mshv2)]
+            {
+                let hv_message: hv_message = Default::default();
+                self.vcpu_fd.run(hv_message)
+            }
+            #[cfg(mshv3)]
+            self.vcpu_fd.run()
         };
-        #[cfg(mshv3)]
-        let run_result = self.vcpu_fd.run();
-        // Note: if a `InterruptHandle::kill()` signal is delivered to this thread **here**
-        // - after we've called `VcpuFd::run()`
-        // - before we've set the running to false
-        // Then this is fine because the call to `VcpuFd::run()` is already finished,
-        // the signal handler itself is a no-op, and the signals will stop being sent
-        // once we've set the `running` to false.
+        // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+        // Then signals will be sent to this thread until `running` is set to false.
+        // This is fine since the signal handler is a no-op.
+        let cancel_requested = self
+            .interrupt_handle
+            .cancel_requested
+            .swap(false, Ordering::Relaxed);
+        // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+        // Then `cancel_requested` will be set to true again, which will cancel the **next vcpu run**.
+        // Additionally signals will be sent to this thread until `running` is set to false.
+        // This is fine since the signal handler is a no-op.
         self.interrupt_handle
             .running
             .store(false, Ordering::Relaxed);
-        // Note: if a `InterruptHandle::kill()` signal is delivered to this thread **here**
-        // - after we've set the running to false,
-        // Then the signal does not have any effect, because the signal handler is a no-op.
-        // This is fine since we are already done with the `VcpuFd::run()` call.
-
-        let result = match run_result {
+        // At this point, `running` is false so no more signals will be sent to this thread,
+        // but we may still receive async signals that were sent before this point.
+        // To prevent those signals from interrupting subsequent calls to `run()`,
+        // we make sure to check `cancel_requested` before cancelling (see `libc::EINTR` match-arm below).
+        let result = match exit_reason {
             Ok(m) => match m.header.message_type {
                 HALT_MESSAGE => {
                     crate::debug!("mshv - Halt Details : {:#?}", &self);
@@ -691,7 +707,15 @@ impl Hypervisor for HypervLinuxDriver {
             },
             Err(e) => match e.errno() {
                 // we send a signal to the thread to cancel execution this results in EINTR being returned by KVM so we return Cancelled
-                libc::EINTR => HyperlightExit::Cancelled(),
+                libc::EINTR => {
+                    // If cancellation was not requested for this specific vm, the vcpu was interrupted because of stale signal
+                    // that was meant to be delivered to a previous/other vcpu on this same thread, so let's ignore it
+                    if !cancel_requested {
+                        HyperlightExit::Retry()
+                    } else {
+                        HyperlightExit::Cancelled()
+                    }
+                }
                 libc::EAGAIN => HyperlightExit::Retry(),
                 _ => {
                     crate::debug!("mshv Error - Details: Error: {} \n {:#?}", e, &self);

src/hyperlight_host/src/hypervisor/kvm.rs

@@ -350,6 +350,7 @@ impl KVMDriver {
             mem_regions,
             interrupt_handle: Arc::new(LinuxInterruptHandle {
                 running: AtomicBool::new(false),
+                cancel_requested: AtomicBool::new(false),
                 tid: AtomicU64::new(unsafe { libc::pthread_self() }),
                 dropped: AtomicBool::new(false),
             }),
@@ -519,29 +520,47 @@ impl Hypervisor for KVMDriver {
         self.interrupt_handle
             .tid
             .store(unsafe { libc::pthread_self() as u64 }, Ordering::Relaxed);
-        // Note: if a `InterruptHandle::kill()` signal is delivered to this thread **here**
-        // - before we've set the running to true,
-        // Then the signal does not have any effect, because the signal handler is a no-op.
+        // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+        // Then this is fine since `cancel_requested` is set to true, so we will skip the `VcpuFd::run()` call
         self.interrupt_handle.running.store(true, Ordering::Relaxed);
-        // Note: if a `InterruptHandle::kill()` signal is delivered to this thread **here**
-        // - after we've set the running to true,
-        // - before we've called `VcpuFd::run()`
-        // Then the individual signal is lost, because the signal is only processed after we've left userspace.
-        // However, for this reason, we keep sending the signal again and again until we see that the atomic `running` is set to false.
-        let exit_reason = self.vcpu_fd.run();
-        // Note: if a `InterruptHandle::kill()` signal is delivered to this thread **here**
-        // - after we've called `VcpuFd::run()`
-        // - before we've set the running to false
-        // Then this is fine because the call to `VcpuFd::run()` is already finished,
-        // the signal handler itself is a no-op, and the signals will stop being sent
-        // once we've set the `running` to false.
+        // Don't run the vcpu is `cancel_requested` is true
+        //
+        // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+        // Then this is fine since `cancel_requested` is set to true, so we will skip the `VcpuFd::run()` call
+        let exit_reason = if self
+            .interrupt_handle
+            .cancel_requested
+            .load(Ordering::Relaxed)
+        {
+            Err(kvm_ioctls::Error::new(libc::EINTR))
+        } else {
+            // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+            // Then the vcpu will run, but we will keep sending signals to this thread
+            // to interrupt it until `running` is set to false. The `vcpu_fd::run()` call will
+            // return either normally with an exit reason, or from being "kicked" by out signal handler, with an EINTR error,
+            // both of which are fine.
+            self.vcpu_fd.run()
+        };
+        // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+        // Then signals will be sent to this thread until `running` is set to false.
+        // This is fine since the signal handler is a no-op.
+        #[allow(unused_variables)]
+        // The variable is only used when `cfg(not(gdb))`, but the flag needs to be reset always anyway
+        let cancel_requested = self
+            .interrupt_handle
+            .cancel_requested
+            .swap(false, Ordering::Relaxed);
+        // Note: if a `InterruptHandle::kill()` called while this thread is **here**
+        // Then `cancel_requested` will be set to true again, which will cancel the **next vcpu run**.
+        // Additionally signals will be sent to this thread until `running` is set to false.
+        // This is fine since the signal handler is a no-op.
         self.interrupt_handle
             .running
             .store(false, Ordering::Relaxed);
-        // Note: if a `InterruptHandle::kill()` signal is delivered to this thread **here**
-        // - after we've set the running to false,
-        // Then the signal does not have any effect, because the signal handler is a no-op.
-        // This is fine since we are already done with the `VcpuFd::run()` call.
+        // At this point, `running` is false so no more signals will be sent to this thread,
+        // but we may still receive async signals that were sent before this point.
+        // To prevent those signals from interrupting subsequent calls to `run()` (on other vms!),
+        // we make sure to check `cancel_requested` before cancelling (see `libc::EINTR` match-arm below).
         let result = match exit_reason {
             Ok(VcpuExit::Hlt) => {
                 crate::debug!("KVM - Halt Details : {:#?}", &self);
@@ -593,7 +612,15 @@ impl Hypervisor for KVMDriver {
                 libc::EINTR => HyperlightExit::Debug(VcpuStopReason::Interrupt),
                 // we send a signal to the thread to cancel execution this results in EINTR being returned by KVM so we return Cancelled
                 #[cfg(not(gdb))]
-                libc::EINTR => HyperlightExit::Cancelled(),
+                libc::EINTR => {
+                    // If cancellation was not requested for this specific vm, the vcpu was interrupted because of stale signal
+                    // that was meant to be delivered to a previous/other vcpu on this same thread, so let's ignore it
+                    if !cancel_requested {
+                        HyperlightExit::Retry()
+                    } else {
+                        HyperlightExit::Cancelled()
+                    }
+                }
                 libc::EAGAIN => HyperlightExit::Retry(),
                 _ => {
                     crate::debug!("KVM Error -Details: Address: {} \n {:#?}", e, &self);

src/hyperlight_host/src/hypervisor/mod.rs

@@ -20,6 +20,8 @@ use tracing::{instrument, Span};
 use crate::error::HyperlightError::ExecutionCanceledByHost;
 use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags};
 use crate::metrics::METRIC_GUEST_CANCELLATION;
+#[cfg(any(kvm, mshv))]
+use crate::signal_handlers::INTERRUPT_VCPU_SIGRTMIN_OFFSET;
 use crate::{log_then_return, new_error, HyperlightError, Result};
 
 /// Util for handling x87 fpu state
@@ -322,10 +324,12 @@ impl VirtualCPU {
 
 /// A trait for handling interrupts to a sandbox's vcpu
 pub trait InterruptHandle: Send + Sync {
-    /// Interrupt the corresponding sandbox's vcpu if it's running.
+    /// Interrupt the corresponding sandbox from running.
     ///
     /// - If this is called while the vcpu is running, then it will interrupt the vcpu and return `true`.
-    /// - If this is called while the vcpu is not running, then it will do nothing and return `false`.
+    /// - If this is called while the vcpu is not running, (for example during a host call), the
+    ///     vcpu will not immediately be interrupted, but will prevent the vcpu from running **the next time**
+    ///     it's scheduled, and returns `false`.
     ///
     /// # Note
     /// This function will block for the duration of the time it takes for the vcpu thread to be interrupted.
@@ -338,25 +342,35 @@ pub trait InterruptHandle: Send + Sync {
 #[cfg(any(kvm, mshv))]
 #[derive(Debug)]
 pub(super) struct LinuxInterruptHandle {
-    /// True when the vcpu is currently running and blocking the thread
+    /// Invariant: vcpu is running => `running` is true. (Neither converse nor inverse is true)
     running: AtomicBool,
-    /// The thread id on which the vcpu was most recently run on or is currently running on
+    /// Invariant: vcpu is running => `tid` is the thread on which it is running.
     tid: AtomicU64,
+    /// True when an "interruptor" has requested the VM to be cancelled. Set immediately when
+    /// `kill()` is called, and cleared when the vcpu is no longer running.
+    /// This is used to
+    /// 1. make sure stale signals do not interrupt the
+    ///     the wrong vcpu (a vcpu may only be interrupted iff `cancel_requested` is true),
+    /// 2. ensure that if a vm is killed while a host call is running,
+    ///     the vm will not re-enter the guest after the host call returns.
+    cancel_requested: AtomicBool,
     /// Whether the corresponding vm is dropped
     dropped: AtomicBool,
 }
 
 #[cfg(any(kvm, mshv))]
 impl InterruptHandle for LinuxInterruptHandle {
     fn kill(&self) -> bool {
-        let sigrtmin = libc::SIGRTMIN();
+        self.cancel_requested.store(true, Ordering::Relaxed);
+
+        let signal_number = libc::SIGRTMIN() + INTERRUPT_VCPU_SIGRTMIN_OFFSET;
         let mut sent_signal = false;
 
         while self.running.load(Ordering::Relaxed) {
             log::info!("Sending signal to kill vcpu thread...");
             sent_signal = true;
             unsafe {
-                libc::pthread_kill(self.tid.load(Ordering::Relaxed) as _, sigrtmin);
+                libc::pthread_kill(self.tid.load(Ordering::Relaxed) as _, signal_number);
             }
             std::thread::sleep(std::time::Duration::from_micros(50));
         }

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

@@ -295,12 +295,10 @@ where
 
 #[cfg(test)]
 mod tests {
-    use std::thread;
-
     use hyperlight_common::flatbuffer_wrappers::function_types::{
         ParameterValue, ReturnType, ReturnValue,
     };
-    use hyperlight_testing::{callback_guest_as_string, simple_guest_as_string};
+    use hyperlight_testing::simple_guest_as_string;
 
     use crate::func::call_ctx::MultiUseGuestCallContext;
     use crate::sandbox::SandboxConfiguration;
@@ -465,54 +463,6 @@ mod tests {
         Ok(())
     }
 
-    // This test is to capture the case where the guest execution is running a host function when cancelled and that host function
-    // is never going to return.
-    // The host function that is called will end after 5 seconds, but by this time the cancellation will have given up
-    // (using default timeout settings)  , so this tests looks for the error "Failed to cancel guest execution".
-    #[test]
-    #[ignore = "We cannot cancel host functions. TODO reenable this test when it's enabled"]
-    fn test_terminate_vcpu_calling_host_spinning_cpu() {
-        // This test relies upon a Hypervisor being present so for now
-        // we will skip it if there isn't one.
-        if !is_hypervisor_present() {
-            println!("Skipping test_call_guest_function_by_name because no hypervisor is present");
-            return;
-        }
-        let mut usbox = UninitializedSandbox::new(
-            GuestBinary::FilePath(callback_guest_as_string().expect("Guest Binary Missing")),
-            None,
-        )
-        .unwrap();
-
-        // Make this host call run for 5 seconds
-
-        fn spin() -> Result<()> {
-            thread::sleep(std::time::Duration::from_secs(5));
-            Ok(())
-        }
-
-        #[cfg(any(target_os = "windows", not(feature = "seccomp")))]
-        usbox.register("Spin", spin).unwrap();
-
-        #[cfg(all(target_os = "linux", feature = "seccomp"))]
-        usbox
-            .register_with_extra_allowed_syscalls("Spin", spin, vec![libc::SYS_clock_nanosleep])
-            .unwrap();
-
-        let sandbox: MultiUseSandbox = usbox.evolve(Noop::default()).unwrap();
-        let mut ctx = sandbox.new_call_context();
-        let result = ctx.call("CallHostSpin", ReturnType::Void, None);
-
-        assert!(result.is_err());
-        match result.unwrap_err() {
-            HyperlightError::GuestExecutionHungOnHostFunctionCall() => {}
-            e => panic!(
-                "Expected HyperlightError::GuestExecutionHungOnHostFunctionCall but got {:?}",
-                e
-            ),
-        }
-    }
-
     #[test]
     fn test_trigger_exception_on_guest() {
         let usbox = UninitializedSandbox::new(

src/hyperlight_host/src/signal_handlers/mod.rs

@@ -17,6 +17,8 @@ limitations under the License.
 #[cfg(feature = "seccomp")]
 pub mod sigsys_signal_handler;
 
+pub(crate) const INTERRUPT_VCPU_SIGRTMIN_OFFSET: i32 = 0;
+
 pub(crate) fn setup_signal_handlers() -> crate::Result<()> {
     // This is unsafe because signal handlers only allow a very restrictive set of
     // functions (i.e., async-signal-safe functions) to be executed inside them.
@@ -45,7 +47,10 @@ pub(crate) fn setup_signal_handlers() -> crate::Result<()> {
             original_hook(panic_info);
         }));
     }
-    vmm_sys_util::signal::register_signal_handler(libc::SIGRTMIN(), vm_kill_signal)?;
+    vmm_sys_util::signal::register_signal_handler(
+        libc::SIGRTMIN() + INTERRUPT_VCPU_SIGRTMIN_OFFSET,
+        vm_kill_signal,
+    )?;
 
     // Note: For libraries registering signal handlers, it's important to keep in mind that
     // the user of the library could have their own signal handlers that we don't want to