Closed Bug 1432479 Opened 2 years ago Closed 2 years ago

Spectre mitigations for Value type checks - 64-bit part

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Attached patch PatchSplinter Review
This implements the XOR-based unboxing described in bug 1432207 comment 0 on 64-bit platforms.

Pretty straight-forward for the most part.
Attachment #8944722 - Flags: review?(luke)
Comment on attachment 8944722 [details] [diff] [review]
Patch

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

::: js/src/jit/BaselineCompiler.cpp
@@ +4801,5 @@
>      // Store the arguments object if there is one.
>      Label noArgsObj;
> +    Address argsObjSlot(genObj, GeneratorObject::offsetOfArgsObjSlot());
> +    masm.branchTestUndefined(Assembler::Equal, argsObjSlot, &noArgsObj);
> +    masm.unboxObject(argsObjSlot, scratch2);

To check my understanding: was this code subtley relying on the fact that, if you stripped off the tag bits of an undefined value, the payload of 'undefined' was 0?

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +757,5 @@
>  
> +    void unboxNonDouble(const ValueOperand& src, Register dest, JSValueType type) {
> +        MOZ_ASSERT(type != JSVAL_TYPE_DOUBLE);
> +        if (type == JSVAL_TYPE_INT32 || type == JSVAL_TYPE_BOOLEAN) {
> +            movl(src.valueReg(), dest);

Hah, so I think the patch actually improves the perf of unboxing an int/bool :)
Attachment #8944722 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #1)
> To check my understanding: was this code subtley relying on the fact that,
> if you stripped off the tag bits of an undefined value, the payload of
> 'undefined' was 0?

Yep and then later we would crash because we got a pointer with one or more high bits set :)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9b4b746ee6
Use XOR for Value unboxing on 64-bit to mitigate certain Spectre attacks. r=luke
It will be interesting to see if this has any effect on crash-stats. This might help catch other type confusion bugs.
https://hg.mozilla.org/mozilla-central/rev/6e9b4b746ee6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
No longer blocks: 1433111
Attached patch mips64.patch (obsolete) — Splinter Review
Attachment #8946594 - Flags: review?(nicolas.b.pierron)
Attached patch mips32.patchSplinter Review
Non functional changes for mips32.
Attachment #8946598 - Flags: review?(nicolas.b.pierron)
Next time please file new bugs :) Thanks!
Comment on attachment 8946594 [details] [diff] [review]
mips64.patch

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

Thank you for your patch.

