1438886 - Spectre JIT mitigation for speculated data after the execution of C++ calls.

Status: RESOLVED FIXED

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

Description

[Nicolas B. Pierron [:nbp]]

As the C++ code is not yet being instrumented to mitigate spectre v1 attacks, we can add mitigations to prevent speculative execution across the C++ call boundary when coming from the JIT.

One idea to do these mitigations is to use lfence instruction, as suggested by Intel and AMD.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Prevent speculative execution past the VMFunction calls.

This adds lfence instruction right before the call instructions into C++ code for x86 and x64.
If there is a regression it should be below the noise level on my computer.

Comment 2

[Jan de Mooij [:jandem]]

Sorry for the delay. Some questions:

(1) I think it's more (or at least as) important to add an LFENCE *after* the call, since most attacks use an out-of-bounds load and that's easy to do with DataView.prototype.getInt32 or a similar VMFunction. We would only do this for VMFunctions that return a value.

(2) How much does this affect a worst-case micro-benchmark? For instance a call to ProxyGetProperty... Last time I measured emitting LFENCE after JSNative calls it was at least a 30% regression.

(3) We should use a similar instruction on ARM(64) (I'd also suggest "speculationFence" instead of "flushOutOfOrderBuffer").

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Sorry for the delay. Some questions:
> 
> (1) I think it's more (or at least as) important to add an LFENCE *after*
> the call, since most attacks use an out-of-bounds load and that's easy to do
> with DataView.prototype.getInt32 or a similar VMFunction. We would only do
> this for VMFunctions that return a value.

My understanding, is that the JIT code contains mitigation checking for the type, which is not the case of the C++ code yet.

Thus we want to protect the C++ code from an extended speculation window created in the JIT code, thus having an lfence before the call to flush the out-of-order buffer of any pending instruction waiting for the resolution.

If the C++ function call returns a Value, then the manipulation of this value should be protected by the JIT value mitigations.

If the C++ function call returns a pointer, then this pointer is checked for null-ness before being interpreted as an error/exception. If this branch is trained against such errors, then we might flow in JIT code with a nullptr, which would do a TLB miss, because we always checks the first words below the pointer (for all our HandlePtr types).

> (2) How much does this affect a worst-case micro-benchmark? For instance a
> call to ProxyGetProperty... Last time I measured emitting LFENCE after
> JSNative calls it was at least a 30% regression.

I have not checked any micro benchmarks, but this is under the noise level on Octane.

> (3) We should use a similar instruction on ARM(64) (I'd also suggest
> "speculationFence" instead of "flushOutOfOrderBuffer").

Ok.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: ~41} from comment #3)
> If the C++ function call returns a Value, then the manipulation of this
> value should be protected by the JIT value mitigations.

I think the main concern is the VM call reading out-of-bounds (array, strings, typed array, NodeLists...) or something similar. Int32/double unboxing isn't mitigated.

> I have not checked any micro benchmarks, but this is under the noise level
> on Octane.

We should check.. Octane spends a lot of time in JIT code, unlike Speedometer for example.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Prevent speculative execution after returning from GC-capable C++ code.

Delta:
 - Move speculation barrier after the calls.
 - Rename the flushOutOfOrderBuffer to speculationBarrier.
 - Add DOM method calls.
 - Rename the preference to include all Jit to C++ calls and not only the VMFunctions.

TODO: testing the performance impact.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8955473 [details] [diff] [review]
Prevent speculative execution after returning from GC-capable C++ code.

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

::: dom/ipc/ContentParent.cpp
@@ +2007,5 @@
>    for (unsigned int i = 0; i < prefsLen; i++) {
>      const char* prefName = ContentPrefs::GetEarlyPref(i);
>      MOZ_ASSERT_IF(i > 0,
> +                  strcmp(prefName, ContentPrefs::GetEarlyPref(i - 1)) > 0,
> +                  "Content process preferences should be sorted alphabetically.");

self-nit: as MOZ_ASSERT_IF with 3 arguments does not exists, I rewrote it to:

    MOZ_ASSERT(i == 0 || strcmp(prefName, ContentPrefs::GetEarlyPref(i - 1)) > 0,
	       "Content process preferences should be sorted alphabetically.");

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: ~38} from comment #5)
> Created attachment 8955473 [details] [diff] [review]
> Prevent speculative execution after returning from GC-capable C++ code.
> 

and also, Delta:
 - Add ARM & ARM64 CSDB instructions, based on ARM security update notes.
 - Move the speculationBarrier function to the generic MacroAssembler.
 - (Move other spectre function in the checked section of the MacroAssembler)

Comment 9

[Jan de Mooij [:jandem]]

We also care about LCallNative*, the similar Baseline IC, LGetDOMProperty*, etc right? I think there are other places where we use custom exit frames to call into C++.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> We also care about LCallNative*, the similar Baseline IC, LGetDOMProperty*,
> etc right? I think there are other places where we use custom exit frames to
> call into C++.

Right, I have them locally.

