Closed Bug 1027359 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86
All
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla33
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- verified
firefox33 --- verified
firefox34 --- verified
firefox-esr24 --- disabled
firefox-esr31 32+ fixed
b2g-v1.2 --- wontfix
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: gkw, Assigned: mjrosenb)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, sec-moderate, testcase, Whiteboard: [adv-main32+][adv-esr31.1+])

Attachments

(3 files)

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>
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.
Flags: needinfo?(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.
Flags: needinfo?(shu)
(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.
See previous comment for explanation.
Attachment #8444912 - Flags: review?(mrosenberg)
Simpler differential test:

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

Try with --no-baseline and without.
Assignee: nobody → mrosenberg
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.
Attachment #8444912 - Flags: review?(mrosenberg)
Summary: Differential Testing: Different output message involving buildPar → Differential Testing: Incorrect codegen for mod (%) on ARM sim
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.
Attachment #8444912 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4e63a5bebd3f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
[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
Attachment #8454387 - Flags: feedback?
Attachment #8454387 - Flags: approval-mozilla-esr24?
Attachment #8454387 - Flags: approval-mozilla-beta?
Attachment #8454387 - Flags: approval-mozilla-aurora?
Attachment #8454387 - Flags: feedback?
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.
Attachment #8454387 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24-
Locking - :dougc sent mail about this.
Group: core-security, javascript-core-security
Doug / Marty, any idea what sec-rating this should have? Also adding :jandem to the loop.
Flags: needinfo?(mrosenberg)
Flags: needinfo?(dtc-moz)
(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.
Flags: needinfo?(dtc-moz)
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.
Attachment #8454976 - Flags: review?(mrosenberg)
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.
Attachment #8454976 - Flags: review?(mrosenberg) → review+
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
Attachment #8454976 - Flags: sec-approval?
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
Attachment #8454976 - Flags: approval-mozilla-beta?
Group: javascript-core-security
FYI, the sec-approval will have to arrive very soon. The GTB for beta 10 / RC will be launched today.
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.
Attachment #8454387 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(dtc-moz)
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?
Flags: needinfo?(mrosenberg)
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?
Flags: needinfo?(mrosenberg)
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.
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 :-)
> 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.
Flags: needinfo?(mrosenberg)
Flags: needinfo?(dtc-moz)
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).
Attachment #8454976 - Flags: sec-approval?
Attachment #8454976 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8454976 [details] [diff] [review]
Very low risk patch proposal for Beta 31.

[Triage Comment]
Attachment #8454976 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
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
Attachment #8454976 - Flags: approval-mozilla-release+ → approval-mozilla-release-
Attachment #8454387 - Flags: approval-mozilla-beta? → approval-mozilla-esr31?
blocking-b2g: --- → 1.4?
Add more Mozilla people.
Let's land this on v1.4.

Hi Marty,

Is it necessary to rebase the patch for 1.4 landing?
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(mrosenberg)
It shouldn't be? the code around there hasn't changed much.
Flags: needinfo?(mrosenberg)
Attachment #8454387 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
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.
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!
Flags: needinfo?(gary)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(gary)
Whiteboard: [adv-main32+][adv-esr31.1+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.