1353347 - [wasm] Crash [@ ??] with --wasm-always-baseline

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-inbound revision fbe76e704b6b+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug). To reproduce, you can run the following code in the JS shell (requires --wasm-always-baseline):

var data = os.file.readFile(file, 'binary');
new WebAssembly.Instance(new WebAssembly.Module(data.buffer));



Backtrace:

==18081==ERROR: AddressSanitizer: SEGV on unknown address 0x7f4a3524c369 (pc 0x2d389e848f80 bp 0x7ffda2a5d4b0 sp 0x7ffda2a5bc38 T0)
==18081==The signal is caused by a READ memory access.
    #0 0x2d389e848f7f  (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (<unknown module>) 
==18081==ABORTING


Marking s-s because it crashes on the heap with a random address.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

Ah interesting, in a standard debug build I see

test1353347.js:2:26 CompileError: at offset 8: binary version 0xd does not match expected version 0x1
Stack:
  @test1353347.js:2:26

Comment 3

[Lars T Hansen [:lth]]

Good grief, I can't even compile the shell on Ubuntu 16 any longer because "../../build/unix/gold/ld: --push-state: unknown option."

Comment 4

[Luke Wagner [:luke]]

The cset in comment 0 (fbe76e704b6b) was when we allowed both 0xd and 0x1.  However, the given binary also validates if you flip the version to 0x1 in a hex editor.  Using that I'm able to reproduce the ASAN failure with both the original fbe76e704b6b but also mozilla-inbound tip.

"Fortunately", it also reproduces without ASAN, just a plain --enable-debug --disable-optimize build produces a crash on x64 Linux.  The failing instruction is `movzwl (%r15,%rax,1),%ecx` where $rax is negative.  This should never be the case and in Ion we rely on the global invariant that all MIRType_Int32's have had their high 32 bits cleared (which is the default for most 32-bit x64 instructions, so it's mostly no work to preserve -- all the bugs I've seen have involved issuing 64-bit loads from a memory location where the high 32 bits are garbage instead of a 32-bit load which will zero the high 32 bits).

Also, FWIW, in bug 1353350 I also ran into a --push-state linker on Ubuntu 16 which I fixed by adding environment variables to the configure line:  CC=clang CXX=clang++ LLVM_SYMBOLIZER=/usr/lib/llvm-3.8/bin/llvm-symbolizer ../../src/configure --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug

Comment 5

[Lars T Hansen [:lth]]

I can repro in a straight debug build on Linux x64.  Last few instructions:

   0x18a1ad2cead:	movups 0x90c(%rip),%xmm1        # 0x18a1ad2d7c0
   0x18a1ad2ceb4:	roundsd $0x3,%xmm1,%xmm1
   0x18a1ad2ceba:	mov    $0xfffffffffffee369,%rax
=> 0x18a1ad2cec1:	movzwl (%r15,%rax,1),%ecx

The problem is that popMemoryAccess uses loadConstI32() to load a constant pointer into the pointer register, which sign extends it.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1353347-chop-to-32bits.patch

A silly mistake.

loadConstI32 from a Stk slot chops the value to 32 bits on 64-bit systems. 

loadConstI32 from an int32_t constant does not chop, but sign-extends instead.  But that is not necessary for correct operation, and in this particular case it is incorrect because the whole register is used by the memory access, not just the low 32 bits.  So make the operation chop.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8855751 [details] [diff] [review]
bug1353347-chop-to-32bits.patch

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

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +1151,5 @@
>          masm.mov(ImmWord((uint32_t)src.i32val() & 0xFFFFFFFFU), r);
>      }
>  
>      void loadConstI32(Register r, int32_t v) {
> +        masm.mov(ImmWord((uint32_t)v & 0xFFFFFFFFU), r);

Why is the cast to uint32_t not sufficient?  Or, to be more explicit, uint64_t(uint32_t(v))?

Comment 8

[Lars T Hansen [:lth]]

An excellent question, I just copied the phrase from the lines above, but the mask should be redundant.

Comment 9

[Lars T Hansen [:lth]]

Attachment: bug1353347-chop-to-32bits-v2.patch

Updates patch, carry luke's r+

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
No known exploit, unclear potential but may be possible to index below the wasm heap.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
None that I'm aware of

If not all supported branches, which bug introduced the flaw?
Bug 1316831

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
N/A

How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause regressions, we have good test coverage

Comment 10

[Luke Wagner [:luke]]

Since wasm baseline isn't enabled by default, I wouldn't think this is security sensitive at all.

Comment 11

[Daniel Veditz [:dveditz]]

Unhiding per comment 10, but leaving the security rating since this does affect anyone who enables it (which we presumably intend to do at some point?).

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 8855815 [details] [diff] [review]
bug1353347-chop-to-32bits-v2.patch

sec-approval=dveditz

Comment 13

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cf22ef8e29c1581d5dbd97714dcec6af521249b
Bug 1353347 - wasm baseline, properly chop int32 constants to 32 bits. r=luke

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0cf22ef8e29c

Comment 15

[Lars T Hansen [:lth]]

The debugger is slated to ship in FF54 and the debugger uses the baseline compiler.  We should uplift this fix to aurora.

Comment 16

[Lars T Hansen [:lth]]

Comment on attachment 8855815 [details] [diff] [review]
bug1353347-chop-to-32bits-v2.patch

Approval Request Comment
[Feature/Bug causing the regression]:
WebAssembly baseline compiler, also used for WebAssembly debugging

[User impact if declined]:
Possible security issue when wasm debugging is enabled in FF54 and later

[Is this code covered by automated tests?]:
Not at the moment, it's a fuzz bug

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Straightforward logic error with a straightforward fix, no controversy

[String changes made/needed]:
None

Comment 17

[Julien Cristau [:jcristau] (back April 22)]

Moving 54 back to affected per comment 15.  But closing again, since this is fixed in trunk.

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8855815 [details] [diff] [review]
bug1353347-chop-to-32bits-v2.patch

wasm baseline compiler fix for aurora54.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e53f7f1785eb