Repository: bytecodealliance/wasmtime
Pull request #8001 (2024-02-27 18:27:09)

winch: Fix bounds checks for dynamic heaps
This commit fixes a fuzz bug in which the current implementation was incorrectly clobbering the index register of a memory access (for addition overflow check) and then using that same clobbered register to perform the memory access. The clobbered register contained the value: `index + offset + access_size`, which resulting in an incorrect access and consequently in an incorrect `HeapOutOfBounds` trap.

This bug is only reproducible when modifying Wasmtime's memory settings, forcing the heap to be treated as `Dynamic`.

Currently in Winch there's no easy way to have specific Wasmtime configurations, so having a filetests for this case is not straightforward. I've opted to add an integration test, in which it's easier to configure Wasmtime.

Note that the `tests/all/winch.rs`  file is temporary, and the plan is to execute all the other integration tests with Winch at some point. In the case of `memory.rs`, that will be once Winch supports `memory64` hoping to reduce the amount of code needed in order to integrate Winch.

<!--
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/winch/codegen/src/codegen/mod.rs b/winch/codegen/src/codegen/mod.rs
index caf76f668..8e933ed74 100644
--- a/winch/codegen/src/codegen/mod.rs
+++ b/winch/codegen/src/codegen/mod.rs
@@ -494,6 +494,7 @@ where
         let index = Index::from_typed_reg(self.context.pop_to_reg(self.masm, None));
         let offset = bounds::ensure_index_and_offset(self.masm, index, memarg.offset, ptr_size);
         let offset_with_access_size = add_offset_and_access_size(offset, access_size);
+        let scratch = <M::ABI as ABI>::scratch_reg();
 
         let addr = match heap.style {
             // == Dynamic Heaps ==
@@ -508,12 +509,17 @@ where
                     bounds::load_dynamic_heap_bounds(&mut self.context, self.masm, &heap, ptr_size);
 
                 let index_reg = index.as_typed_reg().reg;
+
+                // Move the value of the index to the scratch register
+                // to perform the overflow check to avoid clobbering the
+                // initial index value.
+                self.masm.mov(index_reg.into(), scratch, heap.ty.into());
                 // Perform
                 // index = index + offset + access_size, trapping if the
                 // addition overflows.
                 self.masm.checked_uadd(
-                    index_reg,
-                    index_reg,
+                    scratch,
+                    scratch,
                     RegImm::i64(offset_with_access_size as i64),
                     ptr_size,
                     TrapCode::HeapOutOfBounds,


Test patch:
diff --git a/tests/all/winch.rs b/tests/all/winch.rs
index d8a707516..fc8a66744 100644
--- a/tests/all/winch.rs
+++ b/tests/all/winch.rs
@@ -239,3 +239,74 @@ fn wasm_to_native_trap() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+#[cfg_attr(miri, ignore)]
+fn dynamic_heap() -> Result<()> {
+    let mut c = Config::new();
+
+    c.strategy(Strategy::Winch);
+    c.static_memory_maximum_size(0);
+    c.static_memory_guard_size(0);
+    c.guard_before_linear_memory(false);
+    c.dynamic_memory_guard_size(0);
+
+    let engine = Engine::new(&c)?;
+    let wat = r#"
+        (module
+          (type (;0;) (func (result i32)))
+          (func (;0;) (type 0) (result i32)
+            (local i32 i64)
+            global.get 0
+            i32.eqz
+            if ;; label = @1
+              unreachable
+            end
+            global.get 0
+            i32.const 1
+            i32.sub
+            global.set 0
+            memory.size
+            local.set 0
+            block ;; label = @1
+              block ;; label = @2
+                memory.size
+                i32.const 65536
+                i32.mul
+                i32.const 65511
+                local.get 0
+                i32.add
+                i32.le_u
+                br_if 0 (;@2;)
+                local.get 0
+                i32.const 0
+                i32.le_s
+                br_if 0 (;@2;)
+                local.get 0
+                i64.load8_s offset=65503
+                local.set 1
+                br 1 (;@1;)
+              end
+              i64.const 0
+              local.set 1
+            end
+            local.get 1
+            drop
+            i32.const 0
+          )
+          (memory (;0;) 1 3)
+          (global (;0;) (mut i32) i32.const 1000)
+          (export " " (func 0))
+          (export "" (memory 0))
+        )
+    "#;
+    let mut store = Store::new(&engine, ());
+    let module = Module::new(&engine, wat)?;
+    let instance = Instance::new(&mut store, &module, &[])?;
+    let f = instance.get_typed_func::<(), i32>(&mut store, " ")?;
+    let result = f.call(&mut store, ())?;
+
+    assert!(result == 0);
+
+    Ok(())
+}