Vulnerability: Use After Free in lucet ### Impact
There is a bug in the main branch of Lucet's `lucet-runtime` that allows a use-after-free in an `Instance` object that could result in memory corruption, data race, or other related issues.  This bug was introduced early in the development of Lucet and is present in all releases.  As a result of this bug, and dependent on the memory backing for the `Instance` objects, it is possible to trigger a use-after-free when the `Instance` is dropped.

### Patches
Users should upgrade to the `main` branch of the Lucet repository. Lucet does not provide versioned releases on crates.io.

### Workarounds
There is no way to remediate this vulnerability without upgrading.

### Description
Lucet uses a "pool" allocator for new WebAssembly instances that are created. This pool allocator manages everything from the linear memory of the wasm instance, the runtime stack for async switching, as well as the memory behind the Instance itself. `Instances` are referred to via an `InstanceHandle` type which will, on drop, release the memory backing the Instance back to the pool.

When an Instance is dropped, the fields of the `Instance` are destructed top-to-bottom, however when the `alloc: Alloc` field is destructed, the memory backing the `Instance` is released back to the pool before the destructors of the remaining fields are run. If another thread allocates the same memory from the pool while these destructors are still running, a race condition occurs that can lead to use-after-free errors.

The bug was corrected by changing how the `InstanceHandle` destructor operates to ensure that the memory backing an Instance is only returned to the pool once the `Instance` has been completely destroyed.

This security advisory has been assigned CVE-2021-43790.

