Repository: bytecodealliance/wasmtime
Pull request #9156 (2024-08-20 19:17:51)

winch: Solidify bounds check for dynamic heaps
This commit fixes and edge case for bounds checks for dynamic heaps.

https://github.com/bytecodealliance/wasmtime/pull/8157/files erroneously tied the bounds check operation (more concretely the overflow check) to the size derived from from the heap type. Even though offsets and access sizes are validated ahead-of-time and bound to the heap type, in the case of overflow checking, we must ensure that the operation size is tied to the target's pointer size to avoid clamping the access size and offset addition, which would result in missing an out-of-bounds memory access.

This commit also adds a disassembly test to avoid introducing regressions in the future.

Additionally, this commit adds more comments around why `pointer_size` is used for certain bounds checking operations.

<!--
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 97e1f0ce4..6d052982e 100644
--- a/winch/codegen/src/codegen/mod.rs
+++ b/winch/codegen/src/codegen/mod.rs
@@ -593,6 +593,14 @@ where
                 // Move the value of the index to the
                 // index_offset_and_access_size register to perform the overflow
                 // check to avoid clobbering the initial index value.
+                //
+                // We derive size of the operation from the heap type since:
+                //
+                // * This is the first assignment to the
+                // `index_offset_and_access_size` register
+                //
+                // * The memory64 proposal specifies that the index is bound to
+                // the heap type instead of hardcoding it to 32-bits (i32).
                 self.masm.mov(
                     index_reg.into(),
                     index_offset_and_access_size,
@@ -601,11 +609,18 @@ where
                 // Perform
                 // index = index + offset + access_size, trapping if the
                 // addition overflows.
+                //
+                // We use the target's pointer size rather than depending on the heap
+                // type since we want to check for overflow; even though the
+                // offset and access size are guaranteed to be bounded by the heap
+                // type, when added, if used with the wrong operand size, their
+                // result could be clamped, resulting in an erroneus overflow
+                // check.
                 self.masm.checked_uadd(
                     index_offset_and_access_size,
                     index_offset_and_access_size,
                     RegImm::i64(offset_with_access_size as i64),
-                    heap.ty.into(),
+                    ptr_size,
                     TrapCode::HeapOutOfBounds,
                 );
 
@@ -623,7 +638,10 @@ where
                         masm.cmp(
                             index_offset_and_access_size.into(),
                             bounds_reg.into(),
-                            heap.ty.into(),
+                            // We use the pointer size to keep the bounds
+                            // comparison consistent with the result of the
+                            // overflow check above.
+                            ptr_size,
                         );
                         IntCmpKind::GtU
                     },
@@ -707,7 +725,12 @@ where
                         masm.cmp(
                             index_reg,
                             RegImm::i64(adjusted_bounds as i64),
-                            heap.ty.into(),
+                            // Similar to the dynamic heap case, even though the
+                            // offset and access size are bound through the heap
+                            // type, when added they can overflow, resulting in
+                            // an erroneus comparison, therfore we rely on the
+                            // target pointer size.
+                            ptr_size,
                         );
                         IntCmpKind::GtU
                     },


Test patch:
diff --git a/tests/disas/winch/x64/store/oob.wat b/tests/disas/winch/x64/store/oob.wat
new file mode 100644
index 000000000..4cb68e0d8
--- /dev/null
+++ b/tests/disas/winch/x64/store/oob.wat
@@ -0,0 +1,48 @@
+;;! target = "x86_64"
+;;! test = "winch"
+;;! flags = " -O static-memory-maximum-size=0"
+(module
+  (memory 1)
+  (func (export "foo") (param $i i32)
+    i32.const 0
+    (local.get $i)
+    i32.store8 offset=4294967295
+  )
+)
+
+;; wasm[0]::function[0]:
+;;       pushq   %rbp
+;;       movq    %rsp, %rbp
+;;       movq    8(%rdi), %r11
+;;       movq    (%r11), %r11
+;;       addq    $0x20, %r11
+;;       cmpq    %rsp, %r11
+;;       ja      0x85
+;;   1b: movq    %rdi, %r14
+;;       subq    $0x20, %rsp
+;;       movq    %rdi, 0x18(%rsp)
+;;       movq    %rsi, 0x10(%rsp)
+;;       movl    %edx, 0xc(%rsp)
+;;       movl    0xc(%rsp), %eax
+;;       movl    $0, %ecx
+;;       movq    0x68(%r14), %rdx
+;;       movl    %ecx, %ebx
+;;       movabsq $0x100000000, %r11
+;;       addq    %r11, %rbx
+;;       jb      0x87
+;;   52: cmpq    %rdx, %rbx
+;;       ja      0x89
+;;   5b: movq    0x60(%r14), %rsi
+;;       addq    %rcx, %rsi
+;;       movabsq $0xffffffff, %r11
+;;       addq    %r11, %rsi
+;;       movq    $0, %rdi
+;;       cmpq    %rdx, %rbx
+;;       cmovaq  %rdi, %rsi
+;;       movb    %al, (%rsi)
+;;       addq    $0x20, %rsp
+;;       popq    %rbp
+;;       retq
+;;   85: ud2
+;;   87: ud2
+;;   89: ud2
diff --git a/tests/misc_testsuite/winch/oob.wast b/tests/misc_testsuite/winch/oob.wast
new file mode 100644
index 000000000..c16344d64
--- /dev/null
+++ b/tests/misc_testsuite/winch/oob.wast
@@ -0,0 +1,10 @@
+(module
+  (memory 1)
+  (func (export "foo") (param $i i32)
+    i32.const 0
+    (local.get $i)
+    i32.store8 offset=4294967295
+  )
+)
+
+(assert_trap (invoke "foo" (i32.const 0)) "out of bounds")