diff --git a/crates/runtime/src/externref.rs b/crates/runtime/src/externref.rs index dba09a8014..92d7105a8a 100644 --- a/crates/runtime/src/externref.rs +++ b/crates/runtime/src/externref.rs @@ -499,6 +499,14 @@ pub struct VMExternRefActivationsTable { /// is just part of this struct so that we can reuse the allocation, rather /// than create a new hash set every GC. precise_stack_roots: RefCell>>, + + /// A pointer to a `u8` on the youngest host stack frame before we called + /// into Wasm for the first time. When walking the stack in garbage + /// collection, if we don't find this frame, then we failed to walk every + /// Wasm stack frame, which means we failed to find all on-stack, + /// inside-a-Wasm-frame roots, and doing a GC could lead to freeing one of + /// those missed roots, and use after free. + stack_canary: Cell>>, } impl VMExternRefActivationsTable { @@ -515,6 +523,7 @@ impl VMExternRefActivationsTable { end: Cell::new(NonNull::new(end).unwrap()), chunks: RefCell::new(vec![chunk]), precise_stack_roots: RefCell::new(HashSet::with_capacity(Self::INITIAL_CHUNK_SIZE)), + stack_canary: Cell::new(None), } } @@ -710,6 +719,73 @@ impl VMExternRefActivationsTable { } } } + + /// Set the stack canary around a call into Wasm. + /// + /// The return value should not be dropped until after the Wasm call has + /// returned. + /// + /// While this method is always safe to call (or not call), it is unsafe to + /// call the `wasmtime_runtime::gc` function unless this method is called at + /// the proper times and its return value properly outlives its Wasm call. + /// + /// For `gc` to be safe, this is only *strictly required* to surround the + /// oldest host-->Wasm stack frame transition on this thread, but repeatedly + /// calling it is idempotent and cheap, so it is recommended to call this + /// for every host-->Wasm call. + /// + /// # Example + /// + /// ```no_run + /// use wasmtime_runtime::*; + /// + /// #let get_table_from_somewhere = || unimplemented!(); + /// let table: &VMExternRefActivationsTable = get_table_from_somewhere(); + /// + /// // Set the canary before a Wasm call. The canary should always be a + /// // local on the stack. + /// let canary = 0; + /// let auto_reset_canary = table.set_stack_canary(&canary); + /// + /// // Do the call into Wasm. + /// #let call_into_wasm = || unimplemented!(); + /// call_into_wasm(); + /// + /// // Only drop the value returned by `set_stack_canary` after the Wasm + /// // call has returned. + /// drop(auto_reset_canary); + /// ``` + pub fn set_stack_canary<'a>(&'a self, canary: &u8) -> impl Drop + 'a { + let should_reset = if self.stack_canary.get().is_none() { + let canary = canary as *const u8 as *mut u8; + self.stack_canary.set(Some(unsafe { + debug_assert!(!canary.is_null()); + NonNull::new_unchecked(canary) + })); + true + } else { + false + }; + + return AutoResetCanary { + table: self, + should_reset, + }; + + struct AutoResetCanary<'a> { + table: &'a VMExternRefActivationsTable, + should_reset: bool, + } + + impl Drop for AutoResetCanary<'_> { + fn drop(&mut self) { + if self.should_reset { + debug_assert!(self.table.stack_canary.get().is_some()); + self.table.stack_canary.set(None); + } + } + } + } } /// A registry of stack maps for currently active Wasm modules. @@ -954,7 +1030,14 @@ impl std::ops::DerefMut for DebugOnly { } /// Perform garbage collection of `VMExternRef`s. -pub fn gc( +/// +/// # Unsafety +/// +/// You must have called `VMExternRefActivationsTable::set_stack_canary` for at +/// least the oldest host-->Wasm stack frame transition on this thread's stack +/// (it is idempotent to call it more than once) and keep its return value alive +/// across the duration of that host-->Wasm call. +pub unsafe fn gc( stack_maps_registry: &StackMapRegistry, externref_activations_table: &VMExternRefActivationsTable, ) { @@ -963,11 +1046,57 @@ pub fn gc( debug_assert!({ // This set is only non-empty within this function. It is built up when // walking the stack and interpreting stack maps, and then drained back - // into the activations table's bump-allocated space at the end. + // into the activations table's bump-allocated space at the + // end. Therefore, it should always be empty upon entering this + // function. let precise_stack_roots = externref_activations_table.precise_stack_roots.borrow(); precise_stack_roots.is_empty() }); + // Whenever we call into Wasm from host code for the first time, we set a + // stack canary. When we return to that host code, we unset the stack + // canary. If there is *not* a stack canary, then there must be zero Wasm + // frames on the stack. Therefore, we can simply reset the table without + // walking the stack. + let stack_canary = match externref_activations_table.stack_canary.get() { + None => { + if cfg!(debug_assertions) { + // Assert that there aren't any Wasm frames on the stack. + backtrace::trace(|frame| { + let stack_map = stack_maps_registry.lookup_stack_map(frame.ip() as usize); + assert!(stack_map.is_none()); + true + }); + } + externref_activations_table.reset(); + log::debug!("end GC"); + return; + } + Some(canary) => canary.as_ptr() as usize, + }; + + // There is a stack canary, so there must be Wasm frames on the stack. The + // rest of this function consists of: + // + // * walking the stack, + // + // * finding the precise set of roots inside Wasm frames via our stack maps, + // and + // + // * resetting our bump-allocated table's over-approximation to the + // newly-discovered precise set. + + // The SP of the previous frame we processed. + let mut last_sp = None; + + // Whether we have found our stack canary or not yet. + let mut found_canary = false; + + // The `activations_table_set` is used for `debug_assert!`s checking that + // every reference we read out from the stack via stack maps is actually in + // the table. If that weren't true, than either we forgot to insert a + // reference in the table when passing it into Wasm (a bug) or we are + // reading invalid references from the stack (another bug). let mut activations_table_set: DebugOnly> = Default::default(); if cfg!(debug_assertions) { externref_activations_table.elements(|elem| { @@ -977,20 +1106,18 @@ pub fn gc( backtrace::trace(|frame| { let pc = frame.ip() as usize; + let sp = frame.sp() as usize; if let Some(stack_map) = stack_maps_registry.lookup_stack_map(pc) { - let ptr_to_frame = frame.sp() as usize; - for i in 0..(stack_map.mapped_words() as usize) { if stack_map.get_bit(i) { // Stack maps have one bit per word in the frame, and the // zero^th bit is the *lowest* addressed word in the frame, // i.e. the closest to the SP. So to get the `i`^th word in - // this frame, we add `i * sizeof(word)` to the - // lowest-addressed word within this frame. - let ptr_to_ref = ptr_to_frame + i * mem::size_of::(); + // this frame, we add `i * sizeof(word)` to the SP. + let ptr_to_ref = sp + i * mem::size_of::(); - let r = unsafe { std::ptr::read(ptr_to_ref as *const *mut VMExternData) }; + let r = std::ptr::read(ptr_to_ref as *const *mut VMExternData); debug_assert!( r.is_null() || activations_table_set.contains(&r), "every on-stack externref inside a Wasm frame should \ @@ -1003,11 +1130,32 @@ pub fn gc( } } - // Keep walking the stack. - true + if let Some(last_sp) = last_sp { + // We've found the stack canary when we walk over the frame that it + // is contained within. + found_canary |= last_sp <= stack_canary && stack_canary <= sp; + } + last_sp = Some(sp); + + // Keep walking the stack until we've found the canary, which is the + // oldest frame before we ever called into Wasm. We can stop once we've + // found it because there won't be any more Wasm frames, and therefore + // there won't be anymore on-stack, inside-a-Wasm-frame roots. + !found_canary }); - externref_activations_table.reset(); + // Only reset the table if we found the stack canary, and therefore know + // that we discovered all the on-stack, inside-a-Wasm-frame roots. If we did + // *not* find the stack canary, then `libunwind` failed to walk the whole + // stack, and we might be missing roots. Reseting the table would free those + // missing roots while they are still in use, leading to use-after-free. + if found_canary { + externref_activations_table.reset(); + } else { + let mut roots = externref_activations_table.precise_stack_roots.borrow_mut(); + roots.clear(); + } + log::debug!("end GC"); } diff --git a/crates/wasmtime/src/func.rs b/crates/wasmtime/src/func.rs index c8551d2aad..9fb58c743d 100644 --- a/crates/wasmtime/src/func.rs +++ b/crates/wasmtime/src/func.rs @@ -194,9 +194,17 @@ macro_rules! getters { >(export.address); let mut ret = None; $(let $args = $args.into_abi();)* - catch_traps(export.vmctx, &instance.store, || { - ret = Some(fnptr(export.vmctx, ptr::null_mut(), $($args,)*)); - })?; + + { + let canary = 0; + let _auto_reset = instance + .store + .externref_activations_table() + .set_stack_canary(&canary); + catch_traps(export.vmctx, &instance.store, || { + ret = Some(fnptr(export.vmctx, ptr::null_mut(), $($args,)*)); + })?; + } Ok(ret.unwrap()) } @@ -552,14 +560,23 @@ impl Func { } // Call the trampoline. - catch_traps(self.export.vmctx, &self.instance.store, || unsafe { - (self.trampoline)( - self.export.vmctx, - ptr::null_mut(), - self.export.address, - values_vec.as_mut_ptr(), - ) - })?; + { + let canary = 0; + let _auto_reset = self + .instance + .store + .externref_activations_table() + .set_stack_canary(&canary); + + catch_traps(self.export.vmctx, &self.instance.store, || unsafe { + (self.trampoline)( + self.export.vmctx, + ptr::null_mut(), + self.export.address, + values_vec.as_mut_ptr(), + ) + })?; + } // Load the return values out of `values_vec`. let mut results = Vec::with_capacity(my_ty.results().len()); diff --git a/crates/wasmtime/src/runtime.rs b/crates/wasmtime/src/runtime.rs index 31bf83bcc1..6e3f95b5ff 100644 --- a/crates/wasmtime/src/runtime.rs +++ b/crates/wasmtime/src/runtime.rs @@ -1097,10 +1097,14 @@ impl Store { /// Perform garbage collection of `ExternRef`s. pub fn gc(&self) { - wasmtime_runtime::gc( - &*self.inner.stack_map_registry, - &*self.inner.externref_activations_table, - ); + // For this crate's API, we ensure that `set_stack_canary` invariants + // are upheld for all host-->Wasm calls. + unsafe { + wasmtime_runtime::gc( + &*self.inner.stack_map_registry, + &*self.inner.externref_activations_table, + ); + } } }