Stream: git-wasmtime

Topic: wasmtime / issue #4890 x64: Incorrect out-of-bounds acces...

Wasmtime GitHub notifications bot (Sep 09 2022 at 18:13):

alexcrichton opened issue #4890:

This input module:

(module
  (func (param i32) (result f32)
    f32.const 0
    local.get 0
    f32.load offset=1
    f32.copysign
  )
  (memory 1)
  (export "" (func 0))
)

yields:

$ cargo run -q testcase0.shrunken.wat --invoke '' 0
warning: using `--invoke` with a function that takes arguments is experimental and may break in the future
Error: failed to run main module `testcase0.shrunken.wat`

Caused by:
    0: failed to invoke ``
    1: wasm trap: out of bounds memory access
       wasm backtrace:
           0:   0x2e - <unknown>!<wasm function 0>

but the trap reported here is incorrect because there is no out-of-bounds memory access in this module. Instead what's happening is that in the compile function:

0000000000000000 <_wasm_function_0>:
       0:       55                      push   %rbp
       1:       48 89 e5                mov    %rsp,%rbp
       4:       8b c2                   mov    %edx,%eax
       6:       48 8b 4f 50             mov    0x50(%rdi),%rcx
       a:       41 ba 00 00 00 80       mov    $0x80000000,%r10d
      10:       66 45 0f 6e da          movd   %r10d,%xmm11
      15:       66 41 0f 6f c3          movdqa %xmm11,%xmm0
      1a:       0f 55 05 0f 00 00 00    andnps 0xf(%rip),%xmm0        # 30 <_wasm_function_0+0x30>
      21:       44 0f 54 5c 01 01       andps  0x1(%rcx,%rax,1),%xmm11
      27:       41 0f 56 c3             orps   %xmm11,%xmm0
      2b:       48 89 ec                mov    %rbp,%rsp
      2e:       5d                      pop    %rbp
      2f:       c3                      retq

The instruction at 0x21 is segfaulting due to a misaligned address. The segfault is also registered as a trap point in Wasmtime since I believe this is a folding of the f32.load into the f32.copysign and so this could also segfault due to an out-of-bounds memory access.

Bisection reveals that this issue become a segfault in #4730 and then became a trap in #4790 (cc @elliottt). We'll want to be sure to backport the fix for this to the release-1.0.0 branch as well.

Wasmtime GitHub notifications bot (Sep 09 2022 at 18:13):

alexcrichton labeled issue #4890:

This input module:

(module
  (func (param i32) (result f32)
    f32.const 0
    local.get 0
    f32.load offset=1
    f32.copysign
  )
  (memory 1)
  (export "" (func 0))
)

yields:

$ cargo run -q testcase0.shrunken.wat --invoke '' 0
warning: using `--invoke` with a function that takes arguments is experimental and may break in the future
Error: failed to run main module `testcase0.shrunken.wat`

Caused by:
    0: failed to invoke ``
    1: wasm trap: out of bounds memory access
       wasm backtrace:
           0:   0x2e - <unknown>!<wasm function 0>

but the trap reported here is incorrect because there is no out-of-bounds memory access in this module. Instead what's happening is that in the compile function:

0000000000000000 <_wasm_function_0>:
       0:       55                      push   %rbp
       1:       48 89 e5                mov    %rsp,%rbp
       4:       8b c2                   mov    %edx,%eax
       6:       48 8b 4f 50             mov    0x50(%rdi),%rcx
       a:       41 ba 00 00 00 80       mov    $0x80000000,%r10d
      10:       66 45 0f 6e da          movd   %r10d,%xmm11
      15:       66 41 0f 6f c3          movdqa %xmm11,%xmm0
      1a:       0f 55 05 0f 00 00 00    andnps 0xf(%rip),%xmm0        # 30 <_wasm_function_0+0x30>
      21:       44 0f 54 5c 01 01       andps  0x1(%rcx,%rax,1),%xmm11
      27:       41 0f 56 c3             orps   %xmm11,%xmm0
      2b:       48 89 ec                mov    %rbp,%rsp
      2e:       5d                      pop    %rbp
      2f:       c3                      retq

The instruction at 0x21 is segfaulting due to a misaligned address. The segfault is also registered as a trap point in Wasmtime since I believe this is a folding of the f32.load into the f32.copysign and so this could also segfault due to an out-of-bounds memory access.

Bisection reveals that this issue become a segfault in #4730 and then became a trap in #4790 (cc @elliottt). We'll want to be sure to backport the fix for this to the release-1.0.0 branch as well.

Wasmtime GitHub notifications bot (Sep 09 2022 at 18:56):

elliottt commented on issue #4890:

It seems like we want a stronger condition in put_in_xmm_mem, as we need to know that it's a mergeable load and 16-byte aligned to merge the load:

https://github.com/bytecodealliance/wasmtime/blob/2986f6b0fffd576a2ba88a1fc6cb2db959a1f9e8/cranelift/codegen/src/isa/x64/lower/isle.rs#L169-L175

Wasmtime GitHub notifications bot (Sep 09 2022 at 20:06):

cfallin commented on issue #4890:

I sort of suspect that we will find most loads encountered by put_in_xmm_mem won't have any known alignment (greater than 1 byte); at least, in CLIF lowered from Wasm, that will be the case, since Wasm SIMD loads must work with unaligned addresses. The simplest fix here may just be to remove load-op merging in the XMM case, and always force to a register (i.e., delete exactly the lines you highlighted). Thoughts?

