Spectre mitigations for object shape/group/class guards

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

After type barriers (bug 1437483) this is the main thing to fix next. Bug 1440394 already added MacroAssembler abstractions for this.
Attachment #8956129 - Flags: review?(luke)
This has mitigations for most shape/group/class guards in JIT code.

Most of this is pretty straight-forward: adding a scratch register and Specre mitigations to the MacroAssembler methods. The spectreRegToZero argument is a register that will be zeroed on speculative paths.

I changed UnboxedPlainObject::convertToNative to return its argument (as NativeObject*) instead of bool. This simplifies LConvertUnboxedObjectToNative a bit - explaining why is a bit complicated, but it has to do with the defineReuseInput we have when Spectre mitigations are enabled.

The shell benchmarks seem unaffected, but Octane is noisy so let's see what AWFY says.

Ted, flagging you for review too, to spread the Spectre mitigations knowledge a bit. If anything is unclear I'm happy to explain.
Attachment #8956132 - Flags: review?(tcampbell)
Attachment #8956132 - Flags: review?(luke)
Attachment #8956129 - Flags: review?(luke) → review+
Comment on attachment 8956132 [details] [diff] [review]
Part 2 - Mitigate shape/group/class guards

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

Nice work, and glad to see there isn't any significant regression.

::: js/src/jit/Lowering.cpp
@@ +3503,5 @@
> +
> +    if (JitOptions.spectreObjectMitigationsMisc) {
> +        auto* lir = new(alloc()) LConvertUnboxedObjectToNative(useRegisterAtStart(ins->object()),
> +                                                               temp());
> +        defineReuseInput(lir, ins, 0);

Ah, I went with a similar technique in bug 1432345.
Attachment #8956132 - Flags: review?(luke) → review+
Priority: -- → P1
Comment on attachment 8956132 [details] [diff] [review]
Part 2 - Mitigate shape/group/class guards

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

Looks good. Was more approachable than I was expecting!

::: js/src/jit/BaselineIC.cpp
@@ +3428,5 @@
>      masm.branchTestObject(Assembler::NotEqual, R1, &failure);
>  
> +    // Ensure the callee's class matches the one in this stub. Note that we use
> +    // spectreRegToZero := ICStubReg here: the callee register isn't used after
> +    // this, but ICStubReg is used below to make the call.

// Ensure the callee's class matches the one in this stub.
// We use |Address(ICStubReg, ICCall_ClassHook::offsetOfNative())| below instead of extracting the hook from callee. As a result the callee register is no longer used and we must use spectreRegToZero := ICStubReg instead.

::: js/src/jit/CacheIRCompiler.h
@@ +378,5 @@
>          currentOpRegs_.clear();
>          currentInstruction_++;
>      }
>  
> +    bool isDeadAfterInstruction(OperandId opId) const {

Nice trick.

::: js/src/vm/UnboxedObject.cpp
@@ +542,1 @@
>  UnboxedPlainObject::convertToNative(JSContext* cx, JSObject* obj)

Can you add a comment explaining the return value change?
Attachment #8956132 - Flags: review?(tcampbell) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf39f14c6f8
part 1 - Add browser pref for misc Spectre object type mitigations. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/240114d8acd3
part 2 - Add Spectre mitigations for most shape/group/class guards in JIT code. r=luke,tcampbell
https://hg.mozilla.org/mozilla-central/rev/2bf39f14c6f8
https://hg.mozilla.org/mozilla-central/rev/240114d8acd3
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.