Cranelift: riscv64 having memory side-effects when trapping

candymate commented 6 months ago

Test Case

// main.rs
use wasmtime::*;

fn main() -> Result<()> {
    let mut config = Config::default();
    config.strategy(Strategy::Cranelift);
    config.cranelift_opt_level(OptLevel::None);

    let engine = Engine::new(&config)?;
    let wat = r#"
    (module
        (type (;0;) (func (param) (result)))
        (import "mem" "mem" (memory (;0;) 1))
        (func (;0;) (type 0) (param) (result)
          i32.const 65521
          v128.const i32x4 0xffffffff 0xffffffff 0xffffffff 0xffffffff
          v128.store align=1)
        (export "main" (func 0)))
    "#;
    let module = Module::new(&engine, wat)?;
    let mut store = Store::new(&engine, ());

    let memory_ty = MemoryType::new(1, None);
    let memory = Memory::new(&mut store, memory_ty.clone())?;

    let instance = Instance::new(&mut store, &module, &[memory.into()])?;
    let main = instance.get_func(&mut store, "main")
        .expect("`main` was not an exported function");
    let params = vec![];
    let mut results = vec![];
    println!("Opt level None: {:?}", main.call(
        &mut store,
        &params,
        &mut results
    ));
    for i in 65521..65536 {
        print!("{} ", memory.data(&store)[i]);
    }
    println!("");

    Ok(())
}

[package]
name = "wasmtime-wrapper"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
# wasmtime = "19.0.0"
wasmtime = { path = "../wasmtime/crates/wasmtime" } # commit: 6c5184809db3a92de4ee0c718c403bedc9a9ff4f (current latest, Date:   Thu Mar 21 18:24:49 2024 -0700)

Steps to reproduce

Compare the following executions:

cargo run --release
cargo run --release --target=riscv64gc-unknown-linux-gnu

QEMU run options (riscv64) I'm currently using is the following:

qemu-riscv64 -cpu rv64,v=true,vlen=128,vext_spec=v1.0,zba=true,zbb=true,zbs=true,zbc=true,zbkb=true,zcb=true,zicond=true -L /usr/riscv64-linux-gnu -E LD_LIBRARY_PATH=/usr/riscv64-linux-gnu/lib -E WASMTIME_TEST_NO_HOG_MEMORY=1 target/riscv64gc-unknown-linux-gnu/release/wasmtime-wrapper

Expected Results

RISC-V result should not leave side-effect when trapping. By looking at the spec, we can know that store instruction should check first if the memory address (offset + byte-width) is valid, then perform the memory operation.

Opt level None: Err(error while executing at wasm backtrace:
    0:   0x45 - <unknown>!<wasm function 0>

Caused by:
    0: memory fault at wasm address 0x10000 in linear memory of size 0x10000
    1: wasm trap: out of bounds memory access)
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0

Actual Results

RISC-V leaves memory side-effects when the program traps due to invalid memory address. The PoC code posted above leaves 15 bytes of 255 at the end of the memory. (indices from 65521 to 65535)

Opt level None: Err(error while executing at wasm backtrace:
    0:   0x45 - <unknown>!<wasm function 0>

Caused by:
    0: memory fault at wasm address 0x10000 in linear memory of size 0x10000
    1: wasm trap: out of bounds memory access)
255 255 255 255 255 255 255 255 255 255 255 255 255 255 255

Versions and Environment

wasmtime version
- commit: 6c5184809db3a92de4ee0c718c403bedc9a9ff4f(current latest, Date: Thu Mar 21 18:24:49 2024 -0700)
- However, also checked on v19.0.0
Operating system & architecture: Ubuntu 22.04.3 LTS, Arch: x86_64
QEMU version: qemu-riscv64 version 8.2.1 (v8.2.1)

Extra Info

Works fine on other architectures (x86_64, aarch64, s390x)
RISC-V is not the tier 1 platform, so releasing this bug as public (not a security bug)

afonso360 commented 6 months ago

If I'm not mistaken this is the same as https://github.com/bytecodealliance/wasmtime/issues/7237, right? We are writing to an unaligned address, crossing a page boundary and one of the pages triggers a fault.

I'm surprised that it works on AArch64 since that one is also supposed to be affected by this.

alexcrichton commented 6 months ago

I would agree with @afonso360, but thanks regardless @candymate!

I'm going to close this in favor of that issue, and I'll also drop a link to https://github.com/WebAssembly/design/issues/1490 which is upstream spec discussion on this topic too.

bytecodealliance / wasmtime