Repository: bytecodealliance/wasmtime
Pull request #8918 (2024-07-08 20:21:39)

Fix segfault with custom page sizes on aarch64
This commit fixes an issue with static memory initialization and custom page sizes interacting together on aarch64 Linux. (is that specific enough?)

When static memory initialization is enabled chunks of memory to initialize the linear memory are made in host-page-size increments of memory. This is done to enable page-mapping via copy-on-write if customized. With the custom page sizes proposal, however, for the first time it's possible for a linear memory to be smaller than this chunk of memory. This means that a virtual memory allocation of a single host page can be made which is smaller than the initialization chunk.

This currently only happens on aarch64 Linux where we coarsely approximate that the host page size is 64k but many hosts run with 4k pages. This means that a 64k initializer is created but the host only allocates 4k for a linear memory. This means that memory initialization can crash when a 64k initializer is copied into a 4k memory.

This was not caught via fuzzing because fuzzing only runs on x86_64. This was not caught via CI because on CI guard pages are disabled entirely on QEMU and we got lucky in that a number of virtual memory allocations were all placed next to each other meaning that this copy was probably corrupting some other memory. Locally this was found by running tests on `main` as-is on AArch64 Linux (by bjorn3).

This commit implements a few safeguards and a fix for this issue:

* On CI with QEMU modestly-size guard pages are now enabled to catch this sooner in development should it happen again in the future.
* An `assert!` is added during memory initialization that the memory copy is indeed valid. This causes the tests to fail as-is on `main` even on x86_64.
* The issue itself is fixed by bailing out of static memory initialization should the host page size exceed the wasm page size which can now happen on aarch64 Linux with smaller page sizes.

<!--
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/cranelift/codegen/src/ir/extfunc.rs b/cranelift/codegen/src/ir/extfunc.rs
index c4721eb13..7300b9828 100644
--- a/cranelift/codegen/src/ir/extfunc.rs
+++ b/cranelift/codegen/src/ir/extfunc.rs
@@ -233,7 +233,13 @@ pub enum ArgumentPurpose {
     Normal,
 
     /// A C struct passed as argument.
-    StructArgument(u32),
+    ///
+    /// Note that this should only be used when interacting with code following
+    /// a C ABI which is expecting a struct passed *by value*.
+    StructArgument(
+        /// The size, in bytes, of the struct.
+        u32,
+    ),
 
     /// Struct return pointer.
     ///
diff --git a/cranelift/src/bugpoint.rs b/cranelift/src/bugpoint.rs
index 862664b11..7670c58d9 100644
--- a/cranelift/src/bugpoint.rs
+++ b/cranelift/src/bugpoint.rs
@@ -831,6 +831,15 @@ fn try_resolve_aliases(context: &mut CrashCheckContext, func: &mut Function) {
     }
 }
 
