Closed Bug 1092947 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- wontfix
firefox35 + wontfix
firefox36 + verified
firefox-esr31 - wontfix

People

(Reporter: gkw, Assigned: h4writer)

Details

(5 keywords, Whiteboard: [adv-main36+])

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file 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.
Flags: needinfo?(jdemooij)
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.
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.
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.
(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?
I'll try this on Windows later today.
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
Kamil, do you mind testing this on your side? (Windows 7)
Flags: needinfo?(kamiljoz)
I can still reproduce with m-c rev 0c66a9fd9085 on Windows 7, but not on Windows 8.
And yes, I'm on MSVC 2013 on Windows 7 SP1.
Got Windows 7 here. Gonna investigate.
Assignee: nobody → hv1989
Flags: needinfo?(kamiljoz)
Flags: needinfo?(jdemooij)
I also reproduced the crash on my Win 7 VM machine using the information from comment #0. Using MSVC 2013 on Windows 7 SP1.
[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.
(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.
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #8520017 - Flags: review?(kvijayan)
> 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)
Flags: needinfo?(hv1989)
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.
Attachment #8520017 - Flags: review?(kvijayan)
lowering to sec-moderate based on comment 9
Flags: needinfo?(hv1989)
Sec-moderate doesn't qualify for ESR uplift, will track for 35/36 though.
Attached patch PatchSplinter Review
(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.
Attachment #8520017 - Attachment is obsolete: true
Attachment #8521423 - Flags: review?(kvijayan)
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.
Attachment #8521423 - Flags: review?(kvijayan) → review+
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.
Attachment #8521423 - Flags: sec-approval?
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."
Attachment #8521423 - Flags: sec-approval?
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.
Flags: needinfo?(hv1989)
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.
https://hg.mozilla.org/mozilla-central/rev/465b7f68072b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Group: javascript-core-security
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.
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.
Whiteboard: [adv-main36+]
Flags: needinfo?(hv1989)
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.