Closed Bug 1445235 Opened 6 years ago Closed 6 years ago

Spectre bounds check mitigations for stores


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




Tracking Status
firefox60 --- fixed
firefox61 --- fixed


(Reporter: jandem, Assigned: jandem)


(Blocks 1 open bug)



(6 files)

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.
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.
Attachment #8959103 - Flags: review?(luke)
(sed also added a missing \n at the end of some files...)
Attachment #8959141 - Flags: review?(nicolas.b.pierron)
Attachment #8959141 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8959103 [details] [diff] [review]
Part 1 - MBoundsCheck

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


::: 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/
Attachment #8959103 - Flags: review?(luke) → review+
Pushed by
part 1 - Also add Spectre mitigations for MBoundsCheck added for stores. r=luke
part 2 - Rename MacroAssembler boundsCheck32ForLoad to spectreBoundsCheck32. r=nbp
Keywords: leave-open
Depends on: 1447016
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.
Attachment #8961316 - Flags: review?(nicolas.b.pierron)
Attachment #8961316 - Flags: review?(nicolas.b.pierron) → review+
Removing RegisterOrInt32Constant simplifies the next patch.
Attachment #8961761 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8961761 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8961762 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8961763 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8961762 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8961763 - Flags: review?(nicolas.b.pierron) → review+
Pushed by
part 3 - Use Spectre-safe bounds check for LStoreTypedArrayElementHole. r=nbp
part 4 - Remove RegisterOrInt32Constant. r=nbp
part 5 - Refactor spectreBoundsCheck32 to work without a scratch register. r=nbp
part 6 - Use spectreBoundsCheck32 for more stores in JIT code. r=nbp
Pushed by
followup - Add some blank lines to appease masm style checker. r=red
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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.
Attachment #8959103 - Flags: approval-mozilla-beta?
Comment on attachment 8959103 [details] [diff] [review]
Part 1 - MBoundsCheck

Spectre mitigation needed for 60. Approved for 60.0b11.
Attachment #8959103 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1450796
You need to log in before you can comment on or make changes to this bug.