926627 - Assertion failure: a.isFloatReg(), at ../jit/shared/CodeGenerator-shared-inl.h:72

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 211337f7fb83 (run with --fuzzing-safe --ion-eager):


(function f(a) {
  if (b = a) {}
  f(Math.fround(-b) | 1)
})(0)

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Sean Stangl [:sstangl]]

This is a range analysis bug: the  "Truncate Doubles" phase is changing the output type of an MToFloat32 from MIRType_Float32 to MIRType_Int32. This causes the assignation of a GPR output to LInt32ToFloat32. Nicolas is more familiar with range analysis than I am, so setting needinfo.

Comment 3

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

One way to fix that would be to remove the range analysis on Floats:

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#l1105

Otherwise the fix would be to teach range analysis what is a Float, and thus adding a boolean flag to remember that this range was extracted out of a MIR instruction which had a MIRType_Float.  And add MaxTruncatableFloatExponent which would mirror MaxTruncatableExponent.

This should be able to deal with this kind of issues.  In the mean time, I think disabling range analysis on float is the safest move.

Comment 4

[Matt Wobensmith [:mwobensmith][:matt:]]

Nicolas, do you have an opinion on the security severity of this bug?

Comment 5

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Sean Stangl [:sstangl] from comment #2)
> This is a range analysis bug: the  "Truncate Doubles" phase is changing the
> output type of an MToFloat32 from MIRType_Float32 to MIRType_Int32. This
> causes the assignation of a GPR output to LInt32ToFloat32. Nicolas is more
> familiar with range analysis than I am, so setting needinfo.

Without this assertion, we do not prevent interpreting a general register as a float register.  This means that we would have a non-initialized general register which would be used by following instructions, and a corrupted float register.  In both case this can be used to work around bounds checks and do an out-of-bound read/write of arrays.

=> sec-critical

Comment 6

[Dan Gohman [:sunfish]]

MToFloat32::truncate does this:

    // We use the return type to flag that this MToFloat32 sould be replaced by a
    // MTruncateToInt32 when modifying the graph.
    setResultType(MIRType_Int32);

However, replacement by MTruncateToInt32 isn't happening, and I don't see anywhere where it's supposed to happen.

Comment 7

[Dan Gohman [:sunfish]]

Attachment: replace-truncated-float32.patch

The above MToFloat32 code is the same as the MToDouble code, and I found the place where this is done for MToDouble. Attached is a patch which does the same thing for MToFloat32. It fixes the crash -- the testcase goes into too much recursion, but that appears to be what's expected.

It's not clear to me that truncating MToFloat32 to int32 in this way is correct though, since float32 can't represent all the values of int32.

Comment 8

[Dan Gohman [:sunfish]]

Attachment: remove-float32-truncate.patch

And as an alternative, here is a patch which removes MToFloat32::truncate(). This also fixes the given testcase. This looses the potential optimization, but it's more clearly safe.

Comment 9

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 821981 [details] [diff] [review]
replace-truncated-float32.patch

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

::: js/src/jit/RangeAnalysis.cpp
@@ +2291,1 @@
>          truncated->replaceAllUsesWith(truncated->getOperand(0));

Indeed, we cannot safely truncate a float to an Int32 as the mantissa of a float does not provide the precision of an Int32.

Comment 10

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 821992 [details] [diff] [review]
remove-float32-truncate.patch

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

This sounds like the safest approach to me.
Thanks for addressing this issue. :)

Comment 11

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> Review of attachment 821992 [details] [diff] [review]:

Please note that you cannot land this patch on inbound without a sec-approval because this affect Gecko 26 (which might soon means beta)

Bug 888109 introduced this bug.

Comment 12

[Dan Gohman [:sunfish]]

Comment on attachment 821992 [details] [diff] [review]
remove-float32-truncate.patch

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

Pretty easily, if they know where to look. It probably allows someone to feed garbage values in registers, which presumably could get fed into any code the JIT emits.

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

The patch description just says it's removing code. It doesn't spell out why, and I didn't include the testcase, though someone might realize that the main reason for removing optimization code is because the optimization is incorrect, and the patch does show the area that they ought to look to find the bug.

Which older supported branches are affected by this flaw?

At least the Gecko 26 branch is affected.

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

Bug 888109.

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

I don't have backports. They would be easy to create though.

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

It's pretty unlikely to cause regressions. It just deletes code, removing an optimization.

Comment 13

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

Does this affect Firefox 25?

Comment 14

[Dan Gohman [:sunfish]]

No, it is not in the tree at https://hg.mozilla.org/releases/mozilla-release/file/FIREFOX_25_0_RELEASE .

It affects any tree containing the string "MToFloat32::truncate" (in RangeAnalysis.cpp).

Comment 15

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

Comment on attachment 821992 [details] [diff] [review]
remove-float32-truncate.patch

sec-approval+ for trunk. Please create an Aurora (Firefox 26 until next week) patch immediately so we can get this in before we branch to Beta. You'll need to nominate the patch for Aurora approval when you do.

Comment 16

[Dan Gohman [:sunfish]]

Comment on attachment 821992 [details] [diff] [review]
remove-float32-truncate.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Bug 888109.

User impact if declined: 

This sec-critical bug goes unfixed.

Testing completed (on m-c, etc.): 

I have run jit-tests and done a variety of other testing on my own machines.

Risk to taking this patch (and alternatives if risky): 

Pretty low. It's just deleting code, in a simple way, removing an optimization.

String or IDL/UUID changes made by this patch:

None.

Comment 17

[Dan Gohman [:sunfish]]

Ok. So this means I should land the patch on mozilla-central and mozilla-aurora now? This step of the security-critical bug approval process documentation isn't completely clear to me.

Comment 18

[Dan Gohman [:sunfish]]

Ok, I got clarification. Checked into mozilla-central and mozilla-aurora:

https://hg.mozilla.org/mozilla-central/rev/5c0ce0ca7801
https://hg.mozilla.org/releases/mozilla-aurora/rev/32dd7a496ce7

Comment 19

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

Yeah, normally you land on central first. If all is good, you then land on branches. You should also set the status flags to the appropriate branches to "fixed" when you do (27 and 26 in this instance).

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Normally, it would land on *inbound* first...

Comment 21

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

Do we only land on inbound and not central directly anymore? I thought inbound was optional.

Comment 22

[Dan Gohman [:sunfish]]

I believed my patch met the documented rules for mozilla-central [0]. Was I in error?

Documentation [0] [1] suggests that mozilla-inbound is a convenience layer for developers to land changes in mozilla-central without having to watch the tree. If developers are not supposed to land changes on mozilla-central directly, please update the documentation.

[0] https://wiki.mozilla.org/Tree_Rules#mozilla-central_.28Nightly_channel.29
[1] https://wiki.mozilla.org/Tree_Rules#mozilla-inbound
[2] https://wiki.mozilla.org/Mozilla-inbound#What_are_the_tree_rules_for_mozilla-inbound.3F

Comment 23

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

Dan, your understanding matches mine though, as a program manager, I don't generally have to land changes so I can be out of that loop.

Comment 24

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Basic gist is that we strongly prefer landing on integration branches to maintain m-c being in a known-good state as much as possible. While landing on m-c isn't expressly forbidden (yet), it is discouraged.