Ion bug with RegExp{Prototype,Instance}OptimizableRaw and the exception handler

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: arai)

Tracking

({sec-critical})

unspecified
mozilla53
Points:
---
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+])

Attachments

(7 attachments, 2 obsolete attachments)

2.47 KB, patch
Details | Diff | Splinter Review
13.12 KB, patch
jandem
: review+
Details | Diff | Splinter Review
10.27 KB, patch
jandem
: review+
Details | Diff | Splinter Review
13.13 KB, patch
arai
: review+
Details | Diff | Splinter Review
10.27 KB, patch
arai
: review+
Details | Diff | Splinter Review
13.18 KB, patch
arai
: review+
Details | Diff | Splinter Review
9.08 KB, patch
arai
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Posted patch Trigger crashSplinter Review
decoder found some impossible to reproduce crashes under HandleException. I did some code auditing and I found the bug.

To reproduce, apply the patch, then run |jit_test.py --ion| and you should get a number of crashes.

Ion calls RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw and if they return false, it jumps to masm.exceptionLabel(). The problem is that the stack doesn't have an exit frame at that point so the exception handler sees a bogus stack.

The patch just makes us always fail there and adds a JS_GC call to HandleException to trigger the crash more reliably.

The best fix is probably to make these functions infallible - if we hit an OOM in this code we could just recover from it and return false to treat it as non-optimizable.
(Reporter)

Updated

2 years ago
Keywords: sec-critical
Priority: -- → P1
(Reporter)

Comment 1

2 years ago
This code was added in November 2015 / January 2016. I think there were some changes to this code recently, maybe it made this code fallible and it used to be infallible? arai, do you happen to know? Else I can take a look at this later.
Flags: needinfo?(arai.unmht)
(Reporter)

Comment 2

