Closed Bug 926627 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox25 --- unaffected
firefox26 + fixed
firefox27 + fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed

People

(Reporter: decoder, Assigned: sunfish)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, sec-critical, testcase)

Attachments

(3 files)

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)
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.
Flags: needinfo?(nicolas.b.pierron)
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.
Flags: needinfo?(nicolas.b.pierron)
Nicolas, do you have an opinion on the security severity of this bug?
Flags: needinfo?(nicolas.b.pierron)
(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
Flags: needinfo?(nicolas.b.pierron)
Keywords: sec-critical
Blocks: 930697
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.
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.
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 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 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. :)
Attachment #821992 - Flags: review+
(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 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.
Attachment #821992 - Flags: sec-approval?
Does this affect Firefox 25?
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 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.
Attachment #821992 - Flags: sec-approval? → sec-approval+
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.
Attachment #821992 - Flags: approval-mozilla-aurora?
Attachment #821992 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
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
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).
Normally, it would land on *inbound* first...
Assignee: general → sunfish
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Do we only land on inbound and not central directly anymore? I thought inbound was optional.
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
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.
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.