1092947 - Crash [@ EnterIon] or [@ js::jit::IonCannon]

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: stack

function z(f) {
    try {
        f()
    } catch (e) {}
}
s = newGlobal()
s["z"] = this["z"].bind()
evalcx("\
    function a() {\
        x()(x())(x())\
    }\
    function b() {\
        a(Math.imul())\
    }\
    function c() {\
        a(Math.imul());\
        a(Math.imul());\
        a()\
    }\
    function d() {\
        x(x()) * c() + (y > y)\
    }\
    function e(y) {\
        +c() ? x(d()) : b(y > 0)\
    }\
    z(a);\
    a = function() {};\
    function p() {\
        d(e())\
    }\
    z(p);\
    function q() {\
        e(q()(q(e()())))\
    }\
    q()\
", s)

crashes js opt shell on m-c changeset 0b81c10a9074 with --ion-eager --no-threads --ion-regalloc=backtracking at EnterIon with js::jit::IonCannon on the stack.

Debug configure options:

MAKE=mozmake AR=ar sh c://Users//mozillaadmin//trees//mozilla-central//js//src//configure --disable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --enable-gczeal --enable-debug-symbols --disable-tests

(This does not seem to affect debug shells.)

!exploitable mentions that this is exploitable, so setting s-s and assuming sec-critical.

autoBisect is running. Setting needinfo? from Jan as a start.

Comment 1

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

This seems to occur as far back as http://hg.mozilla.org/mozilla-central/rev/a459b02a9ca4 (and possibly even more before.) Unable to nail down an exact changeset though, due to an upgrade to MSVC 2013 on my machines.

Comment 2

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

I've checked that it goes even further back to https://hg.mozilla.org/mozilla-central/rev/1a1968da61b3 or Fx29, so ESR31 is also likely affected.

I couldn't seem to reproduce this on Mac, might be a Windows issue.

Comment 3

[Nicolas B. Pierron [:nbp]]

I failed to reproduce this issue on linux x86 / x64, optimized / debug builds of the shell.
I did not try the configuration flags listed on comment 0.

Comment 4

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

