1546446 - hasExpirableShortRangeBranches ignores the reserved space.

Status: RESOLVED FIXED

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

Description

[Nicolas B. Pierron [:nbp]]

hasExpirableShortRangeBranches ignores the reserved space of enterNoPool and as such finishPool does not flush everything which is required to ensure that hasSpaceForInsts passes after the finishPool call.

We should carry the space to be reserved to the finishPool function such that
thus hasExpirableShortRangeBranches could take the range given as argument instead of the default ShortRangeBranchHysteresis (128 bytes).

This bug might also affect ARM.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1546446 - Carry the pool-free size to the finishPool function.

Comment 2

[Nicolas B. Pierron [:nbp]]

This bug causes us to potentially insert branch targets of long forward branches in case where the short-forward branch range is not enough to reach the target. This insertion of branches addresses can happen in the middle of jump tables (which are also code addresses), except that the register allocation might be different and cause us to miss-interpret our own generated code, thus making it miss-behave in a similar way as a ROP attack.

(In reply to Nicolas B. Pierron [:nbp] from comment #0)

This bug might also affect ARM.

This only affects ARM64, as ARM does not implement PatchShortRangeBranchToVeneer function [1].
However, we should keep this bug closed until it is backported to Firefox 67.

[1] https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/js/src/jit/arm/Assembler-arm.h#1829-1833

Comment 3

[Nicolas B. Pierron [:nbp]]

review-ping?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Too late for 67 at this point. Don't forget to request sec-approval before landing.

Comment 5

[Sean Stangl [:sstangl]]

We're not shipping Ion in 67, and ARM has never hit that issue, so that seems fine.

Comment 6

[Nicolas B. Pierron [:nbp]]

ARM64 is the only architecture which has this issue so far, and we only ship ARM64 builds to nightly and beta users.
Therefore firefox 67 is not affected.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9060685 [details]
Bug 1546446 - Carry the pool-free size to the finishPool function.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Easily with a very large switch statement over integers.
• 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?: 67 (see comment 6)
• If not all supported branches, which bug introduced the flaw?: ?
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This patch should apply to all branches without much issues.
• How likely is this patch to cause regressions; how much testing does it need?: Mildly risky. I would expect the passing test suite to provide a good coverage, and fuzzers to catch remaining issues if any.

Comment 8

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

Comment on attachment 9060685 [details]
Bug 1546446 - Carry the pool-free size to the finishPool function.

Sec-approval+ for trunk. Please ask for approval for a beta patch as well.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/eeb2c18a40baae79fba2f464d133849eaa97f615
https://hg.mozilla.org/mozilla-central/rev/eeb2c18a40ba

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. It grafts cleanly as-landed.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9060685 [details]
Bug 1546446 - Carry the pool-free size to the finishPool function.

Beta/Release Uplift Approval Request

• User impact if declined: Bad branch targets, could lead to miss-use of registers. (comment 2)
• 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 change the logic for filling constant pool and delayed forward branches, which could be corner cases which are hard to hit.
  Alternative would be to invert the logic of the constant pool spilling to compute the dead line each time one is needed instead of recomputing it every-time, but this would be a much larger patch.
• String changes made/needed: none

Comment 12

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9060685 [details]
Bug 1546446 - Carry the pool-free size to the finishPool function.

arm jit fix, approved for 68.0b7

Comment 13

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e7f918e6874a712a7f9ff07d30f24521a32193a