+/// Remove sourcelocs if the function still crashes after they are removed, to make the reduced clif IR easier to read.
+fn try_remove_srclocs(context: &mut CrashCheckContext, func: &mut Function) {
+    let mut func_with_removed_sourcelocs = func.clone();
+    func_with_removed_sourcelocs.srclocs.clear();
+    if let CheckResult::Crash(_) = context.check_for_crash(&func_with_removed_sourcelocs) {
+        *func = func_with_removed_sourcelocs;
+    }
+}
+
 fn reduce(isa: &dyn TargetIsa, mut func: Function, verbose: bool) -> Result<(Function, String)> {
     let mut context = CrashCheckContext::new(isa);
 
@@ -839,6 +848,7 @@ fn reduce(isa: &dyn TargetIsa, mut func: Function, verbose: bool) -> Result<(Fun
     }
 
     try_resolve_aliases(&mut context, &mut func);
+    try_remove_srclocs(&mut context, &mut func);
 
     let progress_bar = ProgressBar::with_draw_target(0, ProgressDrawTarget::stdout());
     progress_bar.set_style(
diff --git a/crates/cli-flags/src/lib.rs b/crates/cli-flags/src/lib.rs
index ab39e9997..33249451c 100644
--- a/crates/cli-flags/src/lib.rs
+++ b/crates/cli-flags/src/lib.rs
@@ -260,6 +260,8 @@ wasmtime_option_group! {
         pub function_references: Option<bool>,
         /// Configure support for the GC proposal.
         pub gc: Option<bool>,
+        /// Configure support for the custom-page-sizes proposal.
+        pub custom_page_sizes: Option<bool>,
     }
 
     enum Wasm {
@@ -660,6 +662,9 @@ impl CommonOptions {
         if let Some(enable) = self.wasm.memory64.or(all) {
             config.wasm_memory64(enable);
         }
+        if let Some(enable) = self.wasm.custom_page_sizes.or(all) {
+            config.wasm_custom_page_sizes(enable);
+        }
 
         macro_rules! handle_conditionally_compiled {
             ($(($feature:tt, $field:tt, $method:tt))*) => ($(
diff --git a/crates/environ/src/compile/module_environ.rs b/crates/environ/src/compile/module_environ.rs
index f8edcc845..7c90f1596 100644
--- a/crates/environ/src/compile/module_environ.rs
+++ b/crates/environ/src/compile/module_environ.rs
@@ -996,9 +996,20 @@ impl ModuleTranslation<'_> {
 
         // Validate that the memory information collected is indeed valid for
         // static memory initialization.
-        for info in info.values().filter(|i| i.data_size > 0) {
+        for (i, info) in info.iter().filter(|(_, info)| info.data_size > 0) {
             let image_size = info.max_addr - info.min_addr;
 
+            // Simplify things for now by bailing out entirely if any memory has
+            // a page size smaller than the host's page size. This fixes a case
+            // where currently initializers are created in host-page-size units
+            // of length which means that a larger-than-the-entire-memory
+            // initializer can be created. This can be handled technically but
+            // would require some more changes to help fix the assert elsewhere
+            // that this protects against.
+            if self.module.memory_plans[i].memory.page_size() < page_size {
+                return;
+            }
+
             // If the range of memory being initialized is less than twice the
             // total size of the data itself then it's assumed that static
             // initialization is ok. This means we'll at most double memory
diff --git a/crates/wasmtime/src/runtime/vm/instance/allocator.rs b/crates/wasmtime/src/runtime/vm/instance/allocator.rs
index 766293eab..ee39886cb 100644
--- a/crates/wasmtime/src/runtime/vm/instance/allocator.rs
+++ b/crates/wasmtime/src/runtime/vm/instance/allocator.rs
@@ -732,7 +732,11 @@ fn initialize_memories(instance: &mut Instance, module: &Module) -> Result<()> {
 
             unsafe {
                 let src = self.instance.wasm_data(init.data.clone());
-                let dst = memory.base.add(usize::try_from(init.offset).unwrap());
+                let offset = usize::try_from(init.offset).unwrap();
+                let dst = memory.base.add(offset);
+
+                assert!(offset + src.len() <= memory.current_length());
+
                 // FIXME audit whether this is safe in the presence of shared
                 // memory
                 // (https://github.com/bytecodealliance/wasmtime/issues/4203).


Test patch:
diff --git a/cranelift/tests/bugpoint_test.clif b/cranelift/tests/bugpoint_test.clif
index 47bcb734a..757bdfc2b 100644
--- a/cranelift/tests/bugpoint_test.clif
+++ b/cranelift/tests/bugpoint_test.clif
@@ -300,12 +300,12 @@ block0(v0: i64, v1: i64, v2: i64):
     v241 -> v1
     v256 -> v1
     v262 -> v1
-    v3 = imul v0, v1
+    @0001 v3 = imul v0, v1
     v4 = imul v1, v2
     store aligned v4, v3
     v5 = load.i64 aligned v2+8
     store aligned v5, v3+8
-    v6 = stack_addr.i64 ss1
+    @0002 v6 = stack_addr.i64 ss1
     v7 = stack_addr.i64 ss2
     v8 = stack_addr.i64 ss3
     v9 = stack_addr.i64 ss4
@@ -366,7 +366,7 @@ block0(v0: i64, v1: i64, v2: i64):
     v64 = stack_addr.i64 ss59
     v65 = stack_addr.i64 ss60
     v66 = stack_addr.i64 ss61
-    v67 = stack_addr.i64 ss62
+    @0003 v67 = stack_addr.i64 ss62
     v68 = stack_addr.i64 ss63
     v69 = stack_addr.i64 ss64
     v70 = stack_addr.i64 ss65
diff --git a/tests/wast.rs b/tests/wast.rs
index 789ab1cb7..71527d4e3 100644
--- a/tests/wast.rs
+++ b/tests/wast.rs
@@ -294,8 +294,10 @@ fn run_wast(wast: &Path, strategy: Strategy, pooling: bool) -> anyhow::Result<()
             cfg.static_memory_maximum_size(0);
         }
         cfg.dynamic_memory_reserved_for_growth(0);
-        cfg.static_memory_guard_size(0);
-        cfg.dynamic_memory_guard_size(0);
+
+        let small_guard = 64 * 1024;
+        cfg.static_memory_guard_size(small_guard);
+        cfg.dynamic_memory_guard_size(small_guard);
     }
 
     let _pooling_lock = if pooling {