Closed Bug 1296640 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- disabled
firefox51 --- disabled
firefox52 --- verified

People

(Reporter: decoder, Assigned: bbouvier)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update,testComment=6,origRev=66a77b9bfe5d])

Crash Data

Attachments

(2 files)

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.
Attached file Testcase
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Hi :decoder,
May I know if we have any updates for this bug or do you know who can we contact?
Flags: needinfo?(choller)
(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 :)
Flags: needinfo?(choller) → needinfo?(bbouvier)
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.
Flags: needinfo?(bbouvier) → needinfo?(choller)
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
bbouvier, can you try the test in the previous comment instead? That seems more reliable.
Flags: needinfo?(choller) → needinfo?(bbouvier)
Whiteboard: [jsbugmon:] → [jsbugmon:update,testComment=6,origRev=66a77b9bfe5d,bisect]
Attached patch fix.patchSplinter Review
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?
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Flags: needinfo?(bbouvier)
Attachment #8795318 - Flags: review?(jcoppeard)
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 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.
Attachment #8795318 - Flags: review?(jcoppeard) → review+
Flags: needinfo?(sphink)
(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.
Whiteboard: [jsbugmon:update,testComment=6,origRev=66a77b9bfe5d,bisect] → [jsbugmon:update,testComment=6,origRev=66a77b9bfe5d]
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.
See Also: → 1303780
Bisection is incorrect here, removing the blocking bug.
No longer blocks: 1297706
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.
Attachment #8795318 - Flags: sec-approval?
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
Attachment #8795318 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/6afa20e5a96d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
(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.
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.
Group: core-security-release
Created new bug 1332730 for the hazard analysis stuff here.
Flags: needinfo?(sphink)
You need to log in before you can comment on or make changes to this bug.