Assertion failure: base() != other.reg(), at js/src/jit/MoveResolver.h:109

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
critical
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla68
ARM64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67 fixed, firefox68 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments)

The following testcase crashes on mozilla-central revision 52b03bc34899 (build with --enable-debug --enable-more-deterministic --enable-simulator=arm64, run with --fuzzing-safe --no-threads --ion-eager --ion-sincos=on):

x = [0];
for (let i = 0; i < 1; ++i) {
    try {
        Math.cos(x[0])(Math.sin(x[0]));
    } catch (e) {}
}

Backtrace:

#0 js::jit::MoveOperand::aliases (this=<optimized out>, other=...) at js/src/jit/MoveResolver.h:108
#1 0x0000562b91fafc78 in js::jit::MoveResolver::addOrderedMove (this=0x7f2470396b88, move=...) at js/src/jit/MoveResolver.cpp:312
#2 0x0000562b91fab36a in js::jit::MoveResolver::resolve (this=0x7f2470396b88) at js/src/jit/MoveResolver.cpp:290
#3 0x0000562b91c92c77 in js::jit::MacroAssembler::callWithABIPre (this=0x7f2470396040, stackAdjust=<optimized out>, callFromWasm=<optimized out>) at js/src/jit/arm64/MacroAssembler-arm64.cpp:794
#4 0x0000562b91faaa0f in js::jit::MacroAssembler::callWithABINoProfiler (this=0x7f2470396040, fun=0x7f246fe940c8, result=js::jit::MoveOp::GENERAL, check=js::jit::CheckUnsafeCallWithABI::Check) at js/src/jit/MacroAssembler.cpp:3150
/snip

For detailed crash information, see attachment.

Likely another Ion on ARM64 issue.

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/afb2e1e1665f
user: Sean Stangl
date: Thu Mar 07 03:57:23 2019 +0000
summary: Bug 1528869 - Enable IonMonkey in the ARM64 shell, but keep it disabled in the browser. r=nbp

Setting needinfo? from Sean/Nicolas again.

Flags: needinfo?(sstangl)
Flags: needinfo?(nicolas.b.pierron)

This is a JS fuzzing bug and sdetar said the team will fix as many as they can in 67 but will not be able to get to all. Marking them fix-optional.

This still reproduces. Setting to P2 for tracking.

Priority: -- → P2

I can reproduce this issue as well. Taking.

Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)

For information: I have a one line work-around for this issue, but I want to understand this problem better before posting it, to make sure I am not missing other cases.

Flags: needinfo?(sstangl)
Group: javascript-core-security

This bug is one instance of a bug which which ""might"" be experienced on all platforms except MacOSX, since Bug 984018.

This bug is a MoveResolver bug which might read a non-normalized double from the stack, which contains stale data including forged data with a JS::Value pointer meant to read/write any memory. One can potentially use this bug to get random memory access.

The problem here is that MSinCos uses passABIArg to call js::math_sincos_impl. However passABIArg is unable to handle cycles with a random base pointer given as address, it has to be a register which is not used by the call convention as an argument register.

Here references to the Sin and Cos values are reserved and referenced by the params register which is freely allocated by the register allocator. This implies that if the params register matches the second argument register. This implies that the second reference would be equal to the first one, and that the js::math_sincos_impl function would not override the stack space. This stack space is then read back out of the stack pointer and stored as a double value, which is assumed to be in a canonical form (NaN). Making this double value an argument of a non-JIT-ed function will allow the non-JIT-ed function to interpret this non-cannonicalized double as a JS::Value, with a forged pointer to the stack, which might emulate an Array with wide bounds.

[…]

While double checking the generated code on all platform, I noticed that this is a non-issue because Sin and Cos address computations happen to be inverted, so even if there is a register override case, it would only be overwritten with the same value or not be reused.

The instruction order written here:
https://searchfox.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#9214-9218

Is inverted by:
https://searchfox.org/mozilla-central/source/js/src/jit/MoveResolver.cpp#252-255

And thus even if the code is incorrect, this is non-exploitable, and works as expected except for this assertion. (I am amazed …)
I will note that I have not seen any other instance of this issue while proof-reading the rest of uses of passABIArg.

Group: javascript-core-security
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1778699fa005
Prevent RegAlloc from allocating an argument register for a temp used in passAbiArg base operand. r=sstangl
Backout by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a10d3faab9e
Backed out changeset 1778699fa005 for spidermonkey bustages on a CLOSED TREE.
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b23278bf5294
Prevent RegAlloc from allocating an argument register for a temp used in passAbiArg base operand. r=sstangl
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Comment on attachment 9055501 [details]
Bug 1534492 - Prevent RegAlloc from allocating an argument register for a temp used in passAbiArg base operand.

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: None
  • User impact if declined: debug-builds crashes or fuzzing false-positive reports.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): force register allocation into picking registers which are avoiding assertions in debug builds.
  • String changes made/needed: None
Flags: needinfo?(nicolas.b.pierron)
Attachment #9055501 - Flags: approval-mozilla-beta?

Comment on attachment 9055501 [details]
Bug 1534492 - Prevent RegAlloc from allocating an argument register for a temp used in passAbiArg base operand.

Low risk assertion fix, minimal patch, approved for 67 beta 10, thanks.

Attachment #9055501 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.