2 years ago
It's not the first time we see these bugs, we should probably start nulling out rt->jitTop in debug builds after VM calls. And we could add a gczeal mode to trigger GC from the exception handler and other weird places...
(In reply to Jan de Mooij [:jandem] from comment #2)
> It's not the first time we see these bugs, we should probably start nulling
> out rt->jitTop in debug builds after VM calls. And we could add a gczeal
> mode to trigger GC from the exception handler and other weird places...

This sounds like a great idea to me. Maybe this helps reproducing other intermittent bugs as well.
(Assignee)

Comment 4

2 years ago
RegExpPrototypeOptimizableRaw was fallible from the beginning, but recently I added some more fallible code.
it may fail in LookupOwnPropertyPure called on RegExp.prototype, called from GetOwnGetterPure/GetOwnNativeGetterPure/HasOwnDataPropertyPure.
will check how it fail actually.

RegExpInstanceOptimizableRaw is not fallible at all.
(Assignee)

Comment 5

2 years ago
maybe related to bug 1323108 that I fixed recently (regression in firefox52, fixed in firefox53)
before it gets fixed, RegExpPrototypeOptimizableRaw was calling GetGetterPure that may fail in more cases.

now I'm running jit-test with m-c 863c2b61bd27 + the patch, but I don't see crash so far.
will check with the revision before bug 1323108 patch later.

if bug 1323108 was the issue, it should be better uplifting it to firefox52.
(Assignee)

Comment 6

2 years ago
after running jit-test some more, I hit the crash with m-c 863c2b61bd27 + the patch.
so looks like the issue persists after bug 1323108.
(Assignee)

Comment 7

2 years ago
sorry, I misunderstood the patch.
with the patch the return value of RegExpPrototypeOptimizableRaw had no meaning :P
(Assignee)

Comment 8

2 years ago
I'll change RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.
any failure in LookupOwnPropertyPure can be ignored if we only interested into successful case.
we can just treat all failure case non-optimizable.

anyway, bug 1323108 needs uplift, to uplift this.
(Assignee)

Comment 9

2 years ago
Here's the patch.
Will ask review after running tests locally.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 10

2 years ago
Comment on attachment 8820510 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

Removed `result` out-parameter from RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw, and now they return "optimizabale or not" as bool.

the behavior difference is that now we treat the failure of GetOwnGetterPure/GetOwnNativeGetterPure/HasOwnDataPropertyPure as non-optimizable
instead of failure.

in JIT code, they now stores the call result to output register directly.
Attachment #8820510 - Flags: review?(jdemooij)
(Reporter)

Comment 11

2 years ago
Comment on attachment 8820510 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

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

Thanks for fixing this! I thought this affected ESR45 too, but I just checked and it landed after that, so that's great.

::: js/src/builtin/RegExp.cpp
@@ +1588,2 @@
>  {
>      JS::AutoCheckCannotGC nogc;

Nit: can also add AutoAssertNoPendingException aanpe(cx);

@@ +1680,2 @@
>  {
>      JS::AutoCheckCannotGC nogc;

And here.

::: js/src/jit/CodeGenerator.cpp
@@ -2121,5 @@
>  {
>      LRegExpPrototypeOptimizable* ins = ool->ins();
>      Register object = ToRegister(ins->object());
>      Register output = ToRegister(ins->output());
> -    Register temp = ToRegister(ins->temp());

We can now also remove the temp from LRegExpPrototypeOptimizable and LRegExpInstanceOptimizable.
Attachment #8820510 - Flags: review?(jdemooij)
(Assignee)

Comment 12

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #11)
> ::: js/src/jit/CodeGenerator.cpp
> @@ -2121,5 @@
> >  {
> >      LRegExpPrototypeOptimizable* ins = ool->ins();
> >      Register object = ToRegister(ins->object());
> >      Register output = ToRegister(ins->output());
> > -    Register temp = ToRegister(ins->temp());
> 
> We can now also remove the temp from LRegExpPrototypeOptimizable and
> LRegExpInstanceOptimizable.

temp is still used in non-OOL path.
https://dxr.mozilla.org/mozilla-central/rev/7083c0d30e75fc102c715887af9faec933e936f8/js/src/jit/CodeGenerator.cpp#2101
https://dxr.mozilla.org/mozilla-central/rev/7083c0d30e75fc102c715887af9faec933e936f8/js/src/jit/CodeGenerator.cpp#2170
(Assignee)

Comment 13

2 years ago
Added AutoAssertNoPendingException.
Attachment #8820510 - Attachment is obsolete: true
Attachment #8820728 - Flags: review?(jdemooij)
(Reporter)

Comment 14

2 years ago
Comment on attachment 8820728 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

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

Sorry, you're right.
Attachment #8820728 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 15

2 years ago
In case you're not familiar with security bugs: don't forget to request sec-approval before landing this :)
(Assignee)

Comment 16

2 years ago
Comment on attachment 8820728 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

Thank you for reviewing :)


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

I don't know how to hit this without the extra patch in comment #0.

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

Not so much.
At least it doesn't describe how it can be hit.

> Which older supported branches are affected by this flaw?

mozilla-release (firefox 50)

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

bug 887016 (from firefox 48)

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

not yet, but it would be easy.

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

less likely and automated test should be able to catch regression.
Attachment #8820728 - Flags: sec-approval?
Comment on attachment 8820728 [details] [diff] [review]
Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

sec-approval+ for trunk.
We'll want beta and aurora patches made and nominated once it lands on trunk and is good there.
Attachment #8820728 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3861ac69c78994cf478e07ce467880f881d7a45
Bug 1324810 - Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible. r=jandem a=abillings
(Assignee)

Comment 22

2 years ago
the issue on linux seems to be conflict with bug 1029245, that changes the gcc version.
now bisecting the issue on windows
(Assignee)

Comment 23

2 years ago
confirmed the issue locally on linux64, and looks like we need "and 0xff" for return value, like IsConstructor
the return register contains random higher bits.
(Assignee)

Comment 24

2 years ago
do you think we should always mask lower 8bits when getting bool return value of callWithABI to other register?
Flags: needinfo?(jdemooij)
(Assignee)

Comment 25

2 years ago
in that case maybe we should add masm.storeCallResultBool or something that does |masm.and32(Imm32(0xFF), output)| too.
(Reporter)

Comment 26

2 years ago
(In reply to Tooru Fujisawa [:arai] from comment #24)
> do you think we should always mask lower 8bits when getting bool return
> value of callWithABI to other register?

Yeah that makes sense. We could add storeBoolCallResult (and maybe rename the other one to storeNonBoolCallResult?) that does this?
(Reporter)

Updated

2 years ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 27

2 years ago
Renamed storeCallResult to storeCallWordResult, and added storeCallBoolResult that does "and 0xff".
and changed storeCallResult callsites to use either, depending on the return type or actual value range in "int" type.

storeCallBoolResult is defined in MacroAssembler-inl.h to call `and32`.

I'll update previous patch to use storeCallBoolResult and rename it to Part 1.
Attachment #8821076 - Flags: review?(jdemooij)
(Assignee)

Comment 28

2 years ago
how do we handle sec-approval for the case that the patch is updated later?
Attachment #8820728 - Attachment is obsolete: true
Attachment #8821078 - Flags: review?(jdemooij)
(Assignee)

Comment 29

2 years ago
apparently windows hit the issue at the point of try run :P
(Reporter)

Comment 30

2 years ago
Comment on attachment 8821076 [details] [diff] [review]
Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}.

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

::: js/src/irregexp/NativeRegExpMacroAssembler.cpp
@@ +446,5 @@
>  
>          masm.setupUnalignedABICall(temp0);
>          masm.passABIArg(temp1);
>          masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, GrowBacktrackStack));
> +        masm.storeCallBoolResult(temp0);

This returns an int so should use storeCallWordResult

@@ +865,5 @@
>          } else {
>              int (*fun)(const char16_t*, const char16_t*, size_t) = CaseInsensitiveCompareUCStrings;
>              masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, fun));
>          }
> +        masm.storeCallBoolResult(temp0);

