Closed Bug 1436983 Opened 4 years ago Closed 4 years ago

CodeGenerator::link can GC before capturing read barriers


(Core :: JavaScript: GC, enhancement, P1)




Tracking Status
firefox-esr52 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed


(Reporter: jonco, Assigned: jonco)



(Keywords: sec-moderate, Whiteboard: [adv-main60+][post-critsmash-triage])


(2 files)

CodeGenerator::link() creates a JitCode object for an Ion compilation that may have happend off-thread.  In that case various read barriers may have been skipped, because we can't fire these off-thread due to race conditions.  This is partly handled by the fact that the GC will trace any GC thing created during an incremental GC:

    // Also, note that creating the code here during an incremental GC will
    // trace the code and mark all GC things it refers to. This captures any
    // read barriers which were skipped while compiling the script off thread.
    Linker linker(masm);
    AutoFlushICache afc("IonLink");
    JitCode* code = linker.newCode<CanGC>(cx, ION_CODE, !patchableBackedges_.empty());

Also, GC will cancel off-thread Ion compilations in various situations to maintain safety.

However, here a GC can happen right here before the new GC is traced, and we won't cancel this compilation because it has already been removed from the list of pending compilations.

I don't think this is safe.
Patch to disallow GC while linking.
Attachment #8949715 - Flags: review?(nicolas.b.pierron)
Blocks: 1018363
Attachment #8949715 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8949715 [details] [diff] [review]

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

Very difficult, as this requires controlling both the timing of Ion compilation and GC.

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


Which older supported branches are affected by this flaw?

Everything back to FF 33.

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

Bug 1018363.

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

Should be trivial.

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

This is a simple change and unliklely to cause regressions.
Attachment #8949715 - Flags: sec-approval?
(In reply to Jon Coppeard (:jonco) from comment #0)
> However, here a GC can happen right here before the new GC is traced, and we

This should read "before the new GC thing is traced".
Calling this a sec-moderate in conversation with Dan. As such, it doesn't need sec-approval to check in, so I'm clearing it.
Keywords: sec-moderate
Attachment #8949715 - Flags: sec-approval?
Backed out for GCG test failures in jit-test/tests/ion/getprop-primitive.js:
The previous patch here set off a test failure in CGC builds, probably by disturbing GC timing.

It seems that the test creates an ArgumentsObject while calling assertEqIf() which causes a GC and invalidates Ion code causing to the test to fail.

nbp suggested this fix.
Attachment #8950210 - Flags: review?(nicolas.b.pierron)
Attachment #8950210 - Flags: review?(nicolas.b.pierron) → review+
Group: javascript-core-security → core-security-release
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
Flags: needinfo?(jcoppeard)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
Having dug a bit deeper it seems this wasn't actually a problem.  Linker::newCode takes a template parameter specifying whether it is allowed to GC, but the first thing it does is to suppress GC:

So whatever you pass, it doesn't GC.  I'll file a bug to clean this up.  This one doesn't require backporting.
Flags: needinfo?(jcoppeard)
Whiteboard: [adv-main60+]
Flags: qe-verify-
Whiteboard: [adv-main60+] → [adv-main60+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.