1027359 - Differential Testing: Incorrect codegen for mod (%) on ARM sim

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

try {
    (function() {
        Array.buildPar(8, function(x) {
            if (x % 511 == 5) {
                n
            }
        })
    })()
    print(this)
} catch (e) {}

$ ./js-dbg-32-dm-ts-armSim-darwin-f78e532e8a10 --fuzzing-safe --ion-offthread-compile=off 1623.js
[object global]

$ ./js-dbg-32-dm-ts-armSim-darwin-f78e532e8a10 --fuzzing-safe --ion-offthread-compile=off --no-baseline 1623.js

(Tested this on 32-bit Mac js dbg threadsafe deterministic ARM-simulator shell off m-c rev f78e532e8a10)

My configure flags are:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-arm-simulator --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

Comment 1

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

I think this might go way back a few months to approximately rev 5ad5f92387a2, but I'm not sure. Setting needinfo? from Shu-yu for buildPar being involved.

I've checked that this only reproduces in an ARM-simulator build, not a regular x86 build off the same m-c rev.

Comment 2

[Shu-yu Guo [:shu]]

This isn't a PJS issue. The codegen for MacroAssemblerARM::ma_mod_mask is incorrect under the emulator. 

The input value is moved into ScratchRegister here: http://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm.cpp#949

And is bitwise anded with the mask here: http://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm.cpp#960

On the ARM emulator, we eventually end up here via ma_and: http://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm.cpp#376

The last link above shows that ScratchRegister is clobbered by the mask we're trying to bitwise and, so we always end up emitting code that's like |mask & mask|.

We probably need a second temp here.

Comment 3

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

