898381 - IonMonkey: Fix ARM moveValue so that GC pointers are traced

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 1

[Marty Rosenberg [:mjrosenb]]

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?

Comment 2

[Jan de Mooij [:jandem]]

(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).

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to [e-mail] [: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 5

[Jan de Mooij [:jandem]]

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.

Comment 6

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

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.

Comment 7

[Jan de Mooij [:jandem]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/affe224de780

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7fbedcc70869

Comment 10

[Daniel Veditz [:dveditz]]

This shouldn't affect b2g18 because ionmonkey is disabled there.