Crash [@ js::jit::JitFrameIterator::checkInvalidation] or Crash [@ js::jit::SnapshotIterator::SnapshotIterator] with weird address

VERIFIED FIXED in Firefox 37

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox36 wontfix, firefox37+ fixed, firefox38+ verified, firefox39+ verified, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [jsbugmon:][adv-main37+], crash signature)

Attachments

(3 attachments)

Reporter

Description

4 years ago
The following testcase crashes on mozilla-central revision eea6188b9b05 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --enable-debug, run with --thread-count=2):

for ( shiftpow = 0; shiftpow < 33; shiftpow++ )
  for ( addpow = 0; addpow < 33; addpow++ )
    Or( 0, 0 )
function ToInt32() {
  if (__lookupSetter__) {}
}
function ToInt32BitString( n ) {
  var b = "";
  for ( p = 30; p >= 0; p-- ) {
      1 * n >= Math.pow(2,p)
      b += 0; 
  }
  return b;
}
function Or(s, a) {
  ToInt32(a);
  var bs = ToInt32BitString(s);
  for ( var bit = 0; bit < bs.length; bit++ ) {}
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::jit::JitFrameIterator::checkInvalidation (this=<optimized out>, ionScriptOut=ionScriptOut@entry=0x7fffffffae30) at js/src/jit/JitFrames.cpp:163
163	    MOZ_ASSERT(ionScript->containsReturnAddress(returnAddr));
#0  js::jit::JitFrameIterator::checkInvalidation (this=<optimized out>, ionScriptOut=ionScriptOut@entry=0x7fffffffae30) at js/src/jit/JitFrames.cpp:163
#1  0x0000000000858a32 in js::jit::JitFrameIterator::ionScript (this=0x7fffffffb210) at js/src/jit/JitFrames.cpp:2261
#2  0x000000000085937a in js::jit::SnapshotIterator::SnapshotIterator (this=0x7fffffffaea0, iter=...) at js/src/jit/JitFrames.cpp:1707
#3  0x0000000000859e00 in js::jit::InlineFrameIterator::resetOn (this=0x7fffffffb240, iter=<optimized out>) at js/src/jit/JitFrames.cpp:2342
#4  0x0000000000865ba3 in js::jit::GetPcScript (cx=0x1a29b50, scriptRes=0x7fffffffb920, pcRes=0x0) at js/src/jit/JitFrames.cpp:1581
#5  0x00000000006213b1 in JSContext::currentScript (this=0x1a29b50, ppc=0x0, allowCrossCompartment=JSContext::ALLOW_CROSS_COMPARTMENT) at js/src/jscntxtinlines.h:462
#6  0x0000000000a09828 in JSContext::findVersion (this=0x1a29b50) at js/src/jscntxt.cpp:1124
#7  0x0000000000a099f0 in JS::CompileOptions::CompileOptions (this=0x7fffffffbbc0, cx=0x1a29b50, version=<optimized out>) at js/src/jsapi.cpp:3633
#8  0x0000000000ad5417 in js::CloneScript (cx=0x1a29b50, enclosingScope=0x0, fun=(JSFunction * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"], src=0x7ffff5646128, newKind=<optimized out>) at js/src/jsscript.cpp:3059
#9  0x0000000000686a16 in JSRuntime::cloneSelfHostedFunctionScript (this=<optimized out>, cx=0x1a29b50, name=..., targetFun=(JSFunction * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"]) at js/src/vm/SelfHosting.cpp:1340
#10 0x0000000000ad4018 in JSFunction::createScriptForLazilyInterpretedFunction (cx=0x1a29b50, fun=(JSFunction * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"]) at js/src/jsfun.cpp:1520
#11 0x000000000041c69f in JSFunction::getOrCreateScript (this=<optimized out>, cx=<optimized out>) at js/src/shell/../jsfun.h:291
#12 0x00000000005e6cc2 in JSObject::makeLazyGroup (cx=0x1a29b50, obj=(JSObject * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"]) at js/src/vm/ObjectGroup.cpp:296
#13 0x00000000004791c0 in JSObject::getGroup (this=(JSObject * const) 0x7ffff565f1c0 [object Function "__lookupSetter__"], cx=0x1a29b50) at js/src/jsobjinlines.h:96
#14 0x00000000007115f8 in js::HeapTypeSetKey::instantiate (this=0x1b311e8, cx=0x1a29b50) at js/src/vm/TypeInference.cpp:1136
#15 0x00000000007119bc in (anonymous namespace)::CompilerConstraintInstance<{anonymous}::ConstraintDataFreezeObjectFlags>::generateTypeConstraint(JSContext *, js::RecompileInfo) (this=0x1b311e0, cx=0x1a29b50, recompileInfo=...) at js/src/vm/TypeInference.cpp:1051
#16 0x00000000006f021e in js::FinishCompilation (cx=0x1a29b50, script=0x7ffff565d1f0, constraints=0x1b2e6d0, precompileInfo=0x7fffffffc350) at js/src/vm/TypeInference.cpp:1238
#17 0x00000000007ebdf4 in js::jit::CodeGenerator::link (this=0x7fffe4001400, cx=0x1a29b50, constraints=0x1b2e6d0) at js/src/jit/CodeGenerator.cpp:7395
#18 0x0000000000879382 in js::jit::LazyLinkTopActivation (cx=0x1a29b50) at js/src/jit/Ion.cpp:474
#19 0x00007ffff558e3f8 in ?? ()
#20 0x000000000000001f in ?? ()
#21 0x00007fffffffc680 in ?? ()
#22 0x0000000000000000 in ?? ()
rax	0x2	2
rbx	0x7ffff558e3b7	140737309631415
rcx	0xe0ff00000002e800	-2234066890152286208
rdx	0x0	0
rsi	0x7fffffffae30	140737488334384
rdi	0x7ffff5672140	140737310564672
rbp	0x7fffffffae20	140737488334368
rsp	0x7fffffffae10	140737488334352
r8	0x0	0
r9	0x0	0
r10	0x1a72648	27731528
r11	0x1b	27
r12	0x7fffffffae30	140737488334384
r13	0x7fffffffb920	140737488337184
r14	0x0	0
r15	0x7ffff558e3b7	140737309631415
rip	0x857631 <js::jit::JitFrameIterator::checkInvalidation(js::jit::IonScript**) const+97>
=> 0x857631 <js::jit::JitFrameIterator::checkInvalidation(js::jit::IonScript**) const+97>:	mov    (%rcx),%rdx
   0x857634 <js::jit::JitFrameIterator::checkInvalidation(js::jit::IonScript**) const+100>:	mov    (%rdx),%rax


The testcase reproduces intermittently, you will have to run it a lot of times (100 should suffice to get 2-3 crashes). Marking s-s and sec-critical due to the bad address involved here.
Reporter

Updated

4 years ago
Group: core-security
Nicolas, is this something you can look at?  Thanks.
Flags: needinfo?(nicolas.b.pierron)
Assignee

Comment 2

4 years ago
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Nicolas, is this something you can look at?  Thanks.

Yes, I already started to look at it.
Assignee

Comment 3

4 years ago
Ok, the problem is the following:

#18 0x0000000000879382 in js::jit::LazyLinkTopActivation (cx=0x1a29b50) at js/src/jit/Ion.cpp:474
#19 0x00007ffff558e3f8 in ?? ()

In JIT code, we call the LazyLink function, which is in charge of linking a function.  But at this time, we might GC, report errors and many more.  All of which might iterate the stack and query what is the status of the top-level frame.

When the Jit made the call, it is assumed that the function is jitted, and only the LazyLinkTopActivation will link and bounce to the compiled link.  In the mean time, the latest JIT frame makes a reference to a JSScript which is supposed to have an IonScript pointer, which is currently the LazyLinkStub.  Thus we fall under the wrong path of checkInvalidation.

The approach, I think, would be to mutate the JSFrame into an another frame until the code is properly linked.
Assignee

Updated

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

Comment 4

4 years ago
Tested ~11143 times, no longer able to reproduce it with this patch.

The problem is that we can iterate / look at the first frame which is one
the stack within the LazyLinkStub.  While under LazyLinkTopActivation, we
have the exit frame, and then the frame that we are trying to link.

Unfortunately, the return address of the exit frame targets the code of the
LazyLinkStub, and the IonScript might not be linked yet, when we are trying
to inspect the stack.

To avoid such unexpected invariant failure, this patch mutate the
JitFrameLayout to be used as an exit frame for the LazyLinkTopActivation
function.  Then, when we return from this function we mutate it back to be a
normal JitFrameLayout.  This way, we do not have to change any of the
invariants of the stack iteration.

On the other hand, we have to handle this frame properly in case of GCs by
marking the arguments of the revamp JitFrameLayout, as well as its callee
token.  We also have to keep track of the LazyLinkStub, as this was implicit
with the enterExitFrame previously.
Attachment #8573390 - Flags: review?(hv1989)
Reporter

Updated

4 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Reporter

Comment 5

4 years ago
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Comment on attachment 8573390 [details] [diff] [review]
LazyLinkStub stops making a call and reuses the parent frame.

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

Good find. My initial prototype was doing something similar, until if it was really necessary to have a new frame type.
Here I made a wrong assessment and thought it actually wasn't needed. Thanks for fixing this.
Attachment #8573390 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #6)
> Comment on attachment 8573390 [details] [diff] [review]
> LazyLinkStub stops making a call and reuses the parent frame.
> 
> Review of attachment 8573390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good find. My initial prototype was doing something similar, until if it was
> really necessary to have a new frame type.
> Here I made a wrong assessment and thought it actually wasn't needed. Thanks
> for fixing this.

s/until/until asked
Reporter

Updated

4 years ago
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Assignee

Comment 8

4 years ago
This issue is caused by Bug 1047346.
Assignee

Comment 9

4 years ago
I tried to find similar signatures on crash-stat (~140), I found other issues with checkInvalidation / SnapshotIterator which are not related to this change.

We should backport this patch, but I think this code is hard to exploit, as the following stack suggest, that we are mostly doing invalid reads.

#4  0x0000000000865ba3 in js::jit::GetPcScript (cx=0x1a29b50, scriptRes=0x7fffffffb920, pcRes=0x0) at js/src/jit/JitFrames.cpp:1581
#5  0x00000000006213b1 in JSContext::currentScript (this=0x1a29b50, ppc=0x0, allowCrossCompartment=JSContext::ALLOW_CROSS_COMPARTMENT) at js/src/jscntxtinlines.h:462
#6  0x0000000000a09828 in JSContext::findVersion (this=0x1a29b50) at js/src/jscntxt.cpp:1124

before returning one incorrect value.
Assignee

Comment 10

4 years ago
Comment on attachment 8573390 [details] [diff] [review]
LazyLinkStub stops making a call and reuses the parent frame.

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

Hard (see comment 9)

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

While the comments are still related to the stack, it does not specify under which context these can be used.

> Which older supported branches are affected by this flaw?

All after Gecko 35.

> If not all supported branches, which bug introduced the flaw?

Bug 1047346

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Not yet, the patch should be almost the same.

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

Tested locally ~11143 times (used to be intermittent ~1/100).
Attachment #8573390 - Flags: sec-approval?
Comment on attachment 8573390 [details] [diff] [review]
LazyLinkStub stops making a call and reuses the parent frame.

sec-approval+ for trunk. We'll want patches for Aurora and Beta as well.
Attachment #8573390 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/b83369a62aca
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Flags: in-testsuite?
(In reply to Al Billings [:abillings] from comment #11)
> sec-approval+ for trunk. We'll want patches for Aurora and Beta as well.

Moreover, I would highly prefer to take this fix in Beta 6, which gtb on Monday. If branch patches are required, can you get them ready by midday on Monday?
Flags: needinfo?(nicolas.b.pierron)
Reporter

Updated

4 years ago
Status: RESOLVED → VERIFIED
Reporter

Comment 15

4 years ago
JSBugMon: This bug has been automatically verified fixed.
Assignee

Comment 18

4 years ago
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #14)
> (In reply to Al Billings [:abillings] from comment #11)
> > sec-approval+ for trunk. We'll want patches for Aurora and Beta as well.
> 
> Moreover, I would highly prefer to take this fix in Beta 6, which gtb on
> Monday. If branch patches are required, can you get them ready by midday on
> Monday?

attachment 8578007 [details] [diff] [review] should cover beta.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8578007 [details] [diff] [review]
(beta) LazyLinkStub stops making a call and reuses the parent frame.

Approved for Beta and Aurora. Sec questionnaire is in comment 10.
Attachment #8578007 - Flags: approval-mozilla-beta+
Attachment #8578008 - Flags: approval-mozilla-aurora+
Reporter

Comment 22

4 years ago
JSBugMon: This bug has been automatically verified fixed on Fx38
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main37+]
Assignee

Updated

4 years ago
Depends on: 1150783

Updated

4 years ago
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.