Closed Bug 898381 Opened 6 years ago Closed 6 years ago

IonMonkey: Fix ARM moveValue so that GC pointers are traced

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 + fixed
firefox25 + fixed
firefox-esr17 --- unaffected
b2g18 --- disabled

People

(Reporter: jandem, Assigned: jandem)

Details

(Keywords: sec-high, Whiteboard: [adv-main24+])

Attachments

(1 file)

Attached patch PatchSplinter Review
I was reading some unrelated code earlier today and noticed this. I'm not sure it can cause crashes right now, but there are some callers that pass a Value that could be an object/string I think, so let's assume it's possible.

x86/x64 already do this and grep shows this is the only place that uses payload.i32 without an isMarkable() check.
Attachment #781644 - Flags: review?(mrosenberg)
Comment on attachment 781644 [details] [diff] [review]
Patch

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

Nothing looks wrong with it, so r+, but I think we should poke a bit first.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +2672,5 @@
>  MacroAssemblerARMCompat::moveValue(const Value &val, Register type, Register data) {
>      jsval_layout jv = JSVAL_TO_IMPL(val);
>      ma_mov(Imm32(jv.s.tag), type);
> +    if (val.isMarkable())
> +        ma_mov(ImmGCPtr(reinterpret_cast<gc::Cell *>(val.toGCThing())), data);

Since without the ImmGCPtr, we'd never record the offset, we shouldn't crash due to the wrong instrutions during marking.  However, we could not trace through that pointer, and get the standard use-after-collect GC bug.  How does x86/x64 handle this case?
Attachment #781644 - Flags: review?(mrosenberg) → review+
(In reply to Marty Rosenberg [:mjrosenb] from comment #1)
> Since without the ImmGCPtr, we'd never record the offset, we shouldn't crash
> due to the wrong instrutions during marking.  However, we could not trace
> through that pointer, and get the standard use-after-collect GC bug.

Yup, it'd be a use-after-collect GC bug.

> How does x86/x64 handle this case?

See comment 0; x86 already has exactly the same code. x64 has similar code (only difference is due to the different boxing format there).
Can we get a sec rating?  If this isn't something highly exploitable that we'd have to chemspill for - or something attributable to high crash volumes - we're probably not going to track this for FF23 since we're in the homestretch there and our final beta is going to build on Monday.
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #3)
> Can we get a sec rating?  If this isn't something highly exploitable that
> we'd have to chemspill for - or something attributable to high crash volumes
> - we're probably not going to track this for FF23 since we're in the
> homestretch there and our final beta is going to build on Monday.

This is potentially sec-critical and can cause all kinds of crashes, but it's likely hard/impossible to trigger atm (fuzzers also didn't find it yet). So either we are lucky and for some reason it's not exploitable right now or maybe we have a sec-critical. Unfortunately I can't say for sure which one it is.
Comment on attachment 781644 [details] [diff] [review]
Patch

lsblakk, I'm requesting sec-approval and aurora approval. Since we don't have a testcase and don't have any evidence this is causing problems right now, I'm fine with taking this on aurora only. If you disagree please a+ for beta too.

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
IonMonkey.

> User impact if declined: 
Maybe crashes, security bugs.

> Testing completed (on m-c, etc.): 
Didn't land yet.

> Risk to taking this patch (and alternatives if risky): 
Very low.

> String or IDL/UUID changes made by this patch:
None.

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

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

> Which older supported branches are affected by this flaw?
Firefox 18+.

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

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

> How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.
Attachment #781644 - Flags: sec-approval?
Attachment #781644 - Flags: approval-mozilla-aurora?
Comment on attachment 781644 [details] [diff] [review]
Patch

Giving sec-appoval+ to land in trunk. After this, please land on Aurora, which I have also set the approval flag for.
Attachment #781644 - Flags: sec-approval?
Attachment #781644 - Flags: sec-approval+
Attachment #781644 - Flags: approval-mozilla-aurora?
Attachment #781644 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/affe224de780
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Whiteboard: [adv-main24+]
This shouldn't affect b2g18 because ionmonkey is disabled there.
Group: core-security
You need to log in before you can comment on or make changes to this bug.