Closed Bug 1268083 Opened 4 years ago Closed 4 years ago

JitcodeGlobalEntry for baseline jitcode is not updated correctly


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 + fixed
firefox-esr45 --- unaffected


(Reporter: jonco, Assigned: jonco)



(Keywords: csectype-uaf, sec-high)


(2 files, 1 obsolete file)

JitcodeGlobalTable lookup methods return a copy of the JitcodeGlobalEntry, so when these are marked then any moved pointers will not be updated.  As far as I can tell this only affects the script pointer in BaselineEntry.
Attached patch bug1268083-jitcode-map (obsolete) — Splinter Review
I changed JitcodeGlobalTable::lookupInfallible to return a reference to the entry rather than copying it, and updated callers.
Attachment #8746060 - Flags: review?(jdemooij)
Comment on attachment 8746060 [details] [diff] [review]

Review of attachment 8746060 [details] [diff] [review]:

Ugh, good catch.

::: js/src/jit/JitcodeMap.h
@@ +1029,5 @@
>          return skiplistSize_ == 0;
>      }
>      bool lookup(void* ptr, JitcodeGlobalEntry* result, JSRuntime* rt);
>      bool lookupForSampler(void* ptr, JitcodeGlobalEntry* result, JSRuntime* rt,

Should we change these two methods to return a pointer (null on failure)? Or rename to lookupCopy?
Attachment #8746060 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #2)

> Should we change these two methods to return a pointer (null on failure)? Or
> rename to lookupCopy?

Good call, I'll try making them return pointers.
Kannan, in this code in ProfilingFrameIterator::getPhysicalFrameAndEntry:

    jit::JitcodeGlobalTable* table = rt_->jitRuntime()->getJitcodeGlobalTable();
    if (hasSampleBufferGen())
        table->lookupForSampler(returnAddr, entry, rt_, sampleBufferGen_);
        table->lookupInfallible(returnAddr, entry, rt_);

I just wanted to check, is lookupForSampler infallible too?  It's assumed to succeed here.
Flags: needinfo?(kvijayan)
This has been an issue since we since we started moving scripts in bug 1122579.
Blocks: CompactingGC
Updated patch with review comments.
Attachment #8746060 - Attachment is obsolete: true
Attachment #8749118 - Flags: review+
Comment on attachment 8749118 [details] [diff] [review]
bug1268083-jitcode-map v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Very difficult.  I haven't see a crash due to this but it is possible.

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? 48 is affected.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply.

How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions.
Attachment #8749118 - Flags: sec-approval?
sec-approval+ for trunk. We'll want a patch for aurora as well.
Attachment #8749118 - Flags: sec-approval? → sec-approval+
Closed: 4 years ago
Resolution: --- → FIXED
Group: javascript-core-security → core-security-release
Approval Request Comment
[Feature/regressing bug #]: Bug 1259180
[User impact if declined]: Possible crash / vulnerability.
[Describe test coverage new/current, TreeHerder]: On m-c since 6th May.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8751212 - Flags: review+
Attachment #8751212 - Flags: approval-mozilla-aurora?
Blocks: 1259180
No longer blocks: CompactingGC
Attachment #8751212 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: core-security-release
Flags: needinfo?(kvijayan)
You need to log in before you can comment on or make changes to this bug.