Nicolas B. Pierron [:nbp] Assignee
	Description • 6 years ago

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.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 2 • 6 years ago

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.

	Jan de Mooij [:jandem]
	Comment 3 • 6 years ago

Comment on attachment 9019996 [details] [diff] [review]
RemoveUnmarkedBlocks should only mark operands of removed blocks.

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

Great find!

	Al Billings [:abillings - ex-MoCo]
	Comment 4 • 6 years ago

sec-approval+ for checkin on November 6, two weeks into the new cycle.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 5 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d762aaeb5711d23e20b809fb8ec07cd04879bb2

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 7 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/8d762aaeb571

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 6 years ago

Please request Beta and ESR60 approval on these when you get a chance.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 9 • 6 years ago

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

	Nicolas B. Pierron [:nbp] Assignee
	Comment 10 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 6 years ago

What's your gut instinct on the riskiness of re-enabling these optimizations? Are we likely to notice any issues quickly?

	Nicolas B. Pierron [:nbp] Assignee
	Comment 12 • 6 years ago

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

	Nicolas B. Pierron [:nbp] Assignee
	Comment 14 • 6 years ago

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 :/)

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 15 • 6 years ago

Letting this wait for a bit for esr uplift, as described in comment 12.

	Julien Cristau [:jcristau]
	Comment 16 • 6 years ago

Comment on attachment 9019996 [details] [diff] [review]
RemoveUnmarkedBlocks should only mark operands of removed blocks.

let's take this for 64.0b9

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/4d3a4ce7e9c6

	Jan de Mooij [:jandem]
	Comment 18 • 6 years ago

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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 19 • 6 years ago

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!

	Nicolas B. Pierron [:nbp] Assignee
	Comment 20 • 6 years ago

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

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 21 • 6 years ago

Oh, my mistake, I was accidentally searching on only the beta 10 build.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 22 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/a73a46ddc848