diff --git a/crates/wasmtime/src/runtime/vm/cow.rs b/crates/wasmtime/src/runtime/vm/cow.rs index 6cc53eff0092..398d86f9352a 100644 --- a/crates/wasmtime/src/runtime/vm/cow.rs +++ b/crates/wasmtime/src/runtime/vm/cow.rs @@ -4,6 +4,7 @@ use super::sys::DecommitBehavior; use crate::Engine; use crate::prelude::*; +use crate::runtime::vm::mpk::ProtectionKey; use crate::runtime::vm::sys::vm::{self, MemoryImageSource, PageMap, reset_with_pagemap}; use crate::runtime::vm::{ HostAlignedByteCount, MmapOffset, ModuleMemoryImageSource, host_page_size, @@ -337,6 +338,14 @@ pub struct MemoryImageSlot { /// initial image content, as appropriate. Everything between /// `self.accessible` and `self.static_size` is inaccessible. dirty: bool, + + /// The MPK protection key that this slot's stripe was colored with, if the + /// pooling allocator is striping memory with protection keys. + /// + /// This must be re-applied after every `mmap` performed on this slot, since + /// `mmap` resets the affected pages back to the default key 0 which is + /// accessible from every stripe. + pkey: Option, } impl fmt::Debug for MemoryImageSlot { @@ -363,6 +372,7 @@ impl MemoryImageSlot { base: MmapOffset, accessible: HostAlignedByteCount, static_size: usize, + pkey: Option, ) -> Self { MemoryImageSlot { base, @@ -370,6 +380,7 @@ impl MemoryImageSlot { accessible, image: None, dirty: false, + pkey, } } @@ -489,6 +500,11 @@ impl MemoryImageSlot { unsafe { image.map_at(&self.base)?; } + // `map_at` above `mmap`'d over part of this slot, which + // reset those pages to the default protection key. Restore + // this slot's key so the image is not left accessible to + // every other stripe. + self.reapply_pkey(image.linear_memory_offset, image.len, true)?; } } self.image = maybe_image.cloned(); @@ -506,6 +522,9 @@ impl MemoryImageSlot { unsafe { image.remap_as_zeros_at(self.base.as_mut_ptr())?; } + // As in `instantiate`, the `mmap` above dropped this slot's + // protection key over the image's range, so restore it. + self.reapply_pkey(image.linear_memory_offset, image.len, true)?; self.image = None; } Ok(()) @@ -682,6 +701,43 @@ impl MemoryImageSlot { Ok(()) } + /// Re-color `offset..offset + len` within this slot with this slot's MPK + /// protection key, if any. + /// + /// This is a no-op unless the pooling allocator is striping memory with + /// protection keys. It must be called after every `mmap` that lands inside + /// this slot: `mmap` associates the pages it replaces with the default key + /// 0, which is accessible regardless of which stripe is currently active, + /// so skipping this would let one instance read and write another + /// instance's memory. + /// + /// Note that `mprotect` preserves the existing key, so `set_protection` + /// does not need this treatment. + fn reapply_pkey( + &self, + offset: HostAlignedByteCount, + len: HostAlignedByteCount, + readwrite: bool, + ) -> Result<()> { + let Some(pkey) = self.pkey else { + return Ok(()); + }; + if len.is_zero() { + return Ok(()); + } + // `mmap` rounds lengths up to a page boundary, so the restored range is + // allowed to extend to the end of the slot's final page. + debug_assert!( + offset.byte_count() + len.byte_count() + <= self.static_size.next_multiple_of(host_page_size()) + ); + unsafe { + let start = self.base.as_mut_ptr().add(offset.byte_count()); + pkey.reprotect(start.addr(), len.byte_count(), readwrite)?; + } + Ok(()) + } + pub(crate) fn has_image(&self) -> bool { self.image.is_some() } @@ -704,6 +760,11 @@ impl MemoryImageSlot { vm::erase_existing_mapping(self.base.as_mut_ptr(), self.static_size)?; } + // The `mmap` above covers the whole slot and left it inaccessible, so + // restore this slot's protection key across the same range. + let static_size = HostAlignedByteCount::new_rounded_up(self.static_size)?; + self.reapply_pkey(HostAlignedByteCount::ZERO, static_size, false)?; + self.image = None; self.accessible = HostAlignedByteCount::ZERO; @@ -813,8 +874,12 @@ mod test { // 4 MiB mmap'd area, not accessible let mmap = mmap_4mib_inaccessible(); // Create a MemoryImageSlot on top of it - let mut memfd = - MemoryImageSlot::create(mmap.zero_offset(), HostAlignedByteCount::ZERO, 4 << 20); + let mut memfd = MemoryImageSlot::create( + mmap.zero_offset(), + HostAlignedByteCount::ZERO, + 4 << 20, + None, + ); assert!(!memfd.is_dirty()); // instantiate with 64 KiB initial size memfd @@ -872,8 +937,12 @@ mod test { // 4 MiB mmap'd area, not accessible let mmap = mmap_4mib_inaccessible(); // Create a MemoryImageSlot on top of it - let mut memfd = - MemoryImageSlot::create(mmap.zero_offset(), HostAlignedByteCount::ZERO, 4 << 20); + let mut memfd = MemoryImageSlot::create( + mmap.zero_offset(), + HostAlignedByteCount::ZERO, + 4 << 20, + None, + ); // Create an image with some data. let image = Arc::new(create_memfd_with_data(page_size, &[1, 2, 3, 4]).unwrap()); // Instantiate with this image @@ -983,8 +1052,12 @@ mod test { ..Tunables::default_miri() }; let mmap = mmap_4mib_inaccessible(); - let mut memfd = - MemoryImageSlot::create(mmap.zero_offset(), HostAlignedByteCount::ZERO, 4 << 20); + let mut memfd = MemoryImageSlot::create( + mmap.zero_offset(), + HostAlignedByteCount::ZERO, + 4 << 20, + None, + ); // Test basics with the image for image_off in [0, page_size, page_size * 2] { @@ -1057,8 +1130,12 @@ mod test { }; let mmap = mmap_4mib_inaccessible(); - let mut memfd = - MemoryImageSlot::create(mmap.zero_offset(), HostAlignedByteCount::ZERO, 4 << 20); + let mut memfd = MemoryImageSlot::create( + mmap.zero_offset(), + HostAlignedByteCount::ZERO, + 4 << 20, + None, + ); let image = Arc::new(create_memfd_with_data(page_size, &[1, 2, 3, 4]).unwrap()); let initial = 64 << 10; @@ -1166,8 +1243,12 @@ mod test { }; let mmap = mmap_4mib_inaccessible(); let mmap_len = page_size * 9; - let mut memfd = - MemoryImageSlot::create(mmap.zero_offset(), HostAlignedByteCount::ZERO, mmap_len); + let mut memfd = MemoryImageSlot::create( + mmap.zero_offset(), + HostAlignedByteCount::ZERO, + mmap_len, + None, + ); let pagemap = PageMap::new(); let pagemap = pagemap.as_ref(); diff --git a/crates/wasmtime/src/runtime/vm/instance/allocator/pooling/memory_pool.rs b/crates/wasmtime/src/runtime/vm/instance/allocator/pooling/memory_pool.rs index 1dc57b12324e..5a554d72cd7a 100644 --- a/crates/wasmtime/src/runtime/vm/instance/allocator/pooling/memory_pool.rs +++ b/crates/wasmtime/src/runtime/vm/instance/allocator/pooling/memory_pool.rs @@ -589,6 +589,19 @@ impl MemoryPool { self.mapping.offset(offset).expect("offset is in bounds") } + /// Return the protection key that this slot's memory was striped with when + /// the pool was created, if any. + /// + /// This mirrors the striping performed in `new`: memory is only colored + /// when there are at least two stripes, and slot `i` is colored with the + /// `i % num_stripes`th key. + fn pkey_for_slot(&self, allocation_index: MemoryAllocationIndex) -> Option { + if self.stripes.len() < 2 { + return None; + } + self.stripes[allocation_index.index() % self.stripes.len()].pkey + } + /// Take ownership of the given image slot. /// /// This method is used when a `MemoryAllocationIndex` has been allocated @@ -620,6 +633,7 @@ impl MemoryPool { self.get_base(allocation_index), HostAlignedByteCount::ZERO, self.layout.max_memory_bytes.byte_count(), + self.pkey_for_slot(allocation_index), ) }); diff --git a/crates/wasmtime/src/runtime/vm/memory.rs b/crates/wasmtime/src/runtime/vm/memory.rs index fce3c4afa03f..ef68214ed745 100644 --- a/crates/wasmtime/src/runtime/vm/memory.rs +++ b/crates/wasmtime/src/runtime/vm/memory.rs @@ -570,8 +570,10 @@ impl LocalMemory { } }; + // Memories allocated on demand are never striped with MPK + // protection keys, so this slot has no key to preserve. let mut slot = - MemoryImageSlot::create(mmap_base, byte_size, alloc.byte_capacity()); + MemoryImageSlot::create(mmap_base, byte_size, alloc.byte_capacity(), None); slot.instantiate(alloc.byte_size(), Some(image), ty, memory_tunables)?; Some(slot) } else { diff --git a/crates/wasmtime/src/runtime/vm/mpk/disabled.rs b/crates/wasmtime/src/runtime/vm/mpk/disabled.rs index 6ebd23823738..365dce91d4d2 100644 --- a/crates/wasmtime/src/runtime/vm/mpk/disabled.rs +++ b/crates/wasmtime/src/runtime/vm/mpk/disabled.rs @@ -1,7 +1,7 @@ //! Noop implementations of MPK primitives for environments that do not support //! the feature. -#[cfg(feature = "pooling-allocator")] +#[cfg(any(feature = "pooling-allocator", has_virtual_memory))] use crate::prelude::*; #[cfg(feature = "pooling-allocator")] @@ -34,6 +34,12 @@ impl ProtectionKey { pub fn as_stripe(&self) -> usize { match *self {} } + // Note: gated on `has_virtual_memory` rather than `pooling-allocator` + // because this is called from `cow.rs`, which is not pooling-specific. + #[cfg(has_virtual_memory)] + pub unsafe fn reprotect(&self, _: usize, _: usize, _: bool) -> Result<()> { + match *self {} + } } #[derive(Clone, Copy, Debug)] diff --git a/crates/wasmtime/src/runtime/vm/mpk/enabled.rs b/crates/wasmtime/src/runtime/vm/mpk/enabled.rs index 14ddba4d628f..bef2fb3b1fa1 100644 --- a/crates/wasmtime/src/runtime/vm/mpk/enabled.rs +++ b/crates/wasmtime/src/runtime/vm/mpk/enabled.rs @@ -108,6 +108,35 @@ impl ProtectionKey { pub fn as_stripe(&self) -> usize { self.stripe as usize } + + /// Re-apply this [`ProtectionKey`] to a region that has just been re-mapped. + /// + /// A fresh `mmap` over a region discards that region's protection key, + /// leaving it associated with the default key 0 which is always accessible. + /// Any code that maps over pkey-protected memory must therefore call this + /// afterwards to restore the key, otherwise the memory becomes readable and + /// writable from any stripe. + /// + /// Note that `mprotect` (unlike `mmap`) preserves the existing key, so only + /// `mmap` call sites need this. + /// + /// # Safety + /// + /// `addr` must be page-aligned and `addr..addr + len` must describe a mapped + /// region owned by the caller. `readwrite` must match the page protections + /// the region was just mapped with, since this overwrites them. + pub unsafe fn reprotect(&self, addr: usize, len: usize, readwrite: bool) -> Result<()> { + let prot = if readwrite { + sys::PROT_READ | sys::PROT_WRITE + } else { + sys::PROT_NONE + }; + sys::pkey_mprotect(addr, len, prot, self.id).with_context(|| { + format!( + "failed to restore pkey on region (addr = {addr:#x}, len = {len}, prot = {prot:#b})" + ) + }) + } } /// A bit field indicating which protection keys should be allowed and disabled. diff --git a/crates/wasmtime/src/runtime/vm/mpk/sys.rs b/crates/wasmtime/src/runtime/vm/mpk/sys.rs index fffe53b85dac..f8f1c2776273 100644 --- a/crates/wasmtime/src/runtime/vm/mpk/sys.rs +++ b/crates/wasmtime/src/runtime/vm/mpk/sys.rs @@ -18,6 +18,14 @@ use std::io::Error; /// to start as `PROT_NONE`. pub const PROT_NONE: u32 = libc::PROT_NONE as u32; // == 0b0000; +/// Protection mask allowing reads of pkey-protected memory (see `prot` in +/// [`pkey_mprotect`]). +pub const PROT_READ: u32 = libc::PROT_READ as u32; // == 0b0001; + +/// Protection mask allowing writes of pkey-protected memory (see `prot` in +/// [`pkey_mprotect`]). +pub const PROT_WRITE: u32 = libc::PROT_WRITE as u32; // == 0b0010; + /// Allocate a new protection key in the Linux kernel ([docs]); returns the /// key ID. /// diff --git a/tests/all/pooling_allocator.rs b/tests/all/pooling_allocator.rs index fc19983934ca..e556f233c269 100644 --- a/tests/all/pooling_allocator.rs +++ b/tests/all/pooling_allocator.rs @@ -1556,3 +1556,96 @@ fn purge_module_with_mpk() -> Result<()> { Ok(()) } + +/// Regression test for both #7942 and #13982: mapping a copy-on-write memory +/// image into a slot must not drop the slot's MPK protection key. +/// +/// A fresh `mmap` associates the pages it replaces with the default protection +/// key 0, which every stripe is allowed to access. If the key is not +/// re-applied afterwards, an instance in one stripe can read and write the +/// linear memory of an instance in another stripe. +#[test] +#[cfg_attr(miri, ignore)] +fn mpk_protects_memory_images() -> Result<()> { + if !wasmtime::PoolingAllocationConfig::are_memory_protection_keys_available() { + println!("skipping test; mpk is not supported"); + return Ok(()); + } + + let mut pool = wasmtime::PoolingAllocationConfig::new(); + pool.memory_protection_keys(Enabled::Yes) + .max_memory_protection_keys(2) + .max_memory_size(1 << 20) + .total_memories(4) + .total_tables(4) + .total_core_instances(4); + let mut config = Config::new(); + config.allocation_strategy(InstanceAllocationStrategy::Pooling(pool)); + let engine = Engine::new(&config)?; + + // The victim has a `(data ...)` segment, so it is instantiated from a + // copy-on-write memory image; that is the mapping which used to clobber + // the protection key. + let victim = Module::new( + &engine, + r#"(module (memory (export "m") 1) (data (i32.const 0) "SECRET"))"#, + )?; + let attacker = Module::new( + &engine, + r#"(module + (memory (export "m") 1) + (func (export "load") (param i32) (result i32) local.get 0 i32.load) + (func (export "store") (param i32 i32) local.get 0 local.get 1 i32.store))"#, + )?; + + let mut attacker_store = Store::new(&engine, ()); + let mut victim_store = Store::new(&engine, ()); + let attacker_instance = Instance::new(&mut attacker_store, &attacker, &[])?; + let victim_instance = Instance::new(&mut victim_store, &victim, &[])?; + + let attacker_mem = attacker_instance + .get_memory(&mut attacker_store, "m") + .unwrap(); + let victim_mem = victim_instance.get_memory(&mut victim_store, "m").unwrap(); + + // Only meaningful if the two instances landed in different stripes and the + // victim is within reach of a 32-bit wasm address. + let attacker_base = attacker_mem.data_ptr(&attacker_store) as usize; + let victim_base = victim_mem.data_ptr(&victim_store) as usize; + let offset = match victim_base + .checked_sub(attacker_base) + .and_then(|offset| u32::try_from(offset).ok()) + { + // Wasm addresses are unsigned, so this is a plain bit-cast. + Some(offset) => offset as i32, + None => { + println!("skipping test; victim memory is not addressable by the attacker"); + return Ok(()); + } + }; + + let load = attacker_instance.get_typed_func::(&mut attacker_store, "load")?; + let store = attacker_instance.get_typed_func::<(i32, i32), ()>(&mut attacker_store, "store")?; + + // The attacker can still use its own memory... + store.call(&mut attacker_store, (0, 0x12345678))?; + assert_eq!(load.call(&mut attacker_store, 0)?, 0x12345678); + + // ...and the victim's image was still applied correctly... + assert_eq!(&victim_mem.data(&victim_store)[..6], b"SECRET"); + + // ...but it must not be able to touch the victim's memory. + assert!( + load.call(&mut attacker_store, offset).is_err(), + "attacker read across an MPK stripe boundary" + ); + assert!( + store + .call(&mut attacker_store, (offset, 0x41414141)) + .is_err(), + "attacker wrote across an MPK stripe boundary" + ); + assert_eq!(&victim_mem.data(&victim_store)[..6], b"SECRET"); + + Ok(()) +}