1296640 - Crash [@ BinaryScalar<js::Uint8x16, js::ShiftLeft>] with SIMD

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision fe895421dfbe (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --disable-oom-functions --ion-eager --ion-extra-checks --ion-offthread-compile=off --baseline-eager):

See attachment.


Backtrace:

 received signal SIGSEGV, Segmentation fault.
BinaryScalar<js::Uint8x16, js::ShiftLeft> (vp=<optimized out>, argc=<optimized out>, cx=0x7ffff695f000) at js/src/builtin/SIMD.cpp:1051
#0  BinaryScalar<js::Uint8x16, js::ShiftLeft> (vp=<optimized out>, argc=<optimized out>, cx=0x7ffff695f000) at js/src/builtin/SIMD.cpp:1051
#1  js::simd_uint8x16_shiftLeftByScalar (cx=0x7ffff695f000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/SIMD.cpp:1450
#2  0x00007ffff7e4acf5 in ?? ()
[...]
#6  0x0000000000000000 in ?? ()
rax	0x7ffff36d4060	140737277411424
rbx	0x7ffff695f000	140737330409472
rcx	0x15	21
rdx	0x7fffffff9b80	140737488329600
rsi	0x10	16
rdi	0x5	5
rbp	0x7fffffff9bb0	140737488329648
rsp	0x7fffffff9b40	140737488329536
r8	0x1	1
r9	0x7fffffff98e8	140737488328936
r10	0x7ffff694d680	140737330337408
r11	0x7ffff366aa30	140737276979760
r12	0x7ffff36d4050	140737277411408
r13	0x7fffffffa6c8	140737488332488
r14	0x7ffff36cce00	140737277382144
r15	0xf	15
rip	0xc92fff <js::simd_uint8x16_shiftLeftByScalar(JSContext*, unsigned int, JS::Value*)+191>
=> 0xc92fff <js::simd_uint8x16_shiftLeftByScalar(JSContext*, unsigned int, JS::Value*)+191>:	movdqu (%r12),%xmm2
   0xc93005 <js::simd_uint8x16_shiftLeftByScalar(JSContext*, unsigned int, JS::Value*)+197>:	pxor   %xmm0,%xmm0

Crash address looks weird, marking s-s.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Fuzzing Team]

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

Comment 3

[Gerry Chang [:gchang]]

Hi :decoder,
May I know if we have any updates for this bug or do you know who can we contact?

Comment 4

[Christian Holler (:decoder)]

(In reply to Gerry Chang [:gchang] from comment #3)
> Hi :decoder,
> May I know if we have any updates for this bug or do you know who can we
> contact?

The bug is supposed to be triaged by the JS team. Since it's SIMD, let's try to start with :bbouvier :)

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Can't reproduce, using the same configure flags and run flags, under x64. Is it intermittent? Is it on x64 or another platform? What version of gcc do you use?

gcc --version is 4.8.4 here.

Comment 6

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Crash [@ BinaryScalar<js::Int8x16, js::ShiftLeft>]
Build version: mozilla-central revision 66a77b9bfe5d
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize
Runtime options: --fuzzing-safe

Testcase:

gczeal(9, 2)
function simdToArray(v) {}
const INT8_MAX = Math.pow(2, 7) - 1;
const INT8_MIN = -Math.pow(2, 7);
function testBinaryScalarFunc(v, scalar, simdFunc, func) {
    var observed = simdToArray(simdFunc(v, scalar));
}
var Int8x16 = SIMD.Int8x16;
function lsh8(a, b) {}
var good = {
    valueOf: () => 21
};
for (var v of[Int8x16(INT8_MAX, INT8_MIN, INT8_MAX - 1, INT8_MIN + 1)]) {
    testBinaryScalarFunc(v, good, Int8x16.shiftLeftByScalar, lsh8);
}

Backtrace:

 received signal SIGSEGV, Segmentation fault.
