Closed Bug 1254167 Opened 4 years ago Closed 4 years ago

Crash [@ ??] or Assertion failure: !heapAccess->hasLengthCheck(), at asmjs/WasmSignalHandlers.cpp:609

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox45 --- unaffected
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- verified
firefox-esr38 --- unaffected
firefox-esr45 --- unaffected

People

(Reporter: decoder, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,bisect])

Crash Data

Attachments

(1 file, 1 obsolete file)

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

function asmCompile() {
    var f = Function.apply(null, arguments);
    return f;
}
function asmLink(f) {
    var ret = f.apply(null, Array.slice(arguments, 1));
    return ret;
}
var code = `
    "use asm";
    var HEAP32 = new stdlib.Int32Array(heap);
    var load = stdlib.Atomics.load;
    function add_sharedEv(i1) {
        i1 = i1 | 0;
        var i2 = 0;
        i2 = 305002 | 0;
        i1 = load(HEAP32, i2 >> 2) | 0;
    }
    return {add_sharedEv:add_sharedEv};
`;
var sab = new SharedArrayBuffer(65536);
var ffi = { _emscripten_asm_const_int: function (...i2) {} };
var m = asmCompile('stdlib', 'ffi', 'heap', code);
var {add_sharedEv} = asmLink(m, this, ffi, sab);
add_sharedEv(13812);



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7fe5031 in ?? ()
#0  0x00007ffff7fe5031 in ?? ()
#1  0x00007ffff6907800 in ?? ()
#2  0x00007ffff7fe508f in ?? ()
#3  0x00007fffffffc890 in ?? ()
#4  0x00007fffffffc910 in ?? ()
#5  0x00007fffffffc9d0 in ?? ()
#6  0x00007fffffffcc68 in ?? ()
#7  0x00007ffff6907800 in ?? ()
#8  0x00007ffff45862e0 in ?? ()
#9  0x00007ffff697c6a0 in ?? ()
#10 0x00000000004cea5b in js::wasm::Module::callExport (this=0x7ffdf44f0000, cx=0x7ffff6907800, exportIndex=<optimized out>, args=...) at js/src/asmjs/WasmModule.cpp:1341
#11 0x00000000004ceeed in WasmCall (cx=<optimized out>, argc=1, vp=0x7fffffffcc58) at js/src/asmjs/WasmModule.cpp:1092
#12 0x0000000000895d7e in CallJSNative (args=..., native=0x4cee60 <WasmCall(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff6907800) at js/src/jscntxtinlines.h:235
#13 js::Invoke (cx=cx@entry=0x7ffff6907800, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:478
#14 0x0000000000896789 in js::Invoke (cx=cx@entry=0x7ffff6907800, thisv=..., fval=..., argc=argc@entry=1, argv=argv@entry=0x7fffffffd088, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:530
#15 0x000000000053630d in js::jit::DoCallFallback (cx=0x7ffff6907800, frame=0x7fffffffd0c8, stub_=0x7ffff4588888, argc=1, vp=0x7fffffffd078, res=...) at js/src/jit/BaselineIC.cpp:6140
#16 0x00007ffff7ff0d54 in ?? ()
[...]
#48 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffc910	140737488341264
rcx	0x1	1
rdx	0x7ffff6929b60	140737330191200
rsi	0x7ffff7fe6000	140737354031104
rdi	0x35f4	13812
rbp	0x7fffffffc9d0	140737488341456
rsp	0x7fffffffc7c0	140737488340928
r8	0x7ffff7fe505e	140737354027102
r9	0x7fffffffffff	140737488355327
r10	0x7fffffffc890	140737488341136
r11	0x7fffffffc9d0	140737488341456
r12	0x7fffffffc910	140737488341264
r13	0x7ffff6907800	140737330051072
r14	0x7ffff45862e0	140737292821216
r15	0x7ffdf44f0000	140728702271488
rip	0x7ffff7fe5031	140737354027057
=> 0x7ffff7fe5031:	mov    0x4a768(%r15,%rax,1),%eax
   0x7ffff7fe5039:	xchg   %ax,%ax


This crash is really weird. The assertion appears to be a release assertion, I even get it in an optimized build. However, when I run under GDB, I get a bad crash on the heap instead. I'm marking this sec-critical because it looks like something's really going wrong here somewhere and it might depend on memory layout or similar things.
The reason why you're seeing two different things is that the first segfault in GDB is actually handled by a specific signal handler (continuing in GDB will lead you to the release assert), but then we crash inside this signal handler.
Interesting test case: because of EAA, the index is entirely folded in the offset. The offset is actually out of bounds, but nothing in the inline code seems to check for that. The two bounds checks succeed, but we get out of bounds, with an atomic access. This get caught by signal handling, which is not supposed to work with atomic accesses (as a comment says somewhere), hence the issue here.
bbouvier: so is the fix here to disable EAA for atomic accesses?
The atomics should not use signals for bounds checking, but there's room for confusion for loads and stores because that code is shared with the regular TA loads and stores.  There can be confusion because an atomic load/store should throw on OOB while a regular TA access should not.
(In reply to Luke Wagner [:luke] from comment #3)
> bbouvier: so is the fix here to disable EAA for atomic accesses?

I'm not sure, maybe the check could be generalized to handle this case? I think even non atomic loads with big offsets could run into this issue on x86 where we have explicit bounds checks.

Say we have a load at heap[ptr + offset]. We have a first check that ptr >u heapLength - offset (and a second one, out of line, checking that ptr <u -offset, though we never reach it). Here, offset >u heapLength, so it wraps around and the check never fails.

Ironically, I think the inverted check was done to avoid addition wraparound; here we have subtract wraparound.
Should we add a check that offset <u heapLength as well?
Haha! This actually doesn't happen on a non-atomic load on x86, because EAA doesn't fold the offset on x86 in this case, because MIRGenerator::foldableOffsetRange returns CheckedImmediateRange in this case.

But on x64, MIRGenerator::foldableOffsetRange doesn't take into account that the load/store may be atomic, and it returns the full range! Then the load/store gets folded, then trouble arises.

https://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIRGraph.cpp#151

So eventually, disabling EAA for atomic loads/stores sounds like a solution.
Attached patch folding.patch (obsolete) — Splinter Review
So something like that? It puts the atomic check at the lower level, so that other optimizations may not be missed, but I don't exactly recall what the CheckedImmediateRange means anymore...
Attachment #8728003 - Flags: feedback?(luke)
Comment on attachment 8728003 [details] [diff] [review]
folding.patch

I'd think so, but probably best to ask Dan.
Attachment #8728003 - Flags: feedback?(luke) → feedback?(sunfish)
Comment on attachment 8728003 [details] [diff] [review]
folding.patch

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

That looks like the right approach for now.
Attachment #8728003 - Flags: feedback?(sunfish) → feedback+
Opened follow-up bug 1254935, so that we can reference it from the code and remove all these checks later.
Assignee: nobody → bbouvier
Attachment #8728003 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8728366 - Flags: review?(sunfish)
Comment on attachment 8728366 [details] [diff] [review]
disable-eea.patch

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

Yes. This looks right. Thanks!
Attachment #8728366 - Flags: review?(sunfish) → review+
This can be triggered on all atomic loads/stores on x64, and EEA hasn't been disabled for those since the beginning, so all versions that can have access to SAB are affected => up to beta. Note that on aurora/beta, SAB are preffed off, at the moment.
Comment on attachment 8728366 [details] [diff] [review]
disable-eea.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't think it could: in the worst case, we hit the assertion mentioned by Decoder in comment 0, causing an insta crash of firefox.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes. Even without any comments in the code or changeset message, it doesn't seem too hard to imagine how to trigger the issue, by reading the code around.

Which older supported branches are affected by this flaw?
All branches which support atomic loads/stores: aurora, beta.

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't have backports, but they shouldn't be too hard to do: only the bits in WasmIonCompile.cpp should be removed, hopefully.

How likely is this patch to cause regressions; how much testing does it need?
Can't cause regressions. Running jstests/jit-tests on all platforms sounds enough.
Attachment #8728366 - Flags: sec-approval?
Comment on attachment 8728366 [details] [diff] [review]
disable-eea.patch

Approval Request Comment
[Feature/regressing bug #]: shared array buffers, I guess? It is a pretty old bug number that I can't find right now.
[User impact if declined]: possibility of insta crashes on nightly x64
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=481f50e68a52
[Risks and why]: pretty low risk: it's disabling an optimization for a feature which is disabled on aurora/beta by default, only available on nightly.
[String/UUID change made/needed]: n/a
Attachment #8728366 - Flags: approval-mozilla-beta?
Attachment #8728366 - Flags: approval-mozilla-aurora?
Why do we need to uplift this to beta if it's a crash only happening on nightly?  The security flaw is still there even if the feature is disabled?
Flags: needinfo?(bbouvier)
The feature can be turned on at run-time with a pref starting in FF46, so the code is in there.
Thanks Lars for answering. Liz, please see comment 16.
Flags: needinfo?(bbouvier)
Benjamin, does this impact esr38 and/or esr45? If yes, could you also nominate patches for uplift to those versions?
Flags: needinfo?(bbouvier)
Comment on attachment 8728366 [details] [diff] [review]
disable-eea.patch

sec-approval+
Attachment #8728366 - Flags: sec-approval? → sec-approval+
Marked ESR45 as affected because 45 is affected.
Ritu, Al: 45 is *not* affected, so no need to uplift there (comment 12). The only way to trigger this is to toggle usage of SharedArrayBuffers (accessible up to beta; this is preffed off by default and would have to be set on) and then run into the test case.
Flags: needinfo?(bbouvier)
(In reply to Benjamin Bouvier [:bbouvier] from comment #21)
> Ritu, Al: 45 is *not* affected, so no need to uplift there (comment 12). The
> only way to trigger this is to toggle usage of SharedArrayBuffers
> (accessible up to beta; this is preffed off by default and would have to be
> set on) and then run into the test case.

Thanks. What about esr38?
Flags: needinfo?(bbouvier)
If 45 is unaffected, I'm going to assume that earlier builds are unaffected as well.
(In reply to Ritu Kothari (:ritu) from comment #23)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #21)
> > Ritu, Al: 45 is *not* affected, so no need to uplift there (comment 12). The
> > only way to trigger this is to toggle usage of SharedArrayBuffers
> > (accessible up to beta; this is preffed off by default and would have to be
> > set on) and then run into the test case.
> 
> Thanks. What about esr38?

Unaffected, as says comment 24.
Flags: needinfo?(bbouvier)
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 8728366 [details] [diff] [review]
disable-eea.patch

Crash fix, verified on 48, OK to uplift to beta and aurora.
Attachment #8728366 - Flags: approval-mozilla-beta?
Attachment #8728366 - Flags: approval-mozilla-beta+
Attachment #8728366 - Flags: approval-mozilla-aurora?
Attachment #8728366 - Flags: approval-mozilla-aurora+
has problems uplifting to beta:

grafting 334826:5a5f9c53b6aa "Bug 1254167 - Don't allow folding to full range for atomic accesses; r=sunfish, a=lizzard" (tip)
merging js/src/asmjs/WasmIonCompile.cpp
merging js/src/jit-test/tests/asm.js/testAtomics.js
merging js/src/jit/EffectiveAddressAnalysis.cpp
merging js/src/jit/MIRGenerator.h
merging js/src/jit/MIRGraph.cpp
merging js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
warning: conflicts while merging js/src/asmjs/WasmIonCompile.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging js/src/jit/MIRGenerator.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging js/src/jit/MIRGraph.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(bbouvier)
The beta patch was even simpler: https://hg.mozilla.org/releases/mozilla-beta/rev/17ced754ca45
Flags: needinfo?(bbouvier)
(In reply to Benjamin Bouvier [:bbouvier] from comment #31)
> The beta patch was even simpler:
> https://hg.mozilla.org/releases/mozilla-beta/rev/17ced754ca45

setting flags
Group: javascript-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.