Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x64: Segfaulting rip-relative load due to misalignment #4812

Closed
alexcrichton opened this issue Aug 30, 2022 · 6 comments · Fixed by #4826
Closed

x64: Segfaulting rip-relative load due to misalignment #4812

alexcrichton opened this issue Aug 30, 2022 · 6 comments · Fixed by #4826
Labels
fuzz-bug Bugs found by a fuzzer

Comments

@alexcrichton
Copy link
Member

Local bisection points to #4730 for this issue but that seems like it may also be just as likely to expose a preexisting issue. Reproducing this is somewhat nontrivial and wasm-tools shrink was pretty unsuccessful on this test case.

Using this input test case on an x86_64 machine this crash can be reproduced with:

$ cargo run -q \
  run foo.wasm \
  --wasm-features all \
  --enable-cranelift-nan-canonicalization \
  --wasm-timeout 20 \
  -g
zsh: segmentation fault  cargo run -q run $HOME/foo.wasm --wasm-features all  --wasm-timeout 20     -g

Note that this reproduction is quite specific to layout of code and the various options enabled here are all required with the input wasm above.

cc @elliottt

@elliottt
Copy link
Member

When I run the test case I get the following output:

% cargo run -q run ./foo.wasm --wasm-features all --enable-cranelift-nan-canonicalization --wasm-timeout 20 -g
Error: failed to run main module `./foo.wasm`

Caused by:
    0: failed to instantiate "./foo.wasm"
    1: wasm trap: integer overflow
       wasm backtrace:
           0:   0x61 - <unknown>!<wasm function 1>

@elliottt
Copy link
Member

I thought to try your example on cee4b20 (the commit where #4730 was merged) and it did indeed segfault. Could the behavior I'm seeing on main be due to the fix for #4761 that was fixed in #4790?

@alexcrichton
Copy link
Member Author

Oh you know it was probably something to do with --disable-cache where my bisection went awry. I always forget to pass that flag, alas!

In that case yeah it looks like this is fixed locally so I'll close.

@alexcrichton
Copy link
Member Author

According to oss-fuzz the original test case here still crashes and trying again locally I'm able to reproduce:

$ cargo run -q run --disable-cache testcase42.wasm --wasm-features all --cranelift-set wasmtime_linkopt_padding_between_functions=1
zsh: segmentation fault  cargo run -q run --disable-cache testcase42.wat --wasm-features all

with testcase42.wasm.gz as an input. This is probably related to the wasmtime_linkopt_padding_between_functions option which is pretty nonstandard and only there for the fuzzers, but it may mean that there's a missing alignment calculation for when a function starts?

@alexcrichton
Copy link
Member Author

Oh and to log, the reproduction for me locally is on 1a59b3e which should be main as of this writing

@elliottt
Copy link
Member

It's definitely coming from the fcopysign lowering:

% lldb -- cargo run -q run --disable-cache testcase42.wasm --wasm-features all --cranelift-set wasmtime_linkopt_padding_between_functions=1
...
Process 3067456 stopped                                                                                                        
* thread #1, name = 'wasmtime', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x00007ffff6c57b3b                               
->  0x7ffff6c57b3b: andnps 0x24ef(%rip), %xmm6                                                                                 
    0x7ffff6c57b42: andps  %xmm3, %xmm4                        
    0x7ffff6c57b45: orps   %xmm4, %xmm6                        
    0x7ffff6c57b48: movl   $0x4f000000, %edx         ; imm = 0x4F000000 
(lldb) disassemble -e 0x7ffff6c57b50 -s 0x7ffff6c57b0b
    0x7ffff6c57b0b: jge    0x7ffff6c57b13
    0x7ffff6c57b11: ud2    
    0x7ffff6c57b13: addl   $0x80000000, %edx         ; imm = 0x80000000 
    0x7ffff6c57b19: movl   %edx, %eax
    0x7ffff6c57b1b: cvtsi2ss %rax, %xmm3
    0x7ffff6c57b20: movl   $0x80000000, %eax         ; imm = 0x80000000 
    0x7ffff6c57b25: movd   %eax, %xmm4
    0x7ffff6c57b29: xorps  %xmm4, %xmm3
    0x7ffff6c57b2c: movl   $0x80000000, %r11d        ; imm = 0x80000000 
    0x7ffff6c57b32: movd   %r11d, %xmm4
    0x7ffff6c57b37: movdqa %xmm4, %xmm6
->  0x7ffff6c57b3b: andnps 0x24ef(%rip), %xmm6
    0x7ffff6c57b42: andps  %xmm3, %xmm4
    0x7ffff6c57b45: orps   %xmm4, %xmm6
    0x7ffff6c57b48: movl   $0x4f000000, %edx         ; imm = 0x4F000000 
(lldb) register read eax
     eax = 0x80000000
(lldb) register read rip
     rip = 0x00007ffff6c57b3b
Prelude> (0x24ef + 0x00007ffff6c57b3b) `divMod` 16
(8796083345922,10)

elliottt added a commit that referenced this issue Aug 31, 2022
Add a function_alignment function to the TargetIsa trait, and use it to align functions when generating objects. Additionally, collect the maximum alignment required for pc-relative constants in functions and pass that value out. Use the max of these two values when padding functions for alignment.

This fixes a bug on x86_64 where rip-relative loads to sse registers could cause a segfault, as functions weren't always guaranteed to be aligned to 16-byte addresses.

Fixes #4812
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzz-bug Bugs found by a fuzzer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants