Stream: git-wasmtime

Topic: wasmtime / issue #9537 Cranelift: incorrect narrow `sdiv`...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2024 at 23:22):

mmcloughlin added the bug label to Issue #9537.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2024 at 23:22):

mmcloughlin added the cranelift label to Issue #9537.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2024 at 23:22):

mmcloughlin opened issue #9537:

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

For the 8 and 16 bit case, the value we are checking for is not in fact the minimum 32-bit signed value. 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 31 2024 at 23:28):

mmcloughlin edited issue #9537:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 18:22):

cfallin closed issue #9537:

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.

Last updated: Dec 23 2024 at 13:07 UTC