As far as I know, we do have custom exit frames for ICs, which I have not yet looked at, and for bailouts, which I think we do not care, because it would be harder to inject any random pointers read from it.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8955473 [details] [diff] [review]
Prevent speculative execution after returning from GC-capable C++ code.

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

Sorry for the delay! This looks good but I'd like to see the CallNative version with the comments below addressed.

::: js/src/jit/CodeGenerator.cpp
@@ +4403,5 @@
>          masm.loadValue(Address(masm.getStackPointer(), IonDOMMethodExitFrameLayout::offsetOfResult()),
>                         JSReturnOperand);
>      }
>  
> +    // Until C++ code is instrumented against Spectre, prevent speculative

I think here we could check if the MIR instruction hasDefUses(), so we don't emit the barrier if we don't use the result?

::: js/src/jit/MacroAssembler.h
@@ +1947,5 @@
>  
> +    // ========================================================================
> +    // Spectre Mitigations.
> +    //
> +    // Spectre attacks are side-channel attacks based on cache polution or

Nit: pollution

@@ +1953,5 @@
> +    // possible:
> +    //
> +    //   - Stop speculative executions, with memory barriers. Memory barriers
> +    //     forces all branches depending on loads to be resolved, and thus
> +    //     resolve all miss-speculated path.

Nit: s/forces/force/, s/path/paths/

@@ +1955,5 @@
> +    //   - Stop speculative executions, with memory barriers. Memory barriers
> +    //     forces all branches depending on loads to be resolved, and thus
> +    //     resolve all miss-speculated path.
> +    //
> +    //   - Use conditional move instructions. Some CPU have a branch predictor,

Nit: CPUs

@@ +1958,5 @@
> +    //
> +    //   - Use conditional move instructions. Some CPU have a branch predictor,
> +    //     and not a flag predictor. In such cases, using a conditional move
> +    //     instruction to zero some pointer/index is enough to add a
> +    //     data-dependency which prevent any futher executions until the load is

Nit: prevents

::: js/src/jit/x86/Trampoline-x86.cpp
@@ +784,5 @@
>      }
> +
> +    // Until C++ code is instrumented against Spectre, prevent speculative
> +    // execution from returning any private data.
> +    if (f.returnsData() && !JitOptions.spectreJitToCxxCalls)

Remove the ! here and everywhere else (does that affect Talos results?).

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: Prevent speculative execution after returning from GC-capable C++ code. r=jandem

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8957529 [details] [diff] [review]
Prevent speculative execution after returning from GC-capable C++ code. r=jandem

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

Looks good!

::: js/src/jit/CodeGenerator.cpp
@@ +4307,5 @@
>  
> +    // Until C++ code is instrumented against Spectre, prevent speculative
> +    // execution from returning any private data.
> +    if (JitOptions.spectreJitToCxxCalls && call->mir()->hasLiveDefUses())
> +        masm.speculationBarrier();

Also add this here, if (!ignoresReturnValue_):

https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/js/src/jit/BaselineIC.cpp#3397

There are also other places where we use custom exit frames like proxy getter calls in Ion CacheIR code. Follow-up bug?

@@ +12103,5 @@
>  
> +    // Until C++ code is instrumented against Spectre, prevent speculative
> +    // execution from returning any private data.
> +    if (JitOptions.spectreJitToCxxCalls && ins->mir()->hasLiveDefUses())
> +        masm.speculationBarrier();

setters don't return a value, so I think this doesn't need mitigations (and there are no hasLiveDefUses)

::: js/xpconnect/src/XPCJSContext.cpp
@@ +812,5 @@
>          Preferences::GetBool(JS_OPTIONS_DOT_STR "spectre.object_mitigations.barriers");
>      bool spectreStringMitigations =
>          Preferences::GetBool(JS_OPTIONS_DOT_STR "spectre.string_mitigations");
>      bool spectreValueMasking = Preferences::GetBool(JS_OPTIONS_DOT_STR "spectre.value_masking");
> +    bool spectreVMFunctionCalls = Preferences::GetBool(JS_OPTIONS_DOT_STR "spectre.jit_to_C++_calls");

Nit: spectreJitToCxxCalls for consistency

Comment 14

[Nicolas B. Pierron [:nbp]]

Latest performance results:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=6fa42b5b7692ce531d3482fee9d073ba08875e4e&framework=1&filter=drom&selectedTimeRange=172800

I still have to fix a few things in the Arm & Arm64 simulators, but this should not block this patch, only the test harness checking with the simulators.

Comment 15

[Nicolas B. Pierron [:nbp]]

Note for sheriffs & release managers:
 - This is a known Dromaeo perf regression.
 - We prefer it landed and toggled off than not landed. ( modules/libpref/init/all.js )
 - This introduces new ARM & ARM64 instructions which were only added in January security-update information from ARM. We should watch for new start-up crashes on these platform, in case the security-update information or the implementation of this patch is incorrect.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90c70e5122f6
Prevent speculative execution after returning from GC-capable C++ code. r=jandem

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf56e43e67ad
Fix arm disassembler by adding support for CSDB instruction. r=me

Comment 18

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/90c70e5122f6
https://hg.mozilla.org/mozilla-central/rev/bf56e43e67ad