1319888 - Assertion failure: containsPC(pc); from Ion resume points.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

This issue got reported by Jon in Bug 1315856 comment 8

This error comes from the js::jit::SplitCriticalEdges function which would add missing basic blocks to make the control flow graph simpler.  What happens is that we take block info from the predecessor, but we take the pc from the successor.  Which causes this mismatch while spewing the snapshot information (IONFLAGS=snapshots).

To have different block info, the split-edge must come from a function call located in an if condition and call a callee with multiple return path which should be inlined. Thus the pc of the successor would resumes at the end of a call-site, and before the condition.  This would cause a split-edge block to be inserted in between.

To exploit this issue one would have to get fallible instructions lifted in the split-edge block. When this fallible instruction fails with a bailout, the bailout will attempt to resume the execution with the frame of the callee, at the pc after the call-site of the caller.

This could be an exploitable security issue if we were to OSR into Ion, right after the bailout.

Note, I would have expected fuzzers to find such issues earlier, by noticing stack frame differences between the caller and the callee with this[1] assertion.

[1] http://searchfox.org/mozilla-central/source/js/src/jit/Recover.cpp#110

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: MBasicBlock::NewSplitEdge: use the successor info instead of the predecessor info. r=

This patch removes the CompileInfo argument, which removes the ability from
the caller to give the wrong one.

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 8813817 [details] [diff] [review]
MBasicBlock::NewSplitEdge: use the successor info instead of the predecessor info. r=

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

Good find!.

::: js/src/jit/MIRGraph.cpp
@@ +279,3 @@
>  {
>      MBasicBlock* split = nullptr;
>      if (!pred->pc()) {

Just for the sake of it, shouldn't we test succ->pc() in that case?

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> ::: js/src/jit/MIRGraph.cpp
> @@ +279,3 @@
> >  {
> >      MBasicBlock* split = nullptr;
> >      if (!pred->pc()) {
> 
> Just for the sake of it, shouldn't we test succ->pc() in that case?

I will do, but this should not matter as of today, as we do not mix wasm & ion & regex MIR graphs yet.

Comment 4

[Nicolas B. Pierron [:nbp]]

I will set this issue as sec-moderate, because at this gives the ability to restore the callee frame content as the caller frame content.

This would have to be combined with some other code which makes assumption of the JSValue type to be able to extract information such as pointers addresses / unexpected writes / read unexpected information.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8813817 [details] [diff] [review]
MBasicBlock::NewSplitEdge: use the successor info instead of the predecessor info. r=

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

Easily.

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

Based on the patch, an crashing-test can be constructed by following these steps:

1. Split-edge blocks are quite common in code which has control-flow such as:

  if (…) { … } /* no else */

2. The fact that this patch replaces "pref->info()" by "succ->info()"  suggests that they have to be different, and the only way to get different CompilerInfo is through inlining of functions.

3. The way of lifting instructions in the Split-edge block can be inspired in previous issues that we had there, which are opened today.  I would personally choose either the "Apply Type" phases which will add guards to enforce Phi types, or use the "Eager Simd Unbox" phase.

4. Make the code fail on one of the lifted guard instructions.

> Which older supported branches are affected by this flaw?

Since Firefox 48.

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

Caused by Bug 1186006.

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

The code should have remained almost identical, backporting the patch should be easy if needed.

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

Unlikely, Checking on TreeHerder should be enough.

Comment 6

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

Comment on attachment 8813817 [details] [diff] [review]
MBasicBlock::NewSplitEdge: use the successor info instead of the predecessor info. r=

Giving sec-approval+ though, technically, as a sec-moderate rated issue, it doesn't need sec-approval. Only critical and high bugs need it.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/13f67e1da316

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8813817 [details] [diff] [review]
MBasicBlock::NewSplitEdge: use the successor info instead of the predecessor info. r=

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1186006
[User impact if declined]: potentially bad crashes.
[Is this code covered by automated tests?]: Not yet, I'll check if I can minimize Bug 1315856 test case, to extract a simple test case for this one. (ni? :nbp)

[Has the fix been verified in Nightly?]: Tested locally, no impact on nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This make the source of information consistent, by taking all information from the same block (successor), instead of a mix of blocks (predecessor & successor).

[String changes made/needed]: N/A

Comment 9

[Nicolas B. Pierron [:nbp]]

The following testcase crashes on mozilla-central revision 908557c762f7 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --no-threads --disable-oom-functions --baseline-eager --ion-eager --ion-check-range-analysis, and the environment variable IONFLAGS=snapshots):

"".split(',');
for (var i = 0; i < 1; i++) { }


In the current test case, the problem of the above test case comes from the fact that String.prototype.split is being inline when we compile the for loop at the second entry.

We currently have no way to add environment variable for test cases.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8813817 [details] [diff] [review]
MBasicBlock::NewSplitEdge: use the successor info instead of the predecessor info. r=

Let's try this in aurora and beta. It should make it into beta 6 build today.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce9addb78dc1
https://hg.mozilla.org/releases/mozilla-beta/rev/2cf364e313e0