1433111 - Spectre mitigations for Value type checks - x86 part

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Nicolas B. Pierron [:nbp]]

This bug is about implementing Spectre mitigation for JSValue Type guards unboxing on 32 bits.  The main idea is to add a data dependency such that we can 0, or near 0 any value flowing it with the wrong type.

The idea so far is similar to Bug 1432207 comment 0, where we want to XOR against the expected value, and produce a mask to filter out payload bits.

One of the problem which remains to be decided is how do we prevent the mask of unexpected value from including any high bits which could potentially be interpreted as a pointer.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Compare Value tag JIT mitigation.

This patch adds all 3 versions for adding the JIT mitigation to the engine. It does not yet have the C++ equivalent of the modification.

I tested this change with Octane. I did not noticed anything major while running SpdierMonkey's shell on Octane. All results (60 runs each) are within the noise level of the others:

No mitigation:
__total__                      =  32170 >< 182.74
cmp & cmov mitigation:
__total__                      =  32106 >< 223.19 [~ -0.00]
cmp & sbb mitigation:
__total__                      =  32091 >< 191.15 [~ -0.00]
popcount mitigation:
__total__                      =  31940 >< 299.71 [~ -0.01]

I suspect they are multiple follow-up to this change, such as the Lowering
attempt to reuse the payload register when possible, or to reuse the address
registers, which causes us to spill values on the stack.

Also, I expect Baseline / SharedIC to be the primary source of regressions, thus
I tested with --no-ion --no-asmjs --no-wasm, with the following results (10 runs
each):

No mitigation:
__total__                      =   3789 ><  62.71
cmp & cmov mitigation:
__total__                      =   3631 ><  35.78 [- -0.04]
cmp & sbb mitigation:
__total__                      =   3543 ><  13.64 [- -0.07]
popcount mitigation:
__total__                      =   3532 ><  70.02 [- -0.07]

I will go to the cmp & cmov approach.

Comment 2

[Jan de Mooij [:jandem]]

It would be great to benchmark Octane/Kraken with the LUnbox changes in Ion, since LUnbox is where most of the unboxing happens I'm afraid.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Zero the payload if the Value tag does not match the expected tag.

This patch does use cmov to zero the payload if the tag does not match. It
unifies all the implementation under one function which relies on Operand to
share the spilling logic.

At the moment, I added an extra scratch register but did not propagated the
change to all callers and still use a spilling mechanism if there is no scratch
register.

Addressing all the callers to provide an extra scratch register which might be
used as intended in the comments is kept for future patches. For example
extractObject provide a scratch register, which could be used as a returned
value, but CacheIR does not seems to understand it that way and reuses the
scratch register for something else. Another example is
UnboxedLayout::maybeConstructCode which runs out of scratch registers and
probably acidentally reuse the same tag register for the scratch register of
extractObject function call (which caused kraken failures).

Strange note, this patch actually improves kraken's gaussian-blur benchmark on
my machine (86ms -> 75ms).

Comment 4

[Jan de Mooij [:jandem]]

Some questions before I take a closer look:

(1) Do we really support CPUs without CMOV? In other words, are there CPUs with SSE2 but not CMOV? We're already using cmovz for wasm so I think we would have had crash reports if that's not true. Maybe we can just release-assert we have CMOV support.

(2) We need a similar patch for ARM32 right?

Comment 5

[Jan de Mooij [:jandem]]

Would you mind posting an updated patch for the CMOV part?

I really like the CMOV changes in this patch! I think I'm going to hold off on bug 1435209 and bug 1434230 until that is in.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Would you mind posting an updated patch for the CMOV part?

I moved it to Bug 1435249.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8947451 [details] [diff] [review]
Zero the payload if the Value tag does not match the expected tag.

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