And here.

::: js/src/jit/MacroAssembler.cpp
@@ +1978,5 @@
>      if (compilingWasm)
>          callWithABI(wasm::SymbolicAddress::ToInt32);
>      else
>          callWithABI(mozilla::BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
> +    storeCallBoolResult(dest);

This is calling the JS::ToInt32 version that returns int32_t I think, so we should use storeCallWordResult.

::: js/src/jit/SharedIC.cpp
@@ +1355,5 @@
>          masm.push(intReg);
>          masm.setupUnalignedABICall(scratchReg);
>          masm.passABIArg(FloatReg0, MoveOp::DOUBLE);
>          masm.callWithABI(mozilla::BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
> +        masm.storeCallBoolResult(scratchReg);

And here.

@@ +1508,5 @@
>          masm.bind(&truncateABICall);
>          masm.setupUnalignedABICall(scratchReg);
>          masm.passABIArg(FloatReg0, MoveOp::DOUBLE);
>          masm.callWithABI(BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
> +        masm.storeCallBoolResult(scratchReg);

And here.

::: js/src/jit/x86/CodeGenerator-x86.cpp
@@ +865,5 @@
>          if (gen->compilingWasm())
>              masm.callWithABI(wasm::SymbolicAddress::ToInt32);
>          else
>              masm.callWithABI(BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
> +        masm.storeCallBoolResult(output);

And here.

@@ +950,5 @@
>              masm.callWithABI(wasm::SymbolicAddress::ToInt32);
>          else
>              masm.callWithABI(BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32));
>  
> +        masm.storeCallBoolResult(output);

And here.
Attachment #8821076 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 31

2 years ago
Comment on attachment 8821078 [details] [diff] [review]
Part 1: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible.

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

It's fine to land these patches, the changes are pretty minor.
Attachment #8821078 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 32

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c11875a3653b74939b44980d50bb6c05386d46
Bug 1324810 - Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/12f897d39a026697efbe0f8a8f4c734dbbefde7e
Bug 1324810 - Part 1: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible. r=jandem a=abillings
https://hg.mozilla.org/mozilla-central/rev/31c11875a365
https://hg.mozilla.org/mozilla-central/rev/12f897d39a02
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 34

2 years ago
Approval Request Comment
> [Feature/Bug causing the regression]

bug 887016

> [User impact if declined]

possible crash

> [Is this code covered by automated tests?]

no
we don't have any reproducible test for the crash.

> [Has the fix been verified in Nightly?]

partly
at least it's working, but there's no dedicated test that verifies the issue

> [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?]

less risky

> [Why is the change risky/not risky?]

It changes only the failure path, to take less risky path than previously taken dangerous path.

> [String changes made/needed]

none
Attachment #8821442 - Flags: review+
Attachment #8821442 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 36

2 years ago
Approval Request Comment
> [Feature/Bug causing the regression]

bug 887016

> [User impact if declined]

possible crash

> [Is this code covered by automated tests?]

no
we don't have any reproducible test for the crash.

> [Has the fix been verified in Nightly?]

partly
at least it's working, but there's no dedicated test that verifies the issue

> [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?]

less risky

> [Why is the change risky/not risky?]

It changes only the failure path, to take less risky path than previously taken dangerous path.

> [String changes made/needed]

none
Attachment #8821444 - Flags: review+
Attachment #8821444 - Flags: approval-mozilla-beta?
(Assignee)

Comment 38

2 years ago
patches for mozilla-aurora has no change from mozilla-central.

patches for mozilla-beta has some change:
  Part 0: only surrounding code change
  Part 1: RegExpPrototypeOptimizableRaw checks less properties
Group: javascript-core-security → core-security-release
Comment on attachment 8821442 [details] [diff] [review]
(mozilla-aurora) Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}. r=jandem

Sec-critical fix, should prevent crashes, OK to land on aurora.
Attachment #8821442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8821443 [details] [diff] [review]
(mozilla-aurora) Part 1: Make RegExpPrototypeOptimizableRaw and RegExpInstanceOptimizableRaw infallible. r=jandem a=abillings

2nd part for aurora fix. 
Thanks arai for making it super clear what should land on which branch!
Attachment #8821443 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8821444 [details] [diff] [review]
(mozilla-beta) Part 0: Add MacroAssembler::{storeCallBoolResult,storeCallWordResult}. r=jandem

Let's aim this at the 51 beta 11 build.
Attachment #8821444 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8821445 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.