### For more information
If you have any questions or comments about this advisory:
* Open an issue in [lucet repository](https://github.com/bytecodealliance/lucet)
* Email [the lucet team](mailto:lucet@fastly.com)
* See the [Bytecode Alliance security policy](https://bytecodealliance.org/security)
Vulnerability ID: GHSA-hf79-8hjp-rrvq
CWE: CWE-416
Severity score: 8.5
Project: https://github.com/bytecodealliance/lucet
Published: 2021-11-30
File: lucet-runtime/lucet-runtime-internals/src/instance.rs
Commit message: Merge pull request from GHSA-hf79-8hjp-rrvq

* Use manual drop

* Add some comments to `ManuallyDrop` usage

* rustfmt

Co-authored-by: Aaron Turon <aturon@fastly.com>
Co-authored-by: Alex Crichton <alex@alexcrichton.com>
Commit author: Pat Hickey

Vulnerable version (before fix):
 use crate::error::Error; #[cfg(feature = "concurrent_testpoints")] use crate::lock_testpoints::LockTestpoints; use crate::module::{self, FunctionHandle, Global, GlobalValue, Module, TrapCode}; use crate::region::RegionInternal; use crate::sysdeps::HOST_PAGE_SIZE_EXPECTED; use crate::val::{UntypedRetVal, Val}; use crate::vmctx::Vmctx; use std::cell::{BorrowError, BorrowMutError, Ref, RefCell, RefMut, UnsafeCell}; use std::convert::TryFrom; use std::marker::PhantomData; use std::mem; use std::ops::{Deref, DerefMut}; use std::ptr::{self, NonNull}; use std::sync::Arc; impl Drop for InstanceHandle {             unsafe {                 let inst = self.inst.as_mut();                  // Grab a handle to the region to ensure it outlives `inst`.                 //                 // This ensures that the region won't be dropped by `inst` being                 // dropped, which could result in `inst` being unmapped by the                 // Region *during* drop of the Instance's fields.                 let region: Arc<dyn RegionInternal> = inst.alloc().region.clone();                  // drop the actual instance                 std::ptr::drop_in_place(inst);                  // and now we can drop what may be the last Arc<Region>. If it is                 // it can safely do what it needs with memory; we're not running                 // destructors on it anymore.                 mem::drop(region);             }         }     } pub struct Instance {     /// Conditionally-present helpers to force permutations of possible races in testing.     pub lock_testpoints: Arc<LockTestpoints>,      /// The memory allocated for this instance     alloc: Alloc,      /// Handler run for signals that do not arise from a known WebAssembly trap, or that involve     /// memory outside of the current instance. impl Instance {             kill_state,             #[cfg(feature = "concurrent_testpoints")]             lock_testpoints,             alloc,             fatal_handler: default_fatal_handler,             c_fatal_handler: None,             signal_handler: Box::new(signal_handler_none) as Box<SignalHandler>,

Fixed version (after fix):
 use crate::error::Error; #[cfg(feature = "concurrent_testpoints")] use crate::lock_testpoints::LockTestpoints; use crate::module::{self, FunctionHandle, Global, GlobalValue, Module, TrapCode}; use crate::sysdeps::HOST_PAGE_SIZE_EXPECTED; use crate::val::{UntypedRetVal, Val}; use crate::vmctx::Vmctx; use std::cell::{BorrowError, BorrowMutError, Ref, RefCell, RefMut, UnsafeCell}; use std::convert::TryFrom; use std::marker::PhantomData; use std::mem; use std::mem::ManuallyDrop; use std::ops::{Deref, DerefMut}; use std::ptr::{self, NonNull}; use std::sync::Arc; impl Drop for InstanceHandle {             unsafe {                 let inst = self.inst.as_mut();                  // The `inst.alloc` field manages the memory of the instance                 // itself. Note, though, that this field is in a `ManuallyDrop`                 // so it won't get dropped automatically in `drop_in_place`.                 // This is the point where we take over that precise drop.                 //                 // By using `take` here we're basically calling `ptr::read`                 // which "duplicates" the `alloc` since the `alloc` local                 // variable here is the exact same as `inst.alloc`. All we do                 // with `inst`, though, is call `drop_in_place`, which                 // invalidates every other field in `inst`.                 let alloc: Alloc = ManuallyDrop::take(&mut inst.alloc);                  // drop the actual instance                 std::ptr::drop_in_place(inst);                  // Now that we're 100% done with the instance, destructors and                 // all, we can release the memory of the instance back to the                 // original allocator from whence it came (be it mmap or uffd                 // based). This will run the "official" destructor for `Alloc`                 // which internally does the release. Note that after this                 // operation the `inst` pointer is invalid and can no longer be                 // used.                 drop(alloc);             }         }     } pub struct Instance {     /// Conditionally-present helpers to force permutations of possible races in testing.     pub lock_testpoints: Arc<LockTestpoints>,      /// The memory allocated for this instance.     ///     /// Note that this is in a `ManuallyDrop` because this manages the memory of     /// this `Instance` itself. To have precise control over this memory we     /// handle this in `Drop for InstanceHandle`.     alloc: ManuallyDrop<Alloc>,      /// Handler run for signals that do not arise from a known WebAssembly trap, or that involve     /// memory outside of the current instance. impl Instance {             kill_state,             #[cfg(feature = "concurrent_testpoints")]             lock_testpoints,             alloc: ManuallyDrop::new(alloc),             fatal_handler: default_fatal_handler,             c_fatal_handler: None,             signal_handler: Box::new(signal_handler_none) as Box<SignalHandler>,

Patch:
patch: @@ -14,7 +14,6 @@ use crate::error::Error;
 #[cfg(feature = "concurrent_testpoints")]
 use crate::lock_testpoints::LockTestpoints;
 use crate::module::{self, FunctionHandle, Global, GlobalValue, Module, TrapCode};
-use crate::region::RegionInternal;
 use crate::sysdeps::HOST_PAGE_SIZE_EXPECTED;
 use crate::val::{UntypedRetVal, Val};
 use crate::vmctx::Vmctx;
@@ -27,6 +26,7 @@ use std::cell::{BorrowError, BorrowMutError, Ref, RefCell, RefMut, UnsafeCell};
 use std::convert::TryFrom;
 use std::marker::PhantomData;
 use std::mem;
+use std::mem::ManuallyDrop;
 use std::ops::{Deref, DerefMut};
 use std::ptr::{self, NonNull};
 use std::sync::Arc;
@@ -147,20 +147,29 @@ impl Drop for InstanceHandle {
             unsafe {
                 let inst = self.inst.as_mut();
 
-                // Grab a handle to the region to ensure it outlives `inst`.
+                // The `inst.alloc` field manages the memory of the instance
+                // itself. Note, though, that this field is in a `ManuallyDrop`
+                // so it won't get dropped automatically in `drop_in_place`.
+                // This is the point where we take over that precise drop.
                 //
-                // This ensures that the region won't be dropped by `inst` being
-                // dropped, which could result in `inst` being unmapped by the
-                // Region *during* drop of the Instance's fields.
-                let region: Arc<dyn RegionInternal> = inst.alloc().region.clone();
+                // By using `take` here we're basically calling `ptr::read`
+                // which "duplicates" the `alloc` since the `alloc` local
+                // variable here is the exact same as `inst.alloc`. All we do
+                // with `inst`, though, is call `drop_in_place`, which
+                // invalidates every other field in `inst`.
+                let alloc: Alloc = ManuallyDrop::take(&mut inst.alloc);
 
                 // drop the actual instance
                 std::ptr::drop_in_place(inst);
 
-                // and now we can drop what may be the last Arc<Region>. If it is
-                // it can safely do what it needs with memory; we're not running
-                // destructors on it anymore.
-                mem::drop(region);
+                // Now that we're 100% done with the instance, destructors and
+                // all, we can release the memory of the instance back to the
+                // original allocator from whence it came (be it mmap or uffd
+                // based). This will run the "official" destructor for `Alloc`
+                // which internally does the release. Note that after this
+                // operation the `inst` pointer is invalid and can no longer be
+                // used.
+                drop(alloc);
             }
         }
     }
@@ -233,8 +242,12 @@ pub struct Instance {
     /// Conditionally-present helpers to force permutations of possible races in testing.
     pub lock_testpoints: Arc<LockTestpoints>,
 
-    /// The memory allocated for this instance
-    alloc: Alloc,
+    /// The memory allocated for this instance.
+    ///
+    /// Note that this is in a `ManuallyDrop` because this manages the memory of
+    /// this `Instance` itself. To have precise control over this memory we
+    /// handle this in `Drop for InstanceHandle`.
+    alloc: ManuallyDrop<Alloc>,
 
     /// Handler run for signals that do not arise from a known WebAssembly trap, or that involve
     /// memory outside of the current instance.
@@ -1055,7 +1068,7 @@ impl Instance {
             kill_state,
             #[cfg(feature = "concurrent_testpoints")]
             lock_testpoints,
-            alloc,
+            alloc: ManuallyDrop::new(alloc),
             fatal_handler: default_fatal_handler,
             c_fatal_handler: None,
             signal_handler: Box::new(signal_handler_none) as Box<SignalHandler>,
path: lucet-runtime/lucet-runtime-internals/src/instance.rs