(In reply to Shu-yu Guo [:shu] from comment #2)
> This isn't a PJS issue. The codegen for MacroAssemblerARM::ma_mod_mask is
> incorrect under the emulator. 

CC'ing our ARM gurus.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Fix incorrect codegen in ma_mod_mask.

See previous comment for explanation.

Comment 5

[Shu-yu Guo [:shu]]

Simpler differential test:

for (var i = 0; i < 2000; i++) {
  if (i % 511 == 5)
    print(i);
}

Try with --no-baseline and without.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8444912 [details] [diff] [review]
Fix incorrect codegen in ma_mod_mask.

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

Marty says there's a better solution than to introduce a new temp.

Comment 7

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8444912 [details] [diff] [review]
Fix incorrect codegen in ma_mod_mask.

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

After thinking about this over the weekend, I decided that this patch is preferable to adding a new instruction to the assembler / macro assembler.  I can do that as a separate pass later.
This code can be improved in the same two ways as the MIPS code, namely, only taking the second temp when it is necessary.  The other is to explicitly use the ScratchRegister to hold the mask, rather than loading it every time it is needed.  I can't imagine either of these having any impact outside of a microbenchmark.

Comment 8

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e63a5bebd3f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e63a5bebd3f

Comment 10

[Douglas Crosher [:dougc]]

Attachment: Backport to Aurora 32, also for Beta 31.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a long standing JS JIT code generation bug. Can not find the bug, but the code was added in this change set: https://hg.mozilla.org/releases/mozilla-beta/rev/fbb1bab307bf

User impact if declined: Incorrect computation or crashes.

Fix Landed on Version: 33.

Risk to taking this patch (and alternatives if risky): Low risk because it fixes broken code generation and there is a test case that this fixes and no regressions have been reported. An alternative might be to disable the failing code generation path.

String or UUID changes made by this patch: n/a

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8454387 [details] [diff] [review]
Backport to Aurora 32, also for Beta 31.

At this point, especially when this is not getting into FF31 and is not a security risk, it doesn't fit the criteria to land in ESR24 either.

Comment 12

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

Locking - :dougc sent mail about this.

Comment 14

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

Doug / Marty, any idea what sec-rating this should have? Also adding :jandem to the loop.

Comment 15

[Douglas Crosher [:dougc]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #14)
> Doug / Marty, any idea what sec-rating this should have? Also adding :jandem
> to the loop.

The potential security concern is if the compiler infers the range for the result and uses this to optimize away code such as a bounds check then if an unexpected result outside this range can be generated then it could be exploited. It's not immediately clear how the range of the result is affected by this bug or if the compiler infers a result range for this operation and uses it to optimize away code. Bad code generation just sticks out as a red flag.

Comment 16

[Douglas Crosher [:dougc]]

Attachment: Very low risk patch proposal for Beta 31.

In case it helps, here is an even lower risk patch for Beta 31 that simply disables the broken optimization path. The compiler will fall back to the regular well tested code generation path for the mod operation.

Comment 17

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8454976 [details] [diff] [review]
Very low risk patch proposal for Beta 31.

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

I usually just prepend |false &&| to the conditional, since there is a lower likelihood of changing semantics by accident, but this works just fine.

Comment 18

[Douglas Crosher [:dougc]]

Comment on attachment 8454976 [details] [diff] [review]
Very low risk patch proposal for Beta 31.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Difficult. Have not analysed it enough to know for sure that it is even possible to exploit.

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

Which older supported branches are affected by this flaw? This is a long standing JS JIT code generation bug.

If not all supported branches, which bug introduced the flaw? Can not find the bug, but the code was added in this change set: https://hg.mozilla.org/releases/mozilla-beta/rev/fbb1bab307bf

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

How likely is this patch to cause regressions; how much testing does it need? This is a very low risk patch because it simply disables an optimization path and the compiler will fall back to the regular well tested code generation path. It has been tested locally and here is a try build  https://tbpl.mozilla.org/?tree=Try&rev=ead3c9e4b9c5

Comment 19

[Douglas Crosher [:dougc]]

Comment on attachment 8454976 [details] [diff] [review]
Very low risk patch proposal for Beta 31.

Approval Request Comment
[Feature/regressing bug #]: Long standing issue.

[User impact if declined]: Errors in numerical computations. There is some unknown risk that this could be an exploitable security issue.

[Describe test coverage new/current, TBPL]: Local testing and a tbpl try run.

[Risks and why]: Very low because it is a small patch that simply disables a compiler optimization and the compiler will fall back to the regular well tested code generation path.

[String/UUID change made/needed]: n/a

Comment 20

[Sylvestre Ledru [:Sylvestre]]

FYI, the sec-approval will have to arrive very soon. The GTB for beta 10 / RC will be launched today.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8454387 [details] [diff] [review]
Backport to Aurora 32, also for Beta 31.

Aurora+ This is already on m-c and so can land immediately on Aurora.

As Sylvestre said, waiting on the sec rating to help with the evaluation of whether to take a late Beta fix.

Comment 22

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

This needs a rating in order to get sec-approval. Are the crashes that this causes exploitable? Do they only occur in the simulator but not normal builds?

Comment 23

[Marty Rosenberg [:mjrosenb]]

They occur in both normal and simulator builds.  I suspect there are no methods by which you can generate an out of bounds access.  Based on the bug in the mod_mask algorithm, the only incorrect result it can generate is 0, and it is also the case that 0 is always included in the computed range for the result of a modulus operation. Since the incorrect result is always in range, it can't be used to circumvent elided bounds checks, nor any other range-based analysis.  I may be missing something here.  Were there any crashes here, or just differences in output between compiled code, and the interpreter?

Comment 24

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

This was just a difference in output, but it may have been a seemingly-benign symptom hiding something more sinister, or may not.

Is this probably just sec-low then?

Comment 25

[Daniel Veditz [:dveditz]]

Changing ESR-24 from "wontfix" to "disabled" because there's no ESR-24 Fennec, and desktop doesn't ship ARM builds.

The main patch doesn't call that much attention to the problem, the code is pretty opaque. The "Fx31" patch, however, does focus attention on the optimization in question and I wouldn't land that one any time soon.

The B2G team may want this fix on 1.3 and later so the Fx31 simple fix would be good to land there, but let's wait until after fx31 ships.

Comment 26

[Marty Rosenberg [:mjrosenb]]

Based on my analysis, I don't see any way this can cause crashes.  Did you see any asserts with range-checking based on this bug? I realize that relying on the fuzzers to find out if a bug is exploitable is a bad idea, but I am of course much happier when my analysis doesn't have any contradicting evidence :-)

Comment 27

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

> Did you see any asserts with range-checking based on this bug?

Nope, I didn't. In any case, dveditz already rated this sec-moderate, so I guess we're okay here.

/me bounces the needinfos over back to sec-approval folks.

Comment 28

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

Comment on attachment 8454976 [details] [diff] [review]
Very low risk patch proposal for Beta 31.

As a sec-moderate, sec-approval is no longer necessary (it is used for high and critical rated issues).

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8454976 [details] [diff] [review]
Very low risk patch proposal for Beta 31.

[Triage Comment]

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8454976 [details] [diff] [review]
Very low risk patch proposal for Beta 31.

Actually, reading comment #25, we will take the full version of ESR 31.1.0

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/65af5e10bfe4

Comment 32

[Ivan Tsay (:ITsay)]

Add more Mozilla people.

Comment 33

[Ivan Tsay (:ITsay)]

Let's land this on v1.4.

Hi Marty,

Is it necessary to rebase the patch for 1.4 landing?

Comment 34

[Marty Rosenberg [:mjrosenb]]

It shouldn't be? the code around there hasn't changed much.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/2cb3c9d0c27b

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/7d6369ab5874

Comment 37

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

Sylvestre, I'm pretty sure policy is that tracking flags for ESR releases are set to the version we're fixing them in (per lsblakk) and not simply a '+'. Otherwise, we can't track which release fixed them.

Comment 38

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

Gary, would you mind verifying that this was fixed for Fx32? If you don't have time this week, let me know and I'll do it, but I thought I'd ask you first since you reported it. Either way. Thanks!

Comment 39

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

Verified on Fx32 rev f3195212d160 and m-c rev 18901d4f3edd. Also setting verified for Fx33 as the fix landed when Fx33 was m-c nightly.