::: js/src/jit/mips64/MacroAssembler-mips64.h
@@ +392,5 @@
> +    void unboxObjectOrNull(const T& src, Register dest) {
> +        unboxNonDouble(src, dest, JSVAL_TYPE_OBJECT);
> +        MOZ_ASSERT(SecondScratchReg != dest);
> +        mov(ImmWord(~JSVAL_OBJECT_OR_NULL_BIT), SecondScratchReg);
> +        ma_and(dest, SecondScratchReg);

Might be replaced by a single dins with zero register as source.

@@ +399,5 @@
> +    void unboxGCThingForPreBarrierTrampoline(const Address& src, Register dest) {
> +        loadPtr(src, dest);
> +        MOZ_ASSERT(ScratchRegister != dest);
> +        mov(ImmWord(JSVAL_PAYLOAD_MASK_GCTHING), ScratchRegister);
> +        ma_and(dest, ScratchRegister);

Not sure if its worth it, but I think you can use previous implementation of unboxObject here (only one dext instruction) ?

@@ +512,1 @@
>              storePtr(ScratchRegister, address);

The offsets used by this method should? be small enough so that storePtr never clobbers ScratchRegister. Is there a possibility to use SecondScratchReg here for src to be future proof? Maybe to use xor masking w/o scratch register with drotr, xori, drotr sequence.
sorry, ok.

(In reply to Jan de Mooij [:jandem] from comment #8)
> Next time please file new bugs :) Thanks!
hi, thanks a lot for the review.

but it looks like we can not   masking w/o scratch register with drotr, xori, drotr sequence?

(In reply to Dragan Mladjenovic from comment #9)
> Comment on attachment 8946594 [details] [diff] [review]
> mips64.patch
> 
> Review of attachment 8946594 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for your patch.
> 
> ::: js/src/jit/mips64/MacroAssembler-mips64.h
> @@ +392,5 @@
> > +    void unboxObjectOrNull(const T& src, Register dest) {
> > +        unboxNonDouble(src, dest, JSVAL_TYPE_OBJECT);
> > +        MOZ_ASSERT(SecondScratchReg != dest);
> > +        mov(ImmWord(~JSVAL_OBJECT_OR_NULL_BIT), SecondScratchReg);
> > +        ma_and(dest, SecondScratchReg);
> 
> Might be replaced by a single dins with zero register as source.
> 
> @@ +399,5 @@
> > +    void unboxGCThingForPreBarrierTrampoline(const Address& src, Register dest) {
> > +        loadPtr(src, dest);
> > +        MOZ_ASSERT(ScratchRegister != dest);
> > +        mov(ImmWord(JSVAL_PAYLOAD_MASK_GCTHING), ScratchRegister);
> > +        ma_and(dest, ScratchRegister);
> 
> Not sure if its worth it, but I think you can use previous implementation of
> unboxObject here (only one dext instruction) ?
> 
> @@ +512,1 @@
> >              storePtr(ScratchRegister, address);
> 
> The offsets used by this method should? be small enough so that storePtr
> never clobbers ScratchRegister. Is there a possibility to use
> SecondScratchReg here for src to be future proof? Maybe to use xor masking
> w/o scratch register with drotr, xori, drotr sequence.
Attached patch mips64.patchSplinter Review
Attachment #8946594 - Attachment is obsolete: true
Attachment #8946594 - Flags: review?(nicolas.b.pierron)
Flags: needinfo?(dragan.mladjenovic)
Attachment #8947015 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8947015 [details] [diff] [review]
mips64.patch

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

::: js/src/jit/mips64/MacroAssembler-mips64.h
@@ +504,5 @@
> +            if (type == JSVAL_TYPE_OBJECT)
> +                unboxObjectOrNull(value, SecondScratchReg);
> +            else
> +                unboxNonDouble(value, SecondScratchReg, type);
> +            computeEffectiveAddress(address, ScratchRegister);

Sorry for nitpicking again,

The only source of BaseIndex for this method seems to be https://searchfox.org/mozilla-central/source/js/src/jit/BaselineCacheIRCompiler.cpp#1254. It might be worth to optimize for this case (scale TimesOne and no offset) and call storePtr directly. In that case SecondScratchReg won't be clobbered?
(In reply to yuyin from comment #11)
> hi, thanks a lot for the review.
> 
> but it looks like we can not   masking w/o scratch register with drotr,
> xori, drotr sequence?

Hi, Thanks a lot

Sorry I haven fully thought through this whole idea. Yes type tag are more than 16 bits. Either way the only possible problem with ScratchRegiter might be storeUnboxedPayload and you fixed it. Are you ok to opening a separate issue for this set of patches ?
Flags: needinfo?(dragan.mladjenovic)
Depends on: 1434790
Comment on attachment 8947015 [details] [diff] [review]
mips64.patch

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

I will note that the spectre mitigation on 64 bits, works by keeping high bits in the pointers. While this works well for x64, this might still be an issue on MIPS.  MIPS is allowed to use the full range of pointers, and is not limited to the 0 — 2^48 - 1 range as is x64.  Thus this mitigation would only works if **all** the allocators of Firefox are restricted to the same range as the JS heap allocator.

You should double check that all other allocators out-side of the JS engine, which are embeddded in Firefox (if any) do produce pointers below 2^48.
Attachment #8947015 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8946598 - Flags: review?(nicolas.b.pierron) → review+
thank you for this information, I will do some investigation about that.

(In reply to Nicolas B. Pierron [:nbp] from comment #15)
> Comment on attachment 8947015 [details] [diff] [review]
> mips64.patch
> 
> Review of attachment 8947015 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I will note that the spectre mitigation on 64 bits, works by keeping high
> bits in the pointers. While this works well for x64, this might still be an
> issue on MIPS.  MIPS is allowed to use the full range of pointers, and is
> not limited to the 0 — 2^48 - 1 range as is x64.  Thus this mitigation would
> only works if **all** the allocators of Firefox are restricted to the same
> range as the JS heap allocator.
> 
> You should double check that all other allocators out-side of the JS engine,
> which are embeddded in Firefox (if any) do produce pointers below 2^48.
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4458bb78bb4
MIPS64: Use XOR for Value unboxing on 64-bit to mitigate certain Spectre attacks. r=draganmladjenovic
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbafd7f786d2
Backed out changeset a4458bb78bb4 due to wrong bug number
Depends on: 1437842
No longer depends on: 1437842
You need to log in before you can comment on or make changes to this bug.