Wasmtime GitHub notifications bot (Sep 09 2022 at 20:17):

elliottt commented on issue #4890:

I was just experimenting with that fix and was going to suggest it :) I'll make a PR!

Wasmtime GitHub notifications bot (Sep 09 2022 at 20:28):

bjorn3 commented on issue #4890:

In cg_clif we do almost always know an alignment bigger than 1.

Wasmtime GitHub notifications bot (Sep 09 2022 at 20:52):

cfallin commented on issue #4890:

In cg_clif we do almost always know an alignment bigger than 1.

Fair enough; we can eventually look at adding this back, while respecting alignment, but IMHO we should fix the fuzzbug first in the most straightforward way, especially given we want to cherry-pick this to the 1.0 branch too :-)

Wasmtime GitHub notifications bot (Sep 09 2022 at 20:52):

elliottt commented on issue #4890:

Here's another option: in is_mergable_load we currently only require alignment of the load if the type being loaded is a vector type. Why don't we extend this to floating point types as well?

https://github.com/bytecodealliance/wasmtime/blob/2986f6b0fffd576a2ba88a1fc6cb2db959a1f9e8/cranelift/codegen/src/isa/x64/lower.rs#L107-L111

Wasmtime GitHub notifications bot (Sep 09 2022 at 20:53):

elliottt edited a comment on issue #4890:

Here's another option: in is_mergable_load we currently only require the address to be aligned if the type being loaded is a vector type. Why don't we extend this to floating point types as well?

https://github.com/bytecodealliance/wasmtime/blob/2986f6b0fffd576a2ba88a1fc6cb2db959a1f9e8/cranelift/codegen/src/isa/x64/lower.rs#L107-L111

Wasmtime GitHub notifications bot (Sep 09 2022 at 20:55):

elliottt commented on issue #4890:

We could also pass a flag to is_mergeable_load that indicates if the destination would be an xmm register, and use that in place of the check for load_ty.is_vector().

Wasmtime GitHub notifications bot (Sep 09 2022 at 20:56):

cfallin commented on issue #4890:

Possibly, yeah, though it's less obviously clear to me that this covers all cases (e.g. what if some lowering were to somehow load an integer-typed value into an XMM register?). The proximate cause here is that loads into XMM registers merged into SSE instructions need to be aligned, so I'd prefer to take the more risk-averse option for the initial fix and turn that path off altogether, I think...

Wasmtime GitHub notifications bot (Sep 09 2022 at 20:57):

elliottt commented on issue #4890:

For now, never merging the loads is obviously correct, and we can always revisit this later :)

Wasmtime GitHub notifications bot (Sep 12 2022 at 17:14):

elliottt closed issue #4890:

This input module:

(module
  (func (param i32) (result f32)
    f32.const 0
    local.get 0
    f32.load offset=1
    f32.copysign
  )
  (memory 1)
  (export "" (func 0))
)

yields:

$ cargo run -q testcase0.shrunken.wat --invoke '' 0
warning: using `--invoke` with a function that takes arguments is experimental and may break in the future
Error: failed to run main module `testcase0.shrunken.wat`

Caused by:
    0: failed to invoke ``
    1: wasm trap: out of bounds memory access
       wasm backtrace:
           0:   0x2e - <unknown>!<wasm function 0>

but the trap reported here is incorrect because there is no out-of-bounds memory access in this module. Instead what's happening is that in the compile function:

0000000000000000 <_wasm_function_0>:
       0:       55                      push   %rbp
       1:       48 89 e5                mov    %rsp,%rbp
       4:       8b c2                   mov    %edx,%eax
       6:       48 8b 4f 50             mov    0x50(%rdi),%rcx
       a:       41 ba 00 00 00 80       mov    $0x80000000,%r10d
      10:       66 45 0f 6e da          movd   %r10d,%xmm11
      15:       66 41 0f 6f c3          movdqa %xmm11,%xmm0
      1a:       0f 55 05 0f 00 00 00    andnps 0xf(%rip),%xmm0        # 30 <_wasm_function_0+0x30>
      21:       44 0f 54 5c 01 01       andps  0x1(%rcx,%rax,1),%xmm11
      27:       41 0f 56 c3             orps   %xmm11,%xmm0
      2b:       48 89 ec                mov    %rbp,%rsp
      2e:       5d                      pop    %rbp
      2f:       c3                      retq

The instruction at 0x21 is segfaulting due to a misaligned address. The segfault is also registered as a trap point in Wasmtime since I believe this is a folding of the f32.load into the f32.copysign and so this could also segfault due to an out-of-bounds memory access.

Bisection reveals that this issue become a segfault in #4730 and then became a trap in #4790 (cc @elliottt). We'll want to be sure to backport the fix for this to the release-1.0.0 branch as well.

Wasmtime GitHub notifications bot (Sep 12 2022 at 17:19):

abrown commented on issue #4890:

Perhaps there should be an issue to revisit this later? @elliottt brought up what seemed to me like good avenues for fixing this.

Wasmtime GitHub notifications bot (Sep 12 2022 at 18:17):

alexcrichton commented on issue #4890:

@elliottt also would you be up for backporting this to the release-1.0.0 branch? If not no worries and I can do that as well.

Wasmtime GitHub notifications bot (Sep 12 2022 at 18:20):

elliottt commented on issue #4890:

Absolutely, I'll make a PR today and send it to you for review.

Last updated: Nov 22 2024 at 16:03 UTC