Closed Bug 996883 Opened 10 years ago Closed 10 years ago

Crash [@ js::jit::Simulator::decodeType2]

Categories

(Core :: JavaScript Engine, defect)

ARM
Linux
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 + fixed
firefox30 + fixed
firefox31 + fixed
firefox-esr24 - wontfix
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: decoder, Assigned: dougc)

References

Details

(4 keywords, Whiteboard: [adv-main29+])

Crash Data

Attachments

(3 files)

The following testcase crashes on mozilla-central revision 1f932e462b84 (x86 ARM simulator build, run with --fuzzing-safe --ion-eager):


var x=0
var y=0;
x++;
var merge_type_maps_x = 0
var merge_type_maps_y = 0;
for (merge_type_maps_x = 0; x & 0xFFFF; ++merge_type_maps_x)
  ++merge_type_maps_y;
Crash trace:

Program received signal SIGSEGV, Segmentation fault.
js::jit::Simulator::decodeType2 (this=0x94d15e8, instr=0xf64a83b4) at js/src/jit/arm/Simulator-arm.cpp:2879
2879                set_register(rd, readW(addr, instr));
(gdb) x /i $pc
=> 0x83aeae3 <js::jit::Simulator::decodeType2(js::jit::SimInstruction*)+451>:   mov    (%ecx),%eax
(gdb) info reg ecx
ecx            0x7e1    2017
(gdb) bt 8
#0  js::jit::Simulator::decodeType2 (this=0x94d15e8, instr=0xf64a83b4) at js/src/jit/arm/Simulator-arm.cpp:2879
#1  0x083e977c in js::jit::Simulator::instructionDecode (this=0x94d15e8, instr=0xf64a83b4) at js/src/jit/arm/Simulator-arm.cpp:4021
#2  0x083ed018 in execute<false> (this=0x94d15e8) at js/src/jit/arm/Simulator-arm.cpp:4068
#3  js::jit::Simulator::callInternal (this=0x94d15e8, entry=0xf64af698 "\360O-\351\r\200\240\341\304\314\005\343L\311", <incomplete sequence \343>) at js/src/jit/arm/Simulator-arm.cpp:4150
#4  0x083ed28c in js::jit::Simulator::call (this=0x94d15e8, entry=0xf64af698 "\360O-\351\r\200\240\341\304\314\005\343L\311", <incomplete sequence \343>, argument_count=8)
    at js/src/jit/arm/Simulator-arm.cpp:4230
#5  0x082dcd4c in EnterIon (data=..., cx=0x94d1f68) at js/src/jit/Ion.cpp:2388
#6  js::jit::IonCannon (cx=0x94d1f68, state=...) at js/src/jit/Ion.cpp:2469
#7  0x085a5c1c in js::RunScript (cx=0x94d1f68, state=...) at js/src/vm/Interpreter.cpp:400
(More stack frames follow...)


Not sure if this is a simulator bug or not, marking s-s until triaged.
Flags: needinfo?(mrosenberg)
Live register was being destroyed by an incorrect destination in an ALU op_tst encoded as a negated pair of instructions.
Assignee: nobody → dtc-moz
Attachment #8407528 - Flags: review?(mrosenberg)
Flags: needinfo?(mrosenberg)
Attachment #8407528 - Flags: review?(mrosenberg) → review+
How far back does this bug go? We need to figure out which versions of b2g need this fix.
Keywords: sec-high
(In reply to Daniel Veditz [:dveditz] from comment #3)
> How far back does this bug go? We need to figure out which versions of b2g
> need this fix.

This appears to be a long standing bug introduced in:
http://hg.mozilla.org/mozilla-central/rev/46fe3e36c11b
Keywords: checkin-needed
Doug, since this affects more than nightly, this needs sec-approval first:

https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(dtc-moz)
Keywords: checkin-needed
Comment on attachment 8407528 [details] [diff] [review]
Correct alu ops encoded as a negated pair of instructions.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It is a systematic error that corrupts a register so it might be easy to use it to construct a reliable exploit.


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

Yes, the test case.  But this could be remove from the patch.


Which older supported branches are affected by this flaw?

From the introduction of the Ion Jit compiler.  b2g18 is affected.


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

Bug 731550.


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

It is a one line fix and should be trivial to backport.


How likely is this patch to cause regressions; how much testing does it need?

It is a low risk.  Tested locally and it passes the tbpl jit-tests.
Attachment #8407528 - Flags: sec-approval?
Flags: needinfo?(dtc-moz)
Is b2g18 really affected? I thought we did not have the JIT enabled on that branch? Or maybe it was disabled for "1.0" and enabled for "1.1" on the same branch. Who would know that for sure?

Since this is ARM-only we don't need to worry about the ESR-24 branch (no ESR for Fennec)
Sylvestre, Dan and I discussed this and we'd like to take this on Beta now (as well as Aurora and Trunk). This is literally a one line change and a test and it only affects ARM.
Flags: needinfo?(sledru)
OK Al. Could you fill the uplift request? Thanks
Flags: needinfo?(sledru)
Comment on attachment 8407528 [details] [diff] [review]
Correct alu ops encoded as a negated pair of instructions.

sec-approval+ for trunk. Please nominate a patch for Aurora and Beta as well and I'll approve them.
Attachment #8407528 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8300867f3f2

Stripped testcase from patch for landing at later date, and I also put in a neutral patch comment.
Flags: in-testsuite?
Target Milestone: --- → mozilla31
This patch strips off the testcase, for landing soon. Carrying forward r+.
Attachment #8409171 - Flags: review+
Splitting out the testcase for landing later.  Carrying forward r+.
Attachment #8409175 - Flags: review+
Douglas, not sure if you'd noticed, I've already landed the patch for you (minus the testcase).
Comment on attachment 8409171 [details] [diff] [review]
Correct alu ops encoded as a negated pair of instructions, excluding the testcase.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Erroneous compiled JS.  Possible exploit.
Testing completed (on m-c, etc.):  Passes tbpl jit-tests locally. Fixes the testcase.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: n/a
Attachment #8409171 - Flags: approval-mozilla-beta?
Attachment #8409171 - Flags: approval-mozilla-aurora?
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #14)
> Douglas, not sure if you'd noticed, I've already landed the patch for you
> (minus the testcase).

Ok, thanks.
Attachment #8409171 - Flags: approval-mozilla-beta?
Attachment #8409171 - Flags: approval-mozilla-beta+
Attachment #8409171 - Flags: approval-mozilla-aurora?
Attachment #8409171 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/d8300867f3f2
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Although I don't have a ARM computer here I did ran the testcase from comment 0 on nightly from 2014-04-15 Ubuntu 13.10 64bit but did not experienced any crash what so ever. Tried the same with Firefox 29RC and with same result. Since this is a ARM-only issue I don't think I can do something here.

Christian, can you please verify that the issue is fixed using latest RC build? http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/29.0-candidates/build1/
Flags: needinfo?(choller)
The test in comment 0 wasn't found on real ARM hardware, but using the ARM simulator builds. However, Fx29 doesn't have that simulator builtin yet, so I cannot verify it on that branch.
Whiteboard: [adv-main29+]
Flags: needinfo?(choller)
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security
You need to log in before you can comment on or make changes to this bug.