Repository: bytecodealliance/wasmtime
Pull request #7363 (2023-10-25 17:55:44)

mpk: protect memory with `PROT_NONE`
This change fixes a bug with `ProtectionKey::protect`: previously it initialized each stripe with read and write permissions (i.e., `pkey_mprotect(..., PROT_READ | PROT_WRITE)` under the mistaken assumption that these permissions were MPK-specific, "what MPK permissions will we be allowed to set in the PKRU for these regions in the future?". This assumption is incorrect: the regions were immediately made accessible for reading and writing. The fix is to initially protect the regions with `PROT_NONE` and allow Wasmtime's `memory.grow` implementation to mark pages with `mprotect(..., PROT_READ | PROT_WRITE)` as usual. Whether a store can access a slice is still determined by the CPU state set in `mpk::allow`.

<!--
Please make sure you include the following information:

- If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

- Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->



Patch:
diff --git a/crates/runtime/src/mpk/enabled.rs b/crates/runtime/src/mpk/enabled.rs
index 9c8774233..fe1bdf858 100644
--- a/crates/runtime/src/mpk/enabled.rs
+++ b/crates/runtime/src/mpk/enabled.rs
@@ -78,7 +78,7 @@ impl ProtectionKey {
     pub fn protect(&self, region: &mut [u8]) -> Result<()> {
         let addr = region.as_mut_ptr() as usize;
         let len = region.len();
-        let prot = sys::PROT_READ | sys::PROT_WRITE;
+        let prot = sys::PROT_NONE;
         sys::pkey_mprotect(addr, len, prot, self.id).with_context(|| {
             format!(
                 "failed to mark region with pkey (addr = {addr:#x}, len = {len}, prot = {prot:#b})"
@@ -169,7 +169,7 @@ mod tests {
         assert!(result.is_err());
         assert_eq!(
             result.unwrap_err().to_string(),
-            "failed to mark region with pkey (addr = 0x1, len = 1, prot = 0b11)"
+            "failed to mark region with pkey (addr = 0x1, len = 1, prot = 0b0)"
         );
     }
 
diff --git a/crates/runtime/src/mpk/sys.rs b/crates/runtime/src/mpk/sys.rs
index fe68decce..671ea4eb9 100644
--- a/crates/runtime/src/mpk/sys.rs
+++ b/crates/runtime/src/mpk/sys.rs
@@ -13,13 +13,10 @@ use crate::page_size;
 use anyhow::Result;
 use std::io::Error;
 
-/// 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;
+/// Protection mask disallowing reads and writes of pkey-protected memory (see
+/// `prot` in [`pkey_mprotect`]); in Wasmtime we expect all MPK-protected memory
+/// to start as `PROT_NONE`.
+pub const PROT_NONE: u32 = libc::PROT_NONE as u32; // == 0b0000;
 
 /// Allocate a new protection key in the Linux kernel ([docs]); returns the
 /// key ID.


Test patch:
diff --git a/crates/test-programs/src/bin/preview2_tcp_bind.rs b/crates/test-programs/src/bin/preview2_tcp_bind.rs
index 824b5978e..0dadd114b 100644
--- a/crates/test-programs/src/bin/preview2_tcp_bind.rs
+++ b/crates/test-programs/src/bin/preview2_tcp_bind.rs
@@ -23,12 +23,16 @@ fn test_tcp_bind_specific_port(net: &Network, ip: IpAddress) {
     let bind_addr = IpSocketAddress::new(ip, PORT);
 
     let sock = TcpSocket::new(ip.family()).unwrap();
-    sock.blocking_bind(net, bind_addr).unwrap();
-
-    let bound_addr = sock.local_address().unwrap();
-
-    assert_eq!(bind_addr.ip(), bound_addr.ip());
-    assert_eq!(bind_addr.port(), bound_addr.port());
+    match sock.blocking_bind(net, bind_addr) {
+        Ok(()) => {
+            let bound_addr = sock.local_address().unwrap();
+
+            assert_eq!(bind_addr.ip(), bound_addr.ip());
+            assert_eq!(bind_addr.port(), bound_addr.port());
+        }
+        Err(ErrorCode::AddressInUse) => {}
+        Err(e) => panic!("error: {e}"),
+    }
 }
 
 /// Two sockets may not be actively bound to the same address at the same time.