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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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;

Comment 1

[Christian Holler (:decoder)]

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.

Comment 2

[Douglas Crosher [:dougc]]

Attachment: Correct alu ops encoded as a negated pair of instructions.

Live register was being destroyed by an incorrect destination in an ALU op_tst encoded as a negated pair of instructions.

Comment 3

[Daniel Veditz [:dveditz]]

How far back does this bug go? We need to figure out which versions of b2g need this fix.

Comment 4

[Douglas Crosher [:dougc]]

(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

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Doug, since this affects more than nightly, this needs sec-approval first:

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 6

[Douglas Crosher [:dougc]]

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.

Comment 7

[Daniel Veditz [:dveditz]]

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)

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

OK Al. Could you fill the uplift request? Thanks

Comment 10

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 12

[Douglas Crosher [:dougc]]

Attachment: Correct alu ops encoded as a negated pair of instructions, excluding the testcase.

This patch strips off the testcase, for landing soon. Carrying forward r+.

Comment 13

[Douglas Crosher [:dougc]]

Attachment: Testcase for error in alu ops encoded as a negated pair of instructions.

Splitting out the testcase for landing later.  Carrying forward r+.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Douglas, not sure if you'd noticed, I've already landed the patch for you (minus the testcase).

Comment 15

[Douglas Crosher [:dougc]]

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

Comment 16

[Douglas Crosher [:dougc]]

(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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8300867f3f2

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4300612b17c5
https://hg.mozilla.org/releases/mozilla-beta/rev/9208db873dbf
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/6610ea5ece58
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8df7676cc23d

Comment 19

[Bogdan Maris, Desktop Test Engineering]

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/

Comment 20

[Christian Holler (:decoder)]

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.

Comment 21

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.