::: js/src/jit/x86/MacroAssembler-x86.h
@@ +708,5 @@
> +                if (scratch != InvalidReg) {
> +                    result = scratch;
> +                } else {
> +                    // We cannot clobber the dest register without loosing the address.
> +                    if (payload.containsReg(dest) && tag.containsReg(dest)) {

self-nit: I will make another patch which removes this complex logic.  The reason why we need this is because we need a scratch register to hold the immediate value which cannot be encoded as argument to cmov.

Except, that today I came up with a work-around which might actually be better. So instead of:

  .. spill ..
  xor     scratch, scratch
  cmpl     IMM_TAG, tag
  cmovzl   payload, scratch
  mov      scratch, dest
  .. drop spill ..

We would encode:

  push     0
  cmpl     IMM_TAG, tag
  cmovzl   payload, dest
  cmovnzl  [esp], dest
  add      4, esp

In which case, if there is any alias between the tag/payload/dest registers, we do not clobber the payload/tag by setting the dest register.

@@ +736,5 @@
> +                xorl(Imm32(JSVAL_TYPE_TO_TAG(type)), result);
> +                cmpl(Imm32(1), result);
> +                // cf = ((type ^ expectedTag) < 1)
> +                // i.e. cf = (type == expectedTag) ? 1 : 0
> +                sbbl(result, result);

self-nit: As we now assume cmov to be present I removed this alternative.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Zero the payload if the Value tag does not match the expected tag.

Delta:
 - Removed cmov from the current patch.
 - Use push(0); cmov(Address(esp, 0), ..) to avoid spilling registers.
 - Use a lambda to factor code which is repeated 3 times.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Add JS Shell and about:config switch for Value masking.

Add JS shell, about:config flag for the Value masking mitigation on x86.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8948750 [details] [diff] [review]
Zero the payload if the Value tag does not match the expected tag.

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

::: js/src/jit/x86/MacroAssembler-x86.h
@@ +704,5 @@
> +            // We zero the destination register and move the payload into it if
> +            // the tag corresponds to the given type.
> +            xorl(dest, dest);
> +            cmpl(Imm32(JSVAL_TYPE_TO_TAG(type)), tag);
> +            cmovzl(payload, dest);

self-nit: missing return.

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8948750 [details] [diff] [review]
Zero the payload if the Value tag does not match the expected tag.

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

::: js/public/Value.h
@@ +313,5 @@
>   *   tag.
> + *
> + * - On 32-bit platforms, when unboxing a Value, we create a mask from the type
> + *   bits with the expected type tag. The mask is then used to mask out to a
> + *   near-zero value the payload, if we are in a speculative execution.

A couple of comment nits: The first part of the second sentence would flow better as "The mask is then used to mask out the payload to a near-zero value" (maybe "mask off" instead of "mask out"? I'm not 100% sure they mean the same thing).

The second part makes the sentence ambiguous for me: Do we only apply the mask if we're in a speculative execution, or do we always apply the mask *in case* we're in a speculative execution? If it's the latter, s/if/in case/ would be clearer I think.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8948750 [details] [diff] [review]
Zero the payload if the Value tag does not match the expected tag.

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

Nice, thanks!

::: js/public/Value.h
@@ +312,5 @@
>   *   some high bits will be set) when called on a Value with a different type
>   *   tag.
> + *
> + * - On 32-bit platforms, when unboxing a Value, we create a mask from the type
> + *   bits with the expected type tag. The mask is then used to mask out to a

Emanuel commented on this, but I think we should rewrite the comment because we no longer use a mask? Something like:

On 32-bit platforms, when unboxing an object/string/symbol Value, we use a conditional move (not speculated) to zero the payload register if the type doesn't match.

::: js/src/jit/x86/Lowering-x86.cpp
@@ +119,5 @@
>  
>      // Swap the order we use the box pieces so we can re-use the payload register.
>      LUnbox* lir = new(alloc()) LUnbox;
> +    if (!JitOptions.spectreValueMasking) {
> +        lir->setOperand(0, usePayloadInRegisterAtStart(inner));

Can we keep this optimization (and defineReuseInput below) for int32/bool unboxing? Especially int32 since it's so common.

::: js/src/jit/x86/MacroAssembler-x86.h
@@ +704,5 @@
> +            // We zero the destination register and move the payload into it if
> +            // the tag corresponds to the given type.
> +            xorl(dest, dest);
> +            cmpl(Imm32(JSVAL_TYPE_TO_TAG(type)), tag);
> +            cmovzl(payload, dest);

Nit: maybe use your new cmovCCl(Assembler::Equal, payload, dest) ? Not everyone may know cmovz and cmove are just aliases.

@@ +729,5 @@
> +            zero = Operand(scratch);
> +        }
> +        cmpl(Imm32(JSVAL_TYPE_TO_TAG(type)), tag);
> +        movPayloadToDest();
> +        cmovnzl(zero, dest);

Same here.

@@ +731,5 @@
> +        cmpl(Imm32(JSVAL_TYPE_TO_TAG(type)), tag);
> +        movPayloadToDest();
> +        cmovnzl(zero, dest);
> +        if (scratch == InvalidReg) {
> +            addl(Imm32(sizeof(void*)), esp);

Maybe leal(Operand(esp, Imm32(sizeof(void*)), esp); ?

Comment 13

[Jan de Mooij [:jandem]]

Maybe file a follow-up to see if we can remove the push/pop fallback? I don't know how hard it is to make sure all callers pass non-aliasing registers or use a scratch register.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8948752 [details] [diff] [review]
Add JS Shell and about:config switch for Value masking.

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

::: modules/libpref/init/all.js
@@ +1549,5 @@
>  pref("javascript.options.dump_stack_on_debuggee_would_run", false);
>  
>  // Spectre security vulnerability mitigations.
>  pref("javascript.options.spectre.index_masking", false);
> +pref("javascript.options.spectre.value_masking", false);

"index masking" is no longer correct if "masking" means "bit masking", and same for value masking. On the other hand, masking can also mean covering/disguising and that one still applies...

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #14)
> ::: modules/libpref/init/all.js
> @@ +1549,5 @@
> >  pref("javascript.options.dump_stack_on_debuggee_would_run", false);
> >  
> >  // Spectre security vulnerability mitigations.
> >  pref("javascript.options.spectre.index_masking", false);
> > +pref("javascript.options.spectre.value_masking", false);
> 
> "index masking" is no longer correct if "masking" means "bit masking", and
> same for value masking. On the other hand, masking can also mean
> covering/disguising and that one still applies...

I will keep it as-is for now, and we can do these renaming in follow-up patches.
An option would to replace the strategy by the purpose, such as "v1_mitigation.array_index" and "v1_mitigation.value_payload".

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/048033244192
Zero the payload if the Value tag does not match the expected tag. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b741fac29b
Add JS Shell and about:config switch for Value masking. r=jandem

Comment 17

[Guilherme Lima]

The bugs of the https://hg.mozilla.org/mozilla-central/rev/c2cddb0cbb20 merge didn't get marked as fixed.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/048033244192
https://hg.mozilla.org/mozilla-central/rev/a5b741fac29b

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thank you, done.

Comment 20

[Jan de Mooij [:jandem]]

Do we need a new bug for ARM?

Comment 21

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #20)
> Do we need a new bug for ARM?

I have not looked at ARM yet. I think this would be better.