944321 - --ion-check-range-analysis failure with Float32Array (SIGTRAP)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: stack

toString = (function() {
    function f() {
        Float32Array()[6] = 33554433
    }
    return f
})()
this + Object({
    e: this % 1
})

causes a SIGTRAP in js opt shell on m-c changeset 77b5c6edfe96 with --ion-check-range-analysis --ion-eager --ion-gvn=off

My configure flags are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c78a87a9aa66
user:        Nathan Froyd
date:        Tue Aug 27 16:32:44 2013 -0400
summary:     Bug 909922 - don't pragma pack ipc message headers; r=bent

but I think autoBisect is wrong here.

Adding :sunfish as well, as this seems to involve --ion-check-range-analysis. s-s because SIGTRAPs can be bad.

Comment 1

[Dan Gohman [:sunfish]]

There is a 33554432 where range analysis expects 33554433. 33554433 rounded to float32 is 33554432.

The SIGTRAP is just --ion-check-range-analysis' way of reporting an error. I should probably convert that to use something like what OsiPointRegisterCheckFailed() does so that it's not as mysterious.

Range analysis errors can theoretically be manipulated into bypassed bounds checks, so security-sensitive is prudent.

Comment 2

[Jesse Ruderman]

Simpler:
./js --ion-check-range-analysis --ion-eager --ion-gvn=off

function f() {
    (new Float32Array(1))[0] = 33554433;
}
f();
f();

Comment 3

[Dan Gohman [:sunfish]]

Attachment: disable-tofloat32-computerange.patch

The attached patch fixes the bug by applying the somewhat heavy hammer of disabling range analysis for MToFloat32, in case that's desired.

Also, this bug is a dup of 940638, which should probably also be security-sensitive (I don't seem to have the ability to change that myself).

Comment 4

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

> Also, this bug is a dup of 940638, which should probably also be
> security-sensitive (I don't seem to have the ability to change that myself).

Changed. (feel free to dupe as necessary)

Comment 6

[Jan de Mooij [:jandem]]

sunfish, can we land that patch or a more targeted fix?

Comment 7

[Dan Gohman [:sunfish]]

Attachment: disable-tofloat32-computerange.patch

A more targeted fix needs something like bug 939843, so I'll pursue the simple fix for now. Attached is a refreshed patch.

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8344610 [details] [diff] [review]
disable-tofloat32-computerange.patch

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

Someone would have to be very familiar with range analysis optimizations to pull this off, but it's presumably possible.

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 makes it clear where the bug is, but not how to exploit it.

Which older supported branches are affected by this flaw?

It's in mozilla-release, and probably others. As of this writing any branch which contains MToFloat32::computeRange (in RangeAnalysis.cpp) contains this flaw.

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?

Backports would be trivial to create, and very low-risk.

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

Very unlikely. The patch is a single deleted line, disabling an optimization.

Comment 9

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a29676c924d7

Comment 10

[Carsten Book [:Tomcat]]

landed on central https://hg.mozilla.org/mozilla-central/rev/a29676c924d7

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Dan, can you please nominate this for Aurora/Beta/b2g26 uplift?

Comment 12

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

Comment on attachment 8344610 [details] [diff] [review]
disable-tofloat32-computerange.patch

[Triage Comment]

Comment 13

[Dan Gohman [:sunfish]]

Attachment: disable-tofloat32-computerange-beta.patch

The original patch seems to have acquired aurora approval already. Attached is a version of the patch rebased to apply cleanly on mozilla-beta and b2g26, and corresponding nominations. Here's the approval form:

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 888109
User impact if declined: security-relevant issue goes unfixed
Testing completed: On mozilla-central for weeks
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c7f10c4e899
https://hg.mozilla.org/releases/mozilla-beta/rev/74fb6443cd2d

Comment 15

[Jason Smith [:jsmith]]

koi+ for sec-high bug

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8cafa32a4515

Comment 17

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

Confirmed SIGTRAP on m-c, 2013-11-28.
Verified fixed on beta, 2014-01-20.
Verified fixed on Aurora, 2014-01-21.
Verified fixed on m-c, 2014-01-21.