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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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);

Comment 1

[Christian Holler (:decoder)]

Marked s-s because I know that this assertion can be security-critical. Not sure if it's the case here.

Comment 2

[Christian Holler (:decoder)]

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.

Comment 3

[Shu-yu Guo [:shu]]

I can't reproduce this neither on tip nor 690b5e0f6562. Tried with all combinations of opt and threadsafe.

Comment 4

[Shu-yu Guo [:shu]]

Okay, this is a 32-bit only crash. Reproduces on tip still.

Comment 5

[Shu-yu Guo [:shu]]

Somewhere along the line, a double incorrectly gets its upper 32bits overwritten with JSVAL_TAG_INT32, which causes incorrect unboxing into a double.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Slightly smaller testcase

Comment 7

[Shu-yu Guo [:shu]]

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

Comment 8

[Jan de Mooij [:jandem]]

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

Comment 9

[Shu-yu Guo [:shu]]

Attachment: fix

Consider address overlap if JS_NUNBOX32.

Comment 10

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9d8977cbbfc6).

Comment 11

[Shu-yu Guo [:shu]]

Attachment: fix v2 + testcase

Per discussion on IRC with jandem, instead of checking for slot overlap in the cycle breaker, just don't reuse double slots for nunboxes.

Comment 12

[Jan de Mooij [:jandem]]

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?

Comment 13

[Shu-yu Guo [:shu]]

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.

Comment 14

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

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.

Comment 15

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10c6814d775e

Comment 16

[Shu-yu Guo [:shu]]

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:

Comment 17

[Shu-yu Guo [:shu]]

Attachment: patch for aurora and beta

[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).

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/10c6814d775e

Comment 19

[Alex Keybl [:akeybl]]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef2db5f5aa7d

Comment 21

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.