bytecodealliance / wasmtime

A fast and secure runtime for WebAssembly
https://wasmtime.dev/
Apache License 2.0
15.46k stars 1.31k forks source link

Cranelift: incorrect narrow `sdiv` lowering on AArch64 #9537

Closed mmcloughlin closed 4 weeks ago

mmcloughlin commented 4 weeks ago

There may be a bug in the lowering of narrow sdiv instructions on AArch64.

.clif Test Case

function %div8(i8, i8) -> i8 {
block0(v0: i8, v1: i8):
  v2 = sdiv v0, v1
  return v2
}

; run: %div8(-128, -1) == -128

Note: I believe this would affect 16-bit sdiv as well, though I have not tested this case as thoroughly.

Steps to Reproduce

Run test case in clif-util via the interpreter and JIT on an AArch64 platform.

$ clif-util interpret sdiv.clif
$ clif-util run sdiv.clif

Expected Results

The CLIF execution should trap, via interpreter and JIT.

Actual Results

In the interpreter, we get Unexpected returned control flow which I confirmed with a small edit is actually a trap:

$ clif-util interpret --verbose sdiv.clif
thread 'main' panicked at cranelift/src/interpret.rs:129:34:
Unexpected returned control flow--this is likely a bug.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

JIT passes the test case:

$ clif-util run --verbose sdiv.clif
sdiv.clif
1 file

Versions and Environment

Cranelift version or commit: recent main branch commit a82bdd833d1787953b866b2c375832dd9b911f1b

Operating system: macOS 15.0.1

Architecture: AArch64 (Apple M1)

Extra Info

Diagnosis

I believe the problem lies in the trap_if_div_overflow rule:

https://github.com/bytecodealliance/wasmtime/blob/2a7f065335ae2ff48c0b8cc486e20ab83d1a1690/cranelift/codegen/src/isa/aarch64/inst.isle#L3681-L3689

This code is checking for the minimum 32-bit signed value, but this is not correct for the 8/16-bit cases. Subtracting 1 from -128 as a 32-bit value does not cause overflow, therefore the trap on the Vs condition does not fire.

Security

Discussed with @cfallin on Zulip who confirmed this is not a security-critical miscompile.