BinaryScalar<js::Int8x16, js::ShiftLeft> (vp=<optimized out>, argc=<optimized out>, cx=0x7ffff695f000) at js/src/builtin/SIMD.cpp:1052
#0  BinaryScalar<js::Int8x16, js::ShiftLeft> (vp=<optimized out>, argc=<optimized out>, cx=0x7ffff695f000) at js/src/builtin/SIMD.cpp:1052
#1  js::simd_int8x16_shiftLeftByScalar (cx=cx@entry=0x7ffff695f000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/SIMD.cpp:1424
#2  0x0000000000af6719 in js::CallJSNative (cx=cx@entry=0x7ffff695f000, native=0xc9bf30 <js::simd_int8x16_shiftLeftByScalar(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:239
#3  0x0000000000ae6913 in js::InternalCallOrConstruct (cx=0x7ffff695f000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:458
#4  0x0000000000ada34f in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:509
#5  Interpret (cx=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:2922
#6  0x0000000000ae6765 in js::RunScript (cx=cx@entry=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:404
#7  0x0000000000aef76e in js::ExecuteKernel (cx=cx@entry=0x7ffff695f000, script=..., script@entry=..., envChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x0) at js/src/vm/Interpreter.cpp:685
#8  0x0000000000aefb20 in js::Execute (cx=cx@entry=0x7ffff695f000, script=..., script@entry=..., envChainArg=..., rval=rval@entry=0x0) at js/src/vm/Interpreter.cpp:718
#9  0x00000000008c5975 in ExecuteScript (cx=cx@entry=0x7ffff695f000, scope=scope@entry=..., script=script@entry=..., rval=rval@entry=0x0) at js/src/jsapi.cpp:4314
#10 0x00000000008c8a95 in JS_ExecuteScript (cx=cx@entry=0x7ffff695f000, scriptArg=scriptArg@entry=...) at js/src/jsapi.cpp:4347
#11 0x000000000042b2eb in RunFile (compileOnly=false, file=0x7ffff031d400, filename=<optimized out>, cx=0x7ffff695f000) at js/src/shell/js.cpp:641
#12 Process (cx=cx@entry=0x7ffff695f000, filename=<optimized out>, forceTTY=forceTTY@entry=false, kind=kind@entry=FileScript) at js/src/shell/js.cpp:1039
#13 0x000000000043b21f in ProcessArgs (op=0x7fffffffdac0, cx=0x7ffff695f000) at js/src/shell/js.cpp:6930
#14 Shell (envp=<optimized out>, op=0x7fffffffdac0, cx=0x7ffff695f000) at js/src/shell/js.cpp:7288
#15 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7663
rax	0x7ffff06c6060	140737227022432
rbx	0x7ffff695f000	140737330409472
rcx	0x15	21
rdx	0x7fffffffcc70	140737488342128
rsi	0x10	16
rdi	0x5	5
rbp	0x7fffffffccc0	140737488342208
rsp	0x7fffffffcc30	140737488342064
r8	0x1	1
r9	0x7fffffffc9d8	140737488341464
r10	0x7ffff03016d0	140737223071440
r11	0x1008	4104
r12	0x7fffffffcc50	140737488342096
r13	0x7ffff06c6050	140737227022416
r14	0x7ffff021a160	140737222123872
r15	0x7ffff021a168	140737222123880
rip	0xc9bfff <js::simd_int8x16_shiftLeftByScalar(JSContext*, unsigned int, JS::Value*)+207>
=> 0xc9bfff <js::simd_int8x16_shiftLeftByScalar(JSContext*, unsigned int, JS::Value*)+207>:	movdqu 0x0(%r13),%xmm2
   0xc9c005 <js::simd_int8x16_shiftLeftByScalar(JSContext*, unsigned int, JS::Value*)+213>:	pxor   %xmm0,%xmm0

Comment 7

[Christian Holler (:decoder)]

bbouvier, can you try the test in the previous comment instead? That seems more reliable.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Thank you, this test case is great.

