1432479 - Spectre mitigations for Value type checks - 64-bit part

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

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

Pretty straight-forward for the most part.

Comment 1

[Luke Wagner [: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 :)

Comment 2

[Jan de Mooij [:jandem]]

(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 :)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9b4b746ee6
Use XOR for Value unboxing on 64-bit to mitigate certain Spectre attacks. r=luke

Comment 4

[Jan de Mooij [:jandem]]

It will be interesting to see if this has any effect on crash-stats. This might help catch other type confusion bugs.

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6e9b4b746ee6

Comment 6

[yuyin]

Attachment: mips64.patch

Comment 7

[Dragan Mladjenovic]

Attachment: mips32.patch

Non functional changes for mips32.

Comment 8

[Jan de Mooij [:jandem]]

Next time please file new bugs :) Thanks!

Comment 9

[Dragan Mladjenovic]

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.

Comment 10

[yuyin]

sorry, ok.

(In reply to Jan de Mooij [:jandem] from comment #8)
> Next time please file new bugs :) Thanks!

Comment 11

[yuyin]

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.

Comment 12

[yuyin]

Attachment: mips64.patch

Comment 13

[Dragan Mladjenovic]

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?

Comment 14

[Dragan Mladjenovic]

(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 ?

Comment 15

[Nicolas B. Pierron [:nbp]]

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.

Comment 16

[yuyin]

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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Narcis Beleuzu [:NarcisB]]

Backed out due to landing with the wrong bug number

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4458bb78bb4004bfcbdfff7388e4ee1ba908d7e

Comment 19

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbafd7f786d2
Backed out changeset a4458bb78bb4 due to wrong bug number