From b224e02b1554d6d55fc9ab59439d547faa768213 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sa=C3=BAl=20Cabrera?= <saulecabrera@gmail.com>
Date: Tue, 27 Feb 2024 13:20:30 -0500
Subject: [PATCH 1/2] winch: Fix bounds checks for dynamic heaps

This commit fixes a fuzz bug in which the current implementation was incorrectly cloberring 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.
---
 tests/all/winch.rs               | 72 ++++++++++++++++++++++++++++++++
 winch/codegen/src/codegen/mod.rs | 10 ++++-
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/tests/all/winch.rs b/tests/all/winch.rs
index d8a7075161dc..fc6b7b063ea6 100644
--- a/tests/all/winch.rs
+++ b/tests/all/winch.rs
@@ -239,3 +239,75 @@ 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 func = Func::wrap::<(), (), Result<()>>(&mut store, || bail!("error"));
+    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(())
+}
diff --git a/winch/codegen/src/codegen/mod.rs b/winch/codegen/src/codegen/mod.rs
index caf76f668406..8e933ed74104 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,

From d2246fc32f8c54d07056b4cda1508ae8f770fccf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sa=C3=BAl=20Cabrera?= <saulecabrera@gmail.com>
Date: Tue, 27 Feb 2024 13:36:49 -0500
Subject: [PATCH 2/2] Remove unused variable in integration tests

---
 tests/all/winch.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/all/winch.rs b/tests/all/winch.rs
index fc6b7b063ea6..fc8a66744419 100644
--- a/tests/all/winch.rs
+++ b/tests/all/winch.rs
@@ -302,7 +302,6 @@ fn dynamic_heap() -> Result<()> {
     "#;
     let mut store = Store::new(&engine, ());
     let module = Module::new(&engine, wat)?;
-    let func = Func::wrap::<(), (), Result<()>>(&mut store, || bail!("error"));
     let instance = Instance::new(&mut store, &module, &[])?;
     let f = instance.get_typed_func::<(), i32>(&mut store, " ")?;
     let result = f.call(&mut store, ())?;