Jon, choosing you to review this. What's happening is that we're taking a memory reference to a typed object's memory, then there's probably a moving GC happening (in ToInt32), and then the typed object's reference is stall (args moved!).

Here's a patch papering over the issue. I'd like to make a full audit of SIMD.cpp, because this issue could be present in other functions. But there has to be a way, using the static analysis, to detect this kind of issues more generally, right? How can I do so?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

This affects only SIMD, which has been enabled on Nightlies only for a while (and is implemented only on x86 and x64). Can we unhide?

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8795318 [details] [diff] [review]
fix.patch

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

Looks good!

> But there has to be a way, using the static analysis, to detect this kind of issues more generally, right?

Yep, the hazard analysis should be able to catch this.  I think you need to annotate your Elem types with JS_HAZ_GC_INVALIDATED, but Steve will know more.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> This affects only SIMD, which has been enabled on Nightlies only for a while
> (and is implemented only on x86 and x64). Can we unhide?

Please don't unhide it until a few days after the fix has landed on Nightly. Or needinfo me then and I can do it.

Comment 12

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/d1bf9267ba7d
user:        Jan de Mooij
date:        Tue Sep 13 14:14:32 2016 +0200
summary:     Bug 1297706 - Syntax parse arrow functions. r=shu

This iteration took 237.296 seconds to run.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Bisection is incorrect here, removing the blocking bug.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8795318 [details] [diff] [review]
fix.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Hardly but possible (the user should cause a GC at one precise time and have a full understanding of the variables positions in memory to be able to create an arbitrary JS value, leading to explointing).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, in the changeset message; but I could remove it, if needed.

Which older supported branches are affected by this flaw?
SIMD.js is needed for this issue, and SIMD.js is enabled on nightlies only; so only trunk is affected.

If not all supported branches, which bug introduced the flaw?
Possibly here for a long time.

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?
Test added in this patch, all the tests passing locally. A full audit of SIMD.cpp is to be done in addition to this.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8795318 [details] [diff] [review]
fix.patch

Cancelling sec-approval since it's SIMD.js (nightly only), per https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6afa20e5a96d

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6afa20e5a96d

Comment 18

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) from comment #10)
> > But there has to be a way, using the static analysis, to detect this kind of issues more generally, right?
> 
> Yep, the hazard analysis should be able to catch this.  I think you need to
> annotate your Elem types with JS_HAZ_GC_INVALIDATED, but Steve will know
> more.

Yes, we should totally expose these to the analysis.

The usual difficulty is that you have to have a C++ type to distinguish a pointer to GC memory vs pointers to anything else. You kind of have that here, with Elem. Except that the pointer that would get invalidated is the Elem*, not the Elem.

So you'd sort of like to mark the type Elem* as either JS_HAZ_GC_INVALIDATED or JS_HAZ_GC_POINTER (depending on whether you want to consider interior pointers to be GC pointers or not; I could argue either way, but the analysis doesn't care.) But you can only annotate bare C++ types, not pointers to them or whatever. So I think you'd need to tell a little white lie and claim that Elem is a JS_HAZ_GC_THING. That will make a pointer to an Elem be considered a GC pointers that could be invalidated on GC.

Ugh. But Elem is not a real type, it's a typedef to V::Elem, which is a template parameter... I'll try some things and see if I can get a try push out of this.

Comment 20

[Steve Fink [:sfink] [:s:]]

Oh, darn. V::Elem is generally something like int8_t. The compiler sees through typedefs for the most part, so this isn't going to work; we'd be declaring 'char' to be a GC type. :(

Maybe I'll see if we can make TypedObjectMemory return some sort of wrapper type instead of a plain Elem*, though I'm skeptical that we won't need it to degrade to a plain pointer immediately.

Or perhaps the big hammer -- requiring TypedObjectMemory to be given an AutoAssertOnGC&, and fixing up all callers.

This is the same sort of interior pointer pain we have with pointers to JSString contents.

Comment 21

[Steve Fink [:sfink] [:s:]]

Created new bug 1332730 for the hazard analysis stuff here.