1455610 - Crash [@ js::jit::Simulator::instructionDecode] with wasm GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision cc0d7de218cb (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm, run with --fuzzing-safe --ion-offthread-compile=off --wasm-gc --arm-asm-nop-fill=1):

function wasmEvalText(str, imports) {
    let binary = wasmTextToBinary(str);
    m = new WebAssembly.Module(binary);
    return new WebAssembly.Instance(m, imports);
}
var f = wasmEvalText(`(module (func (result i32) (param i32)
  (block $0
   (block $1
    (block $2
     (block $default
      (br_table $0 $1 $2 $default (get_local 0))
     )
    )
   )
  )
  (return (i32.const 0))
) (export "" 0))`).exports[""];
f(0);


Backtrace:

received signal SIGSEGV, Segmentation fault.
js::jit::Simulator::instructionDecode (this=0xf6e58000, instr=0xeaffffff) at js/src/jit/arm/Simulator-arm.cpp:4832
#0  js::jit::Simulator::instructionDecode (this=0xf6e58000, instr=0xeaffffff) at js/src/jit/arm/Simulator-arm.cpp:4832
#1  0x085f82ea in js::jit::Simulator::execute<false> (this=0xf6e58000) at js/src/jit/arm/Simulator-arm.cpp:4911
#2  js::jit::Simulator::callInternal (this=0xf6e58000, entry=0x40dca138 "\377\377\377\352\004\340-\345\377\377\377\352\360\037-\351\377\377\377\352\020\212-\355\377\377\377", <incomplete sequence \352>) at js/src/jit/arm/Simulator-arm.cpp:4991
#3  0x085f8509 in js::jit::Simulator::call (this=<optimized out>, entry=<optimized out>, argument_count=<optimized out>) at js/src/jit/arm/Simulator-arm.cpp:5074
#4  0x089c51e7 in js::wasm::Instance::callExport (this=<optimized out>, cx=<optimized out>, funcIndex=<optimized out>, args=...) at js/src/wasm/WasmInstance.cpp:780
#5  0x089c5b5b in WasmCall (cx=<optimized out>, argc=1, vp=0xf561d058) at js/src/wasm/WasmJS.cpp:1233
#6  0x081ed899 in js::CallJSNative (cx=0xf6e1d800, native=0x89c5ab0 <WasmCall(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:280
[..]
#20 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9234
eax	0x1	1
ebx	0xf6e58000	-152731648
ecx	0x0	0
edx	0xf579f6a0	-176556384
esi	0xeaffffff	-352321537
edi	0x8e3aff4	149139444
ebp	0xffffbd28	4294950184
esp	0xffffbd00	4294950144
eip	0x85f432f <js::jit::Simulator::instructionDecode(js::jit::SimInstruction*)+47>
=> 0x85f432f <js::jit::Simulator::instructionDecode(js::jit::SimInstruction*)+47>:	mov    (%esi),%edx
   0x85f4331 <js::jit::Simulator::instructionDecode(js::jit::SimInstruction*)+49>:	mov    %edx,%eax


Not s-s because it requires --wasm-gc which is disabled right now.

Comment 1

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 2

[Lars T Hansen [:lth]]

I guess we knew we might run into some of these issues.  If it's only the debugger that's affected we probably shouldn't panic, but worth tracking these.

Comment 3

[Lars T Hansen [:lth]]

(Sigh, comment 2 pertained to another bug.  This one is probably more serious but doesn't appear to be about the GC being disabled.)

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

For what it's worth, I've started investigating: it seems the issue happens during the table switch, where we read the case entry and write it into pc; the case entry contains a NOP fill instruction (not an address!), so we jump to the encoding of the NOP instruction, hence crash. Somebody needs to investigate a bit more there, but there might be places which lack AutoForbidPools, maybe.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Trivial patch, just took some time to investigate. We should not allow nop fills in the jump table, obviously.

Comment 6

[Lars T Hansen [:lth]]

Hm, probably not this.  First, we need to worry about other platforms too (ARM64 in particular but I'm guessing also MIPS).  There is a nopfill concept on ARM64 though it's poorly developed right now, and it would be better for us if it didn't bite us when it gets better.  Second, Sean said (in Paris) that AutoFlushPools is severely limited on some platforms to a small instruction window, he guessed four instructions but didn't know for sure and I've not dug into it.  So using that here might not be safe?

What we really want is explicitly to disable NOP fill, I expect.  There must be a way to do that apart from AutoForbidPools, which has a lot of semantics associated with flushing and so on?

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: autoforbidnops.patch

So something more like this? (added ARM64 support; the MIPS assemblers don't use the buffer which inserts nops)

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 8970593 [details] [diff] [review]
autoforbidnops.patch

Review of attachment 8970593 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h
@@ +641,5 @@
>      // checking that instruction locations are correctly referenced and/or
>      // followed.
>      const uint32_t nopFillInst_;
>      const unsigned nopFill_;
> +

Looks like a spurious space at the beginning of the line.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +3835,5 @@
>          // constant pool entries.
>          masm.flush();
>  
> +#if defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_ARM64)
> +        // Prevent nop slides to appear in the jump table.

Are we sure "nop slide" is the word we want here (also in the name of the test case)?  To my mind, a "nop slide" is something that is created by careful manipulation of the compiler so as to create a sequence that can be used for an attack.

How about the more neutral "nop sequences" or even "fuzzing-requested nop sequences", since these nops only appear (I think) as a result of the shell's command line argument?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc311a047273
Prevent nop fills from happening in jump tables; r=lth

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/bc311a047273