test and support forced unwinding of guests stopped from stack overflows

Author: awortman-fastly

lucet-runtime/lucet-runtime-internals/src/alloc/mod.rs

@@ -82,6 +82,8 @@ pub struct Slot {
     pub limits: Limits,
 
     pub region: Weak<dyn RegionInternal>,
+
+    pub(crate) redzone_stack_enabled: bool,
 }
 
 // raw pointers require unsafe impl
@@ -92,6 +94,14 @@ impl Slot {
     pub fn stack_top(&self) -> *mut c_void {
         (self.stack as usize + self.limits.stack_size) as *mut c_void
     }
+
+    pub fn stack_redzone_start(&self) -> *mut c_void {
+        (self.stack as usize - self.stack_redzone_size()) as *mut c_void
+    }
+
+    pub fn stack_redzone_size(&self) -> usize {
+        host_page_size()
+    }
 }
 
 /// The structure that manages the allocations backing an `Instance`.
@@ -113,6 +123,24 @@ impl Drop for Alloc {
 }
 
 impl Alloc {
+    pub(crate) fn enable_stack_redzone(&mut self) {
+        let slot = self
+            .slot
+            .as_mut()
+            .expect("alloc has a Slot when toggling stack redzone");
+        slot.redzone_stack_enabled = true;
+        self.region.enable_stack_redzone(slot)
+    }
+
+    pub(crate) fn disable_stack_redzone(&mut self) {
+        let slot = self
+            .slot
+            .as_mut()
+            .expect("alloc has a Slot when toggling stack redzone");
+        slot.redzone_stack_enabled = false;
+        self.region.disable_stack_redzone(slot)
+    }
+
     pub fn addr_in_heap_guard(&self, addr: *const c_void) -> bool {
         let heap = self.slot().heap as usize;
         let guard_start = heap + self.heap_accessible_size;
@@ -261,13 +289,41 @@ impl Alloc {
         std::slice::from_raw_parts_mut(self.slot().heap as *mut u64, self.heap_accessible_size / 8)
     }
 
+    pub(crate) fn stack_start(&self) -> *mut u8 {
+        let mut stack_start = self.slot().stack as usize;
+
+        if self
+            .slot
+            .as_ref()
+            .expect("alloc has a slot when we want to access its stack")
+            .redzone_stack_enabled
+        {
+            stack_start -= host_page_size();
+        }
+
+        stack_start as *mut u8
+    }
+
+    pub(crate) fn stack_size(&self) -> usize {
+        let mut stack_size = self.slot().limits.stack_size;
+        if self
+            .slot
+            .as_ref()
+            .expect("alloc has a slot when we want to access its stack")
+            .redzone_stack_enabled
+        {
+            stack_size += host_page_size();
+        }
+        stack_size
+    }
+
     /// Return the stack as a mutable byte slice.
     ///
     /// Since the stack grows down, `alloc.stack_mut()[0]` is the top of the stack, and
     /// `alloc.stack_mut()[alloc.limits.stack_size - 1]` is the last byte at the bottom of the
     /// stack.
     pub unsafe fn stack_mut(&mut self) -> &mut [u8] {
-        std::slice::from_raw_parts_mut(self.slot().stack as *mut u8, self.slot().limits.stack_size)
+        std::slice::from_raw_parts_mut(self.stack_start(), self.stack_size())
     }
 
     /// Return the stack as a mutable slice of 64-bit words.
@@ -276,18 +332,12 @@ impl Alloc {
     /// `alloc.stack_mut()[alloc.limits.stack_size - 1]` is the last word at the bottom of the
     /// stack.
     pub unsafe fn stack_u64_mut(&mut self) -> &mut [u64] {
-        assert!(
-            self.slot().stack as usize % 8 == 0,
-            "stack is 8-byte aligned"
-        );
-        assert!(
-            self.slot().limits.stack_size % 8 == 0,
-            "stack size is multiple of 8-bytes"
-        );
-        std::slice::from_raw_parts_mut(
-            self.slot().stack as *mut u64,
-            self.slot().limits.stack_size / 8,
-        )
+        let stack_start = self.stack_start();
+        let stack_size = self.stack_size();
+
+        assert!(stack_start as usize % 8 == 0, "stack is 8-byte aligned");
+        assert!(stack_size % 8 == 0, "stack size is multiple of 8-bytes");
+        std::slice::from_raw_parts_mut(stack_start as *mut u64, stack_size / 8)
     }
 
     /// Return the globals as a slice.

lucet-runtime/lucet-runtime-internals/src/context/mod.rs

@@ -115,6 +115,7 @@ pub struct Context {
     retvals_gp: [u64; 2],
     retval_fp: __m128,
     sigset: signal::SigSet,
+    pub(crate) stop_addr: Option<u64>,
 }
 
 impl Context {
@@ -126,6 +127,7 @@ impl Context {
             retvals_gp: [0; 2],
             retval_fp: unsafe { _mm_setzero_ps() },
             sigset: signal::SigSet::empty(),
+            stop_addr: None,
         }
     }
 }

lucet-runtime/lucet-runtime-internals/src/instance.rs

@@ -949,63 +949,119 @@ impl Instance {
         Ok(())
     }
 
-    fn push(&mut self, value: u64) {
-        let stack_offset = self.ctx.gpr.rsp as usize - self.alloc.slot().stack as usize;
+    fn push(&mut self, value: u64) -> Result<(), ()> {
+        let stack_offset = self.ctx.gpr.rsp as usize - self.alloc.stack_start() as usize;
         let stack_index = stack_offset / 8;
         assert!(stack_offset % 8 == 0);
 
         let stack = unsafe { self.alloc.stack_u64_mut() };
 
         // check for at least one free stack slot
-        if stack.len() - stack_index >= 1 {
+        if stack_index >= 1 {
             self.ctx.gpr.rsp -= 8;
             stack[stack_index - 1] = value;
+            Ok(())
         } else {
-            panic!("caused a guest stack overflow!");
+            Err(())
         }
     }
 
+    fn with_redzone_stack<T, F: FnOnce(&mut Self) -> T>(&mut self, f: F) -> T {
+        self.alloc.enable_stack_redzone();
+
+        let res = f(self);
+
+        self.alloc.disable_stack_redzone();
+
+        res
+    }
+
+    // Force a guest to unwind the stack from the specified guest address
     fn force_unwind(&mut self) -> Result<(), Error> {
-        #[unwind(allowed)]
-        extern "C" fn initiate_unwind() {
-            panic!(TerminationDetails::ForcedUnwind);
-        }
+        // if we should unwind by returning into the guest to cause a fault, do so with the redzone
+        // available in case the guest was at or close to overflowing.
+        self.with_redzone_stack(|inst| {
+            #[unwind(allowed)]
+            extern "C" fn initiate_unwind() {
+                panic!(TerminationDetails::ForcedUnwind);
+            }
 
-        // The logic for this conditional can be a bit unintuitive: we _require_ that the stack
-        // is aligned to 8 bytes, but not 16 bytes, when pushing `initiate_unwind`.
-        //
-        // A diagram of the required layout may help:
-        // `XXXXX0`: ------------------ <-- call frame start -- SysV ABI requires 16-byte alignment
-        // `XXXXX8`: | return address |
-        // `XXXX..`: | ..locals etc.. |
-        // `XXXX..`: | ..as needed... |
-        //
-        // By the time we've gotten here, we have already pushed "return address", the address of
-        // wherever in the guest we want to start unwinding. If it leaves the stack 16-byte
-        // aligned, it's 8 bytes off from the diagram above, and we would have the call frame for
-        // `initiate_unwind` in violation of the SysV ABI. Functionally, this means that
-        // compiler-generated xmm accesses will fault due to being misaligned.
-        //
-        // So, instead, push a new return address to construct a new call frame at the right
-        // offset. `unwind_stub` has CFA directives so the unwinder can connect from
-        // `initiate_unwind` to guest/host frames to unwind. The unwinder, thankfully, has no
-        // preferences about stack alignment of frames being unwound.
-        //
-        // extremely unsafe, doesn't handle any stack exhaustion edge cases yet
-        if self.ctx.gpr.rsp % 16 == 0 {
-            self.push(crate::context::unwind_stub as u64);
-        }
+            let guest_addr = inst
+                .ctx
+                .stop_addr
+                .expect("guest that stopped in guest code has an address it stopped at");
+
+            // set up the faulting instruction pointer as the return address for `initiate_unwind`;
+            // extremely unsafe, doesn't handle any edge cases yet
+            //
+            // TODO(Andy) if the last address is obtained through the signal handler, for a signal
+            // received exactly when we have just executed a `call` to a guest function, we
+            // actually want to not push it (or push it +1?) lest we try to unwind with a return
+            // address == start of function, where the system unwinder will unwind for the function
+            // at address-1, (probably) fail to find the function, and `abort()`.
+            //
+            // if `rip` == the start of some guest function, we can probably just discard it and
+            // use the return address instead.
+            inst.push(guest_addr as u64)
+                .expect("stack has available space");
+
+            // The logic for this conditional can be a bit unintuitive: we _require_ that the stack
+            // is aligned to 8 bytes, but not 16 bytes, when pushing `initiate_unwind`.
+            //
+            // A diagram of the required layout may help:
+            // `XXXXX0`: ------------------ <-- call frame start -- SysV ABI requires 16-byte alignment
+            // `XXXXX8`: | return address |
+            // `XXXX..`: | ..locals etc.. |
+            // `XXXX..`: | ..as needed... |
+            //
+            // Now ensure we _have_ an ABI-conformant call fame like above, by handling the case that
+            // could lead to an unaligned stack - the guest stack pointer currently being unaligned.
+            // Among other errors, a misaligned stack will result in compiler-generated xmm accesses to
+            // fault.
+            //
+            // Eg, we would have a stack like:
+            // `XXXXX8`: ------------------ <-- guest stack end, call frame start
+            // `XXXXX0`: |   unwind_stub  |
+            // `XXXX..`: | ..locals etc.. |
+            // `XXXX..`: | ..as needed... |
+            //
+            // So, instead, push a new return address to construct a new call frame at the right
+            // offset. `unwind_stub` has CFA directives so the unwinder can connect from
+            // `initiate_unwind` to guest/host frames to unwind. The unwinder, thankfully, has no
+            // preferences about alignment of frames being unwound.
+            //
+            // And we end up with a guest stack like this:
+            // `XXXXX8`: ------------------ <-- guest stack end
+            // `XXXXX0`: | guest ret addr | <-- guest return address to unwind through
+            // `XXXXX0`: ------------------ <-- call frame start -- SysV ABI requires 16-byte alignment
+            // `XXXXX8`: |   unwind_stub  |
+            // `XXXX..`: | ..locals etc.. |
+            // `XXXX..`: | ..as needed... |
+            if inst.ctx.gpr.rsp % 16 == 0 {
+                // extremely unsafe, doesn't handle any stack exhaustion edge cases yet
+                inst.push(crate::context::unwind_stub as u64)
+                    .expect("stack has available space");
+            }
 
-        assert!(self.ctx.gpr.rsp % 16 == 8);
-        self.push(initiate_unwind as u64);
+            assert!(inst.ctx.gpr.rsp % 16 == 8);
+            // extremely unsafe, doesn't handle any stack exhaustion edge cases yet
+            inst.push(initiate_unwind as u64)
+                .expect("stack has available space");
 
-        match self.swap_and_return() {
-            Ok(_) => panic!("forced unwinding shouldn't return normally"),
-            Err(Error::RuntimeTerminated(TerminationDetails::ForcedUnwind)) => (),
-            Err(e) => panic!("unexpected error: {}", e),
-        }
+            inst.state = State::Ready;
 
-        Ok(())
+            match inst.swap_and_return() {
+                Ok(_) => panic!("forced unwinding shouldn't return normally"),
+                Err(Error::RuntimeTerminated(TerminationDetails::ForcedUnwind)) => (),
+                Err(e) => panic!("unexpected error: {}", e),
+            }
+
+            // we've unwound the stack, so we know there are no longer any host frames.
+            inst.hostcall_count = 0;
+            inst.ctx.stop_addr = None;
+
+            Ok(())
+        })
     }
 }
 

lucet-runtime/lucet-runtime-internals/src/instance/signals.rs

@@ -226,24 +226,7 @@ extern "C" fn handle_signal(signum: c_int, siginfo_ptr: *mut siginfo_t, ucontext
             // `Context::swap()` here because then we'd swap back to the signal handler instead of
             // the point in the guest that caused the fault
             ctx.save_to_context(&mut inst.ctx);
-
-            // set up the faulting instruction pointer as the return address for `initiate_unwind`;
-            // extremely unsafe, doesn't handle any edge cases yet
-            //
-            // TODO(Andy) can we avoid pushing onto the guest stack until knowing we want to force
-            // an unwind? maybe a "last address" field on ctx. This can be populated on context
-            // swap out (return address of lucet_context_swap) as well as here in the signal
-            // handler.
-            //
-            // TODO(Andy) if the last address is obtained through the signal handler, for a signal
-            // received exactly when we have just executed a `call` to a guest function, we
-            // actually want to not push it (or push it +1?) lest we try to unwind with a return
-            // address == start of function, where the system unwinder will unwind for the function
-            // at address-1, (probably) fail to find the function, and `abort()`.
-            //
-            // if `rip` == the start of some guest function, we can probably just discard it and
-            // use the return address instead.
-            inst.push(rip as u64);
+            inst.ctx.stop_addr = Some(rip as u64);
         }
         switch_to_host
     });

lucet-runtime/lucet-runtime-internals/src/region/mmap.rs

@@ -246,6 +246,28 @@ impl RegionInternal for MmapRegion {
     fn as_dyn_internal(&self) -> &dyn RegionInternal {
         self
     }
+
+    fn enable_stack_redzone(&self, slot: &Slot) {
+        unsafe {
+            mprotect(
+                slot.stack_redzone_start(),
+                host_page_size(),
+                ProtFlags::PROT_READ | ProtFlags::PROT_WRITE,
+            )
+            .expect("can set permissions on stack redzone page")
+        }
+    }
+
+    fn disable_stack_redzone(&self, slot: &Slot) {
+        unsafe {
+            mprotect(
+                slot.stack_redzone_start(),
+                host_page_size(),
+                ProtFlags::PROT_NONE,
+            )
+            .expect("can set permissions on stack redzone page")
+        }
+    }
 }
 
 impl Drop for MmapRegion {
@@ -329,6 +351,7 @@ impl MmapRegion {
             sigstack: sigstack as *mut c_void,
             limits: region.limits.clone(),
             region: Arc::downgrade(region) as Weak<dyn RegionInternal>,
+            redzone_stack_enabled: false,
         })
     }
 

lucet-runtime/lucet-runtime-internals/src/region/mod.rs

@@ -52,6 +52,10 @@ pub trait RegionInternal: Send + Sync {
     fn reset_heap(&self, alloc: &mut Alloc, module: &dyn Module) -> Result<(), Error>;
 
     fn as_dyn_internal(&self) -> &dyn RegionInternal;
+
+    fn enable_stack_redzone(&self, slot: &Slot);
+
+    fn disable_stack_redzone(&self, slot: &Slot);
 }
 
 /// A trait for regions that are created with a fixed capacity and limits.

lucet-runtime/lucet-runtime-tests/guests/host/bindings.json

@@ -1,6 +1,7 @@
 {
     "env": {
-        "fault_unwind": "hostcall_fault_unwind",
+        "bad_access_unwind": "hostcall_bad_access_unwind",
+        "stack_overflow_unwind": "hostcall_stack_overflow_unwind",
         "hostcall_test_func_hello": "hostcall_test_func_hello",
         "hostcall_test_func_hostcall_error": "hostcall_test_func_hostcall_error",
         "hostcall_test_func_hostcall_error_unwind": "hostcall_test_func_hostcall_error_unwind",