1445235 - Spectre bounds check mitigations for stores

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

We've decided to prevent speculative stores in addition to loads, to be on the safe side, to match what some other engines are doing, and to be more consistent.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - MBoundsCheck

This is actually a lot simpler because we can remove BoundsCheckKind now. I also added some comments.

On Kraken I don't see much of a regression from this, definitely much smaller than for the loads. I think a lot of code does both a load + store and in that case we will be able to fold the MSpectreMaskIndex so there's no overhead.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Rename boundsCheck32ForLoad to spectreBoundsCheck32

(sed also added a missing \n at the end of some files...)

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8959103 [details] [diff] [review]
Part 1 - MBoundsCheck

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

Nice!

::: js/src/jit/IonBuilder.cpp
@@ +13312,5 @@
>          check->setNotMovable();
>  
> +    if (JitOptions.spectreIndexMasking) {
> +        // Use a separate MIR instruction for the index masking. Doing this as
> +        // part of MBoundsCheck is unsound because bounds checks can be

nit: s/is/would be/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/485c44f946e9
part 1 - Also add Spectre mitigations for MBoundsCheck added for stores. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a403eafe620
part 2 - Rename MacroAssembler boundsCheck32ForLoad to spectreBoundsCheck32. r=nbp

Comment 5

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/485c44f946e9
https://hg.mozilla.org/mozilla-central/rev/0a403eafe620

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 3 - LStoreTypedArrayElementHole

This is a bit of an edge case: well-written code shouldn't write to out-of-bounds typed array indexes (it's a no-op), so I also simplified the code a bit.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Remove RegisterOrInt32Constant

Removing RegisterOrInt32Constant simplifies the next patch.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8961761 [details] [diff] [review]
Part 4 - Remove RegisterOrInt32Constant

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

I could ask for seeing a tiny micro benchmark regression, but my understanding is that we are going to ship with this regression anyway … so let's just land it and see what AWFY reports.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Refactor spectreBoundsCheck32

This patch refactors spectreBoundsCheck32 so that its scratch register is optional.

On (32-bit) x86, the scratch register helps emit better code - without a scratch register we do a push/pop and some jumps. On other platforms the scratch register makes no difference.

This simplifies the next patch, because in IC code we don't have scratch registers available on x86 unfortunately.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Use spectreBoundsCheck32 for stores

The StoreElementHole code is pretty complicated unfortunately :/

In Ion code this requests a spectreTemp register only on x86 because it helps spectreBoundsCheck32.

In Baseline IC code we don't have registers available on x86 so we pass InvalidReg as maybeScratch.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8961762 [details] [diff] [review]
Part 5 - Refactor spectreBoundsCheck32

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

::: js/src/jit/MacroAssembler.h
@@ +1349,5 @@
>          DEFINED_ON(arm, arm64, mips_shared, x86_shared);
>  
>      // Performs a bounds check and zeroes the index register if out-of-bounds
>      // (to mitigate Spectre).
> +    inline void spectreBoundsCheck32(Register index, const Operand& length, Register maybeScratch,

nit: Operand like templates should not be allowed in public the interface of the MacroAssembler as they obscure with what arguments they are being used.

Make it private.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8961763 [details] [diff] [review]
Part 6 - Use spectreBoundsCheck32 for stores

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

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1687,5 @@
>      // Reload obj->elements as callTypeUpdateIC used the scratch register.
>      masm.loadPtr(Address(obj, NativeObject::offsetOfElements()), scratch);
>  
>      // Increment initLength and length.
> +    masm.add32(Imm32(1), elementsInitLength);

note: I found my-self confused while reading this code, because we define the Address with the base register "scratch", but we do change the value of scratch in the mean time.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa5b7d5198c
part 3 - Use Spectre-safe bounds check for LStoreTypedArrayElementHole. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f44f69accd
part 4 - Remove RegisterOrInt32Constant. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba5e089f9a0
part 5 - Refactor spectreBoundsCheck32 to work without a scratch register. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/03f1b458b986
part 6 - Use spectreBoundsCheck32 for more stores in JIT code. r=nbp

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d9c1cc3c97
followup - Add some blank lines to appease masm style checker. r=red

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7aa5b7d5198c
https://hg.mozilla.org/mozilla-central/rev/70f44f69accd
https://hg.mozilla.org/mozilla-central/rev/0ba5e089f9a0
https://hg.mozilla.org/mozilla-central/rev/03f1b458b986
https://hg.mozilla.org/mozilla-central/rev/a0d9c1cc3c97

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8959103 [details] [diff] [review]
Part 1 - MBoundsCheck

(This applies to all patches in this bug.)
---
Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: We want to uplift Spectre fixes in 61 to 60.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Has been on Nightly for a few weeks now.
[String changes made/needed]: None.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8959103 [details] [diff] [review]
Part 1 - MBoundsCheck

Spectre mitigation needed for 60. Approved for 60.0b11.

Comment 18

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/39a6945f06ed8899c0146e91418ee8e322c3a0aa
https://hg.mozilla.org/releases/mozilla-beta/rev/02a7736cbf001656b7aa15480fd92520632f5872
https://hg.mozilla.org/releases/mozilla-beta/rev/93bd90643c5b5bb0e90eee0dd3f71645c638d656
https://hg.mozilla.org/releases/mozilla-beta/rev/707fd3a2a6886566d9f7a56017c4983c869b5f2c
https://hg.mozilla.org/releases/mozilla-beta/rev/d757d31f4b9d88462f24e57f4823717b986d3572
https://hg.mozilla.org/releases/mozilla-beta/rev/ce00b156aeb78fb7ddafd71d03c9709278265880
https://hg.mozilla.org/releases/mozilla-beta/rev/ebe5227dc318fc234d0abb45d868b1e34c6779c2