Closed Bug 1502013 Opened 10 months ago Closed 10 months ago

js::jit::RemoveUnmarkedBlocks does not mark operands of removed chunks.

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 64+ fixed
firefox63 --- wontfix
firefox64 + fixed
firefox65 + fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

(Keywords: sec-high, Whiteboard: [post-critsmash-triage][adv-main64+][adv-esr60.4+])

Attachments

(3 files)

js::jit::RemoveUnmarkedBlocks iterates over all the blocks and FlagAllOperandsAsHavingRemovedUses of blocks which are marked (to be kept), instead of blocks which are unmarked (to be removed).

Flagging all operands as having removed uses is an operation which is required to prevent optimizations which have a destructive impact on the data which is used on bailout.

Not marking properly instructions has having removed uses has been experienced as potentially being the source of security issues in the past.
Keywords: sec-high
Comment on attachment 9019996 [details] [diff] [review]
RemoveUnmarkedBlocks should only mark operands of removed blocks.

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: I am not completely sure yet. I fear it is hard to build an exploit for  this issue because this involves making GVN remove all branches which are making use of a value (by prooving branches as being unreachable), without having these branches removed by BranchPruning earlier.

This is possible but extremely tricky to hit.

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

Which older supported branches are affected by this flaw?: all

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

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: Unfortunately, this patch revive a bunch of optimizations which got implictly disabled after Bug 1238592. I would expect that a few bugs might have appeared in these optimization since the last 2 years.

If this fix causes too many issues, we can probably re-disable these optimizations by "conservatively" always flagging all operands as having removed uses. This can be done by removing the `if (…) { continue; }` from this patch instead of patching the condition.
Attachment #9019996 - Flags: sec-approval?
Comment on attachment 9019996 [details] [diff] [review]
RemoveUnmarkedBlocks should only mark operands of removed blocks.

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

Great find!
Attachment #9019996 - Flags: review?(jdemooij) → review+
sec-approval+ for checkin on November 6, two weeks into the new cycle.
Whiteboard: [checkin on 11/6]
Attachment #9019996 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/8d762aaeb571
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please request Beta and ESR60 approval on these when you get a chance.
Flags: needinfo?(nicolas.b.pierron)
Whiteboard: [checkin on 11/6]
Comment on attachment 9019996 [details] [diff] [review]
RemoveUnmarkedBlocks should only mark operands of removed blocks.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1238592

User impact if declined: A security issue might be forged based on the fact that we are flagging the wrong set of instructions (comment 0)

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): This will re-enable some optimizations which were inhibited by the previous code. The risk is that these optimizations bit-rotted since this bug got introduced and would cause other failures.

One potential alternative would be to completely remove the "if (…) …;" statements from the current code, and keep the current changes riding the train until we catch these issues if any.

String changes made/needed: None
Flags: needinfo?(nicolas.b.pierron)
Attachment #9019996 - Flags: approval-mozilla-beta?
Comment on attachment 9023036 [details] [diff] [review]
(esr60) RemoveUnmarkedBlocks should only mark operands of removed blocks.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: A security issue might be forged based on the fact that we are flagging the wrong set of instructions (comment 0)

Fix Landed on Version: 65

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): This will re-enable some optimizations which were inhibited by the previous code. The risk is that these optimizations bit-rotted since this bug got introduced and would cause other failures.

One potential alternative would be to completely remove the "if (…) …;" statements from the current code, and keep the current changes riding the train until we catch these issues if any.

String or UUID changes made by this patch: None
Attachment #9023036 - Flags: approval-mozilla-esr60?
What's your gut instinct on the riskiness of re-enabling these optimizations? Are we likely to notice any issues quickly?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> What's your gut instinct on the riskiness of re-enabling these
> optimizations? Are we likely to notice any issues quickly?

If we notice any issue, this would be an increase in the volume of crashes in the generated JIT code (Bug 858032), or in the JIT compiler it-self.

I see multiple scenario:
 1/ We take the patch to beta & esr.
 2/ We take the patch to beta, wait for a week and take it to esr.
 3/ We take the alternative patch to beta & esr and let the current one ride the train.

So far I do not see anything in Nightly. Thus I suggest taking option (2).
In case of any issue, the fallback is to go with the option (3) which is less risky.

In the eventual case of option (3), I will attach the alternative patch for nightly & beta and another version for release & esr60.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 9023613 [details] [diff] [review]
(alternative) RemoveUnmarkedBlocks mark all operands as having removed uses.

Asking for review, just to avoid r=me on security patches.

This is the conservative approach of the fix to be taken if the previous patch causes any issues.
The difference is that instead of omitting the wrong set, we just mark everything as having removed uses (in a really inefficient way :/)
Attachment #9023613 - Flags: review?(jdemooij)
Letting this wait for a bit for esr uplift, as described in comment 12.
Comment on attachment 9019996 [details] [diff] [review]
RemoveUnmarkedBlocks should only mark operands of removed blocks.

let's take this for 64.0b9
Attachment #9019996 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9023613 [details] [diff] [review]
(alternative) RemoveUnmarkedBlocks mark all operands as having removed uses.

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

Hesitant r=me, let's hope we don't need it.
Attachment #9023613 - Flags: review?(jdemooij) → review+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Nicolas, I do see some new JIT crash signatures for beta 10: https://crash-stats.mozilla.com/signature/?date=%3C2018-11-19T18%3A03%3A12%2B00%3A00&date=%3E%3D2018-11-12T18%3A03%3A12%2B00%3A00&product=Firefox&version=64.0b10&signature=js%3A%3Ajit%3A%3AMaybeEnterJit#summary Can you take a look and see if you think they are related? Thanks!
Flags: needinfo?(nicolas.b.pierron)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #19)
> Nicolas, I do see some new JIT crash signatures for beta 10:
> https://crash-stats.mozilla.com/signature/?date=%3C2018-11-
> 19T18%3A03%3A12%2B00%3A00&date=%3E%3D2018-11-
> 12T18%3A03%3A12%2B00%3A00&product=Firefox&version=64.
> 0b10&signature=js%3A%3Ajit%3A%3AMaybeEnterJit#summary Can you take a look
> and see if you think they are related? Thanks!

I see no difference in volume compared to 64.0b9 and before, and the crash addresses have the same patterns.
This sounds safe enough to me.
Flags: needinfo?(nicolas.b.pierron)
Oh, my mistake, I was accidentally searching on only the beta 10 build.
Comment on attachment 9023036 [details] [diff] [review]
(esr60) RemoveUnmarkedBlocks should only mark operands of removed blocks.

Since last week's beta uplift has gone smoothly, let's uplift for ESR60 as well.
Attachment #9023036 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main64+][adv-esr60.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.