Closed Bug 865883 Opened 6 years ago Closed 6 years ago

IonMonkey: Assertion failure: Assertion failure: JSVAL_IS_DOUBLE_IMPL(l), at ./dist/include/js/Value.h:363

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox21 --- wontfix
firefox22 + fixed
firefox23 + verified
firefox-esr17 --- wontfix
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected

People

(Reporter: decoder, Assigned: shu)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-moderate, testcase, Whiteboard: [jsbugmon:update,ignore][adv-main22+])

Attachments

(2 files, 2 obsolete files)

The following testcase asserts on mozilla-central revision 690b5e0f6562 (run with --ion-eager):


function AddTestCase( description, expect, actual ) {}
function computeSetByRow(x, y) {
  var Cr = (x - 256) / scale + 0.407476;
  var Ci = false <= propertyIsEnumerable;
  var I = 0, R = 0, I2 = 0, R2 = 0;
  var n = 0;
  while ((R2+I2 < 2.0) && (n < 512)) {
    I = (R+R)*I+Ci;
    R = AddTestCase -- -I2+Cr;
    R2 = R*R;
    I2 = I*I;
  }
}
var scale = 10000*300;
var rows = 4;
var cols = 4;
r = new ParallelArray([rows, cols], computeSetByRow);
Marked s-s because I know that this assertion can be security-critical. Not sure if it's the case here.
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   129606:9aff2a52d88b
user:        Brian Hackett
date:        Mon Apr 22 20:22:30 2013 -0600
summary:     Bug 863518 - Consider types added by loop body when unboxing OSR values, r=dvander.

This iteration took 140.067 seconds to run.
I can't reproduce this neither on tip nor 690b5e0f6562. Tried with all combinations of opt and threadsafe.
Okay, this is a 32-bit only crash. Reproduces on tip still.
Somewhere along the line, a double incorrectly gets its upper 32bits overwritten with JSVAL_TAG_INT32, which causes incorrect unboxing into a double.
Attached file Slightly smaller testcase (obsolete) —
On 32bit, this bit of code gets generated for near the beginning of the function at crash.js:2:


[Codegen] instruction MoveGroup
[Codegen] movsd      %xmm0, 0xf8(%esp)            <-- Spilling xmm0 to 0xf8(%esp)
[Codegen] movl       %eax, 0xf4(%esp)
[Codegen] instruction CompareVM
[Codegen] push       %ebx
[Codegen] push       %edx
[Codegen] push       %ebp
[Codegen] push       %ecx
[Codegen] pushl      $0x1100
[Codegen] call       ((189))
[Codegen] #label     ((189))
[Codegen] instruction MoveGroup
[Codegen] instruction Nop
[Codegen] instruction OsiPoint
[Codegen] Encoding 11 of resume point 0xa60cca0's operands starting from 0
[Codegen] #label     ((189))
[Codegen] #label     ((189))
[Codegen] #label     ((189))
[Codegen] #label     ((189))
[Codegen] instruction Value
[Codegen] movl       $0xffffff81, %ecx
[Codegen] movl       $0x0, %eax
[Codegen] instruction Value
[Codegen] movl       $0xffffff81, %ebx
[Codegen] movl       $0x0, %edx
[Codegen] instruction Value
[Codegen] movl       $0xffffff81, %esi
[Codegen] movl       $0x0, %ebp
[Codegen] instruction MoveGroup
[Codegen] movl       %eax, 0xe8(%esp)
[Codegen] instruction Value
[Codegen] movl       $0xffffff81, %eax
[Codegen] movl       $0x0, %edi
[Codegen] instruction MoveGroup
[Codegen] movl       %ecx, 0xfc(%esp)             <-- Spilling %ecx *into the spill location of* %xmm0. 0xfc - 0xf8 == 4!
[Codegen] movl       0xe8(%esp), %ecx
[Codegen] movsd      0xf8(%esp), %xmm0            <-- %xmm0 is now hosed
I just looked into this. I think it's a problem with the MoveResolver. The MoveResolver handles cycles like this:

[xmm1 -> stack:d2]
[stack:d2 -> =xmm0]

..without problems. It guarantees xmm1 will be in stack:d2 after the MoveGroup and the original value of stack:d2 will be in xmm0.

However, here we have a move group like this:

[=edx -> stack:i1]
[stack:i6 -> stack:i2]
[stack:d2 -> =xmm0]

The move resolver should also treat this as a cycle, but that's a bit more complicated because there are both type and payload moves. The simplest fix is to never use the same slot for both doubles and nunbox values.

So this seems 32-bit only. Definitely security sensitive and not a new bug, but I'm not sure why we are only seeing it now. It may be easier to trigger now for some reason..
Attached patch fix (obsolete) — Splinter Review
Consider address overlap if JS_NUNBOX32.
Attachment #742605 - Flags: review?(jdemooij)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9d8977cbbfc6).
Per discussion on IRC with jandem, instead of checking for slot overlap in the cycle breaker, just don't reuse double slots for nunboxes.
Attachment #742206 - Attachment is obsolete: true
Attachment #742605 - Attachment is obsolete: true
Attachment #742605 - Flags: review?(jdemooij)
Attachment #744334 - Flags: review?(jdemooij)
Comment on attachment 744334 [details] [diff] [review]
fix v2 + testcase

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

Nice, thanks! It may be possible to exploit this bug on supported branches (still not sure why the fuzzers are only seeing it now though..) so we should:

(1) Not add the testcase
(2) Request sec-approval? on the patch before landing
(3) Request approval-mozilla-aurora? and approval-mozilla-beta?
Attachment #744334 - Flags: review?(jdemooij) → review+
Comment on attachment 744334 [details] [diff] [review]
fix v2 + testcase

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

Not easily. It's not clear how to game the regalloc to get a double and a nunbox slot in the same MoveGroup. In fact, in the meantime I think the test case in this bug has already stopped reproducing on tip.

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

Possibly, the check-in comment should be removed. The testcase will also be removed before committing.

Which older supported branches are affected by this flaw?

Aurora and Beta

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

Not sure.

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

This patch should work as is.

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

Unlikely to cause regressions.
Attachment #744334 - Flags: sec-approval?
Comment on attachment 744334 [details] [diff] [review]
fix v2 + testcase

sec-approval+ for M-C. You'll have to nominate the patch for Aurora and Beta and, given the timeline, it may not make Beta.
Attachment #744334 - Flags: sec-approval? → sec-approval+
Comment on attachment 744334 [details] [diff] [review]
fix v2 + testcase

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Exploitable security bug
Testing completed (on m-c, etc.): Waiting on m-c
Risk to taking this patch (and alternatives if risky): Low, regalloc change, not user visible.
String or IDL/UUID changes made by this patch:
Attachment #744334 - Flags: approval-mozilla-beta?
Attachment #744334 - Flags: approval-mozilla-aurora?
Attachment #744334 - Flags: approval-mozilla-beta?
Attachment #744334 - Flags: approval-mozilla-aurora?
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Exploitable security bug
Testing completed (on m-c, etc.): Waiting on m-c
Risk to taking this patch (and alternatives if risky): Low, regalloc change, not user visible.
String or IDL/UUID changes made by this patch:

(Ignore other patch. This patch has the test case and commit comment removed and is the one on m-i).
Attachment #744817 - Flags: approval-mozilla-beta?
Attachment #744817 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/10c6814d775e
Assignee: general → shu
Target Milestone: --- → mozilla23
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 744817 [details] [diff] [review]
patch for aurora and beta

This would have to go into our final beta, so we'll take this in Aurora 22 instead.
Attachment #744817 - Flags: approval-mozilla-beta?
Attachment #744817 - Flags: approval-mozilla-beta-
Attachment #744817 - Flags: approval-mozilla-aurora?
Attachment #744817 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main22+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.