(In reply to Gary Kwong [:gkw] [:nth10sd] GMT+8 after Oct 9 from comment #2)
> I couldn't seem to reproduce this on Mac, might be a Windows issue.

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> I failed to reproduce this issue on linux x86 / x64, optimized / debug builds of the shell.

Anyone has any luck on Windows?

Comment 5

[Jan de Mooij [:jandem]]

I'll try this on Windows later today.

Comment 6

[Jan de Mooij [:jandem]]

Unfortunately I can't reproduce this. Windows 8, VS 2013. I tried the revision, configure flags and shell flags in comment 0 and ran the test at least 30 times.

$ ./js --ion-eager --no-threads --ion-regalloc=backtracking /c/dev/test.js
js_ReportOverRecursed called
c:/dev/test.js:9:381 InternalError: too much recursion

Comment 7

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

Kamil, do you mind testing this on your side? (Windows 7)

Comment 8

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

I can still reproduce with m-c rev 0c66a9fd9085 on Windows 7, but not on Windows 8.

Comment 9

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

And yes, I'm on MSVC 2013 on Windows 7 SP1.

Comment 10

[Hannes Verschore [:h4writer]]

Got Windows 7 here. Gonna investigate.

Comment 11

[Kamil Jozwiak [:kjozwiak]]

I also reproduced the crash on my Win 7 VM machine using the information from comment #0. Using MSVC 2013 on Windows 7 SP1.

Comment 12

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

[Tracking Requested - why for this release]:

!exploitable says this is exploitable, and we've got separate confirmation that this happens in Windows 7, though not Linux, Mac, nor Windows 8.

Comment 13

[Hannes Verschore [:h4writer]]

(In reply to Gary Kwong [:gkw] [:nth10sd] GMT+8 after Oct 9 from comment #12)
> [Tracking Requested - why for this release]:
> 
> !exploitable says this is exploitable, and we've got separate confirmation
> that this happens in Windows 7, though not Linux, Mac, nor Windows 8.

AFAIK, this is backtracking only, without asmjs. So should normally not be an issue in deployed builds. (Since they only run backtracking in asmjs).

The bug is caused due to stack overflow. We could make this a normal ionmonkey linking failure, which would result in non-ionmonkey code for that script, which would make this safe. Though currently looking into how we normally should have captured this.

Comment 14

[Hannes Verschore [:h4writer]]

Attachment: Patch

Ok, this is bug 995704, which wasn't fully fixed. There are 3 entries to Ionmonkey (w/wo argumentscheck and OSR). Only 1 of the the 3 were patched. This patches all three entries

Comment 15

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

> Ok, this is bug 995704, which wasn't fully fixed.

So what would be the sec rating of this bug then? (The one you mention is sec-low)

Comment 16

[Kannan Vijayan [:djvj]]

Comment on attachment 8520017 [details] [diff] [review]
Patch

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

Does the visitOsrEntry method need to be split up across all the methods?  The logic there is almost identical across all architectures.  The only change is that the stack-reservation methods are more involved for x86 and x64.

I'm wondering if it wouldn't be simpler to keep |visitOsrEntry| defined on the high-level CodeGenerator class, and define an arch-specific |masm.allocateFrame(size_t)| that does-the-right-thing for the architecture.

Lastly, it's concerning that we're adding a mandatory |jump| to all argcheck codepaths.  Why is it not OK to have the arg-check logic appear before the prologue logic, as it was before?

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +79,5 @@
> +    // Allocate the full frame for this function. See comment in
> +    // generatePrologue on why we are increasing by 1k at a time.
> +    uint32_t size = frameSize();
> +    if (size != 0) {
> +        while (size > 4096) {

Both prologue and OSR entry points use this frame-filling loop.  Would be good to put it into a helper method.

Comment 17

[Daniel Veditz [:dveditz]]

lowering to sec-moderate based on comment 9

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Sec-moderate doesn't qualify for ESR uplift, will track for 35/36 though.

Comment 19

[Hannes Verschore [:h4writer]]

Attachment: Patch

(In reply to Kannan Vijayan [:djvj] from comment #16)
> Comment on attachment 8520017 [details] [diff] [review]
> Patch
> 
> Review of attachment 8520017 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does the visitOsrEntry method need to be split up across all the methods? 
> The logic there is almost identical across all architectures.  The only
> change is that the stack-reservation methods are more involved for x86 and
> x64.
> 
> I'm wondering if it wouldn't be simpler to keep |visitOsrEntry| defined on
> the high-level CodeGenerator class, and define an arch-specific
> |masm.allocateFrame(size_t)| that does-the-right-thing for the architecture.

I found it easier to split visitOsrEntry. Though I understand the concern.
Btw isn't reserveStack already the arch-specific function you want to add? So I added the logic over there. That way reserveStack will now be fixed for all calls to it. (Since quick inspection shows that we use reserveStack in other places that might be vulnerable and hard to diagnose).

> Lastly, it's concerning that we're adding a mandatory |jump| to all argcheck
> codepaths.  Why is it not OK to have the arg-check logic appear before the
> prologue logic, as it was before?

The old code did something suboptimal:

> entryWithArgCheck:
>  ; generateArgCheck:
>    ; reserveStack (1)
>    ; do arg check
>    ; freeStack (3)
> entryWithoutArgCheck:
>  ; generatePrologue
>    ; reserveStack (2)
>  ; ...


So (1) only did the 1kb at a time allocation. So meaning entryWithArgCheck do (1) and (3) for no purpose. Now we also need the 1kb at a time allocation for (2).

> entryWithArgCheck:
>  ; generatePrologue
>    ; reserveStack
>    ; do arg check
> jump to start
> entryWithoutArgCheck:
>  ; generatePrologue
>    ; reserveStack
>  start:

As a result we only have to reserveStack once for every possible entry. We don't need to freeStack anymore ...

So I think it is better to have a jump here, instead of reservingStack twice and doing a freeStack.

Comment 20

[Kannan Vijayan [:djvj]]

Comment on attachment 8521423 [details] [diff] [review]
Patch

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

Ah, I didn't notice the removal of the freeStack call.  My bad.  I thought there was a way to have the argCheck logic be a strict prologue, and have a zero-overhead entry for both cases.

Looks good.

Comment 21

[Hannes Verschore [:h4writer]]

Comment on attachment 8521423 [details] [diff] [review]
Patch

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

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
There is a comment present which get's moved that might shed some light on this issue. Though this flaw only makes it possible to crash the browser if constructed correctly. It doesn't enable exploiting the browser.

Which older supported branches are affected by this flaw?
FF18+

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Fairly easy to backport. Patches should be very similar.

How likely is this patch to cause regressions; how much testing does it need?
Maybe some perf regressions, but should be unmeasurable. Should be safe to land and have low backout probability.

Comment 22

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

Comment on attachment 8521423 [details] [diff] [review]
Patch

As a sec-moderate, this doesn't need sec-approval (we only need it for high and critical rated issues). If you want to backport this to Aurora or Beta, we probably will want some justification on why this needs to be. Otherwise, go ahead and check it in and it can "ride the trains."

Comment 23

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d94b6a6469b4

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for jsreftest crashes on linux64.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fdc0c44b6c

https://treeherder.mozilla.org/logviewer.html#?job_id=3919596&repo=mozilla-inbound

Comment 25

[Hannes Verschore [:h4writer]]

Very stupid bug, sorry. I could reproduce and fix it locally. Since everything else in the push was green, this should be green now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cf79ca692cc

NI myself to try to create a testcase out of this. This should be better covered than with 1 test, which only tests one part of the functionality of big stacks.

Comment 26

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0edc83d3670f under suspicion of breaking windows jittest failures:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3973107&repo=mozilla-inbound

Comment 27

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

So, the jit failures popped up on another repo, and they went away just as quickly everywhere.

I relanded this patch in https://hg.mozilla.org/integration/mozilla-inbound/rev/465b7f68072b

Sorry for the churn in here.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/465b7f68072b

Comment 29

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original crash using m-c changeset 0b81c10a9074 with --ion-eager --no-threads --ion-regalloc=backtracking and the following configuration:

- MAKE=mozmake AR=ar sh ../configure --disable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --enable-gczeal --enable-debug-symbols --disable-tests

Once reproduced, I used m-c changeset cef590a6f946 and couldn't reproduce the original issue anymore. Ran the poc from comment #0 10 different times without the original crash occurring.

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

If this is low-risk please nominate for beta uplift approval before Mon Dec 29th otherwise we'll have to wait to ship this with FF36.