Closed Bug 1395100 Opened 2 years ago Closed 2 years ago

Assertion failure: cmpret == 0, at js/src/jit/arm/Simulator-arm.cpp:1074


(Core :: JavaScript Engine, defect, critical)

Not set



Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + fixed
firefox57 + fixed


(Reporter: gkw, Assigned: luke)


(Blocks 1 open bug)


(6 keywords, Whiteboard: [fuzzblocker][jsbugmon:update][post-critsmash-triage])


(2 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision db7f19e26e57 (build with --32 --enable-debug --enable-more-deterministic --enable-simulator=arm, run with --fuzzing-safe --no-threads --ion-eager --arm-sim-icache-checks --gc-zeal=14):

for (var i = 0; i < 1; ++i) {


#0  0x08550136 in js::jit::SimulatorProcess::checkICacheLocked (instr=0x56376f4c) at js/src/jit/arm/Simulator-arm.cpp:1074
#1  0x08552eea in js::jit::Simulator::instructionDecode (this=0xf7131000, instr=0x56376f4c) at js/src/jit/arm/Simulator-arm.cpp:4737
#2  0x08556a04 in js::jit::Simulator::execute<false> (this=0xf7131000) at js/src/jit/arm/Simulator-arm.cpp:4837
#3  js::jit::Simulator::callInternal (this=0xf7131000, entry=0x56356990 "\360O-\351\004\320M\342\020\212-\355\r\200\240\341h\220\235\345t\240\235", <incomplete sequence \345>) at js/src/jit/arm/Simulator-arm.cpp:4922
#4  0x08556ea9 in js::jit::Simulator::call (this=<optimized out>, entry=<optimized out>, argument_count=<optimized out>) at js/src/jit/arm/Simulator-arm.cpp:5005

For detailed crash information, see attachment.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Benjamin Bouvier
date:        Wed Aug 23 16:04:02 2017 +0200
summary:     Bug 1392105. r=luke

Luke, is bug 1392105 a likely regressor? (because :bbouvier is away)

Also setting s-s because bug 1392105 is s-s.
Blocks: 1392105
Group: javascript-core-security
Flags: needinfo?(luke)
Attached patch fix-iter-use (obsolete) — Splinter Review
Hah, so I started to re-read the regressing patch to see if I could spot the bug.  When I got to TraceOneDataRelocation() I decided to tidy it up not pass around a mutable Iter* everywhere b/c that's just a footgun with everyone calling the mutating iter->next() all over the place.  Turns out that fixed the bug and, on further inspection I figured out that the bug was that GetPtr32Target() mutates 'iter' and by using 'iter' later (at the flush calls) instead of the original 'ins', the wrong bytes were getting flushed.
Assignee: nobody → luke
Flags: needinfo?(luke)
Attachment #8902714 - Flags: review?(jdemooij)
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
Attached patch fix-iter-useSplinter Review
Also enables --gc-zeal flag in !JS_GC_ZEAL builds to fix opt builds.
Attachment #8902714 - Attachment is obsolete: true
Attachment #8902714 - Flags: review?(jdemooij)
Attachment #8903265 - Flags: review?(jdemooij)
Comment on attachment 8903265 [details] [diff] [review]

Review of attachment 8903265 [details] [diff] [review]:

Nice find, and definitely simpler this way.
Attachment #8903265 - Flags: review?(jdemooij) → review+
Bug 1392105 landed on beta so this one should be backported too.
Comment on attachment 8903265 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very hard, this involves icache invalidation only caught by the ARM simulator and possibly not exploitable in real hardware.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
They do, but again, it's just this theoretical icache invalidation.  Also the regressing patch only landed two days ago.

Which older supported branches are affected by this flaw?
beta, because uplift of the regressing patch.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Very easy to backport.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely: simple bug and simple fix
Attachment #8903265 - Flags: sec-approval?
Sec-approval+ for trunk.
We'll want a beta patch made and nominated as well.
Attachment #8903265 - Flags: sec-approval? → sec-approval+

This grafts cleanly to Beta as-is. Just needs an approval nomination when ready :)
Flags: in-testsuite+
Comment on attachment 8903265 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: bug 1392105
[User impact if declined]: probably nothing in practice (see comment 7)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes (manually in the shell)
[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?]: simple bug and simple fix
Attachment #8903265 - Flags: approval-mozilla-beta?
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Attachment #8903265 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker][jsbugmon:update][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.