1595329 - Possible JIT bug: failure to set negative zero flag in 'ceil' operator

Status: RESOLVED FIXED

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

Description

[mlfbrown]

We're verifying range analysis logic in IonMonkey and came across an inconsistency:
Math.ceil(-.5) = -0, but the ceil range analysis operator does not set the canBeNegativeZero flag for the result range in this case.
The relevant function is here:
https://searchfox.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#1169

Please let us know if we're missing something, and thanks so much for your time!

Comment 1

[Deian Stefan]

function myCeil(x) {
    if(x >= -2) {
      if(x <= -0.5) {
        return Math.ceil(x);
      }
    }
  return 42;
}

var a = [ 10.0, 20.2, 30.3, 3.0, 0.2, -2 ];

for(var i = 0;i < 1000000; i++)
  for(var j = 0;j < a.length; j++)
    myCeil(a[j])

copy in Range::ceil has lower -2, upper 0, with canBeNegativeZero unset, so op also ends up with lower -2, upper 0, with canBeNegativeZero unset. I'm not sure if there is a bailout on -0, but even so it seems like ranges are incorrect and may be used assuming -0 is not possible. Are we missing something obvious?

Comment 2

[Luke Wagner [:luke]]

(Forwarding)

Comment 3

[Daniel Veditz [:dveditz]]

nbp: what are the security implications of this?

Comment 4

[Nicolas B. Pierron [:nbp]]

This is correct, there is a bug in Range::ceil which do not set the canBeNegativeZero flag.

However, this bug will not appear as a problem in MCeil code generation, as it bails on the range ]-1, -0], which includes -0.5.
Identically, this will not appear as a problem in MIntegerToInt32, as the upper bound contains 0 due to the current representation of Ranges.

In cases where the instruction is removed from the code generation, but its range was used to decide anything, such as MBeta computation and unreachable code elimination, then the ranges are computed using only the Integer ranges, which are including 0:
https://searchfox.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#502-503,514

So far, I do not think this bug in Range Analysis leads to any bug which can be leveraged by a differential behavior of JavaScript. I will double check this last assumption by making a test case and fix this issue, as this indeed confusing and could have lead to a bug, if we had made further refinement.

Comment 5

[Deian Stefan]

Thanks! That makes sense. After playing around with the code my conclusion was that it can only affect other analyses. Am probably more paranoid about the redundant/bounds check elimination than DCE though.

Comment 6

[Deian Stefan]

Attachment: 0001-Bug-1595329-Test-Math.ceil-range-for-1-0.patch

Comment 7

[Deian Stefan]

Attachment: 0001-Bug-1595329-Math.ceil-range-analysis-should-set-canB.patch

Comment 8

[Deian Stefan]

Attachment: 0001-Bug-1595329-Math.ceil-range-analysis-should-set-canB.patch

Ran clang-format on this one. All js, jsapi, and jit tests pass.

Comment 9

[Deian Stefan]

I can't seem to modify the attachment to ask for review. (Sorry for not using moz-phab on this; can do it if ya'll want though.)

Comment 10

[Deian Stefan]

I should mention, I also ran this code though our verifier and it checks out.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9112154 [details] [diff] [review]
0001-Bug-1595329-Math.ceil-range-analysis-should-set-canB.patch

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

Thanks for making this patch, I will import it in phabricator and merge it (if phabricator accept that I push and land my-self)

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1595329 - Math.ceil range analysis should set canBeNegativeZero flag for ]-1,-0]

Comment 13

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

https://hg.mozilla.org/integration/autoland/rev/75948d4b03dde9b9b5b47df025083482f81f2d93
https://hg.mozilla.org/mozilla-central/rev/75948d4b03dd

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact which justifies Beta backport consideration here or can this fix ride Fx73 to release (it seems pretty clearly not-needed for ESR)? Please nominate the patch for approval if so - it grafts cleanly as-landed.

Comment 15

[Frederik Braun [:freddy]]

This security bug is being publicly discussed on Twitter.
How much of a problem is this? Do we need to uplift?
https://mobile.twitter.com/tlk___/status/1202640545115463682

Comment 16

[Nicolas B. Pierron [:nbp]]

Sorry, I was on PTO for the last few days.

To answer comment 14 and comment 15, there should be no user affected by these changes, which is the conclusion of comment 4 and comment 5.
To paraphrase, the analysis is indeed incorrect but no execution nor optimization relies on this incorrect result in a way which would change JS execution or caused a security issue.

This bug remained hidden in case of uncertainty around the analysis.
I think we can safely open this bug, and there should not be any need for back-porting this patch.

Comment 17

[Julien Cristau [:jcristau]]

wontfix for 72 based on comment 16.

Comment 18

[Deian Stefan]

I'm still a bit more paranoid. Nicolas, you're confident this can't affect DCE or RCE?

The Range::toIntegerInt32 path seems safe because of the copy->refineToExcludeNegativeZero();. I'm less confident about the MBeta case though. I'll try to look at this today.

Comment 19

[Tom Ritter [:tjr]]

Unless further comments indicate otherwise (in which case, ni me) I'm going to classify this as not exploitable and therefore doesn't need an advisory.