1430053 - Add browser pref for Spectre bounds check mitigations

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This adds:

* javascript.options.spectre.index-masking browser pref, defaulting to false.

* --spectre-mitigations=on/off shell flag. JIT options can also be toggled with environment variables, so I think a single shell flag is fine for now.

I tested this in the shell and browser and the option is set correctly.

Comment 2

[Lars T Hansen [:lth]]

Now I'm just being pedantic, but probably index_masking, not index-masking.  All other JS prefs that do not use camel-case use underscore.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

The pref is now *.index_masking as suggested.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8942666 [details] [diff] [review]
Patch

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

Thanks!  I'm taking "masking" to mean the intuitive "hiding", not the literal & bitmask impl technique that JSC mentioned in their docs (which we I think don't want to do).

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #4)
> Thanks!  I'm taking "masking" to mean the intuitive "hiding", not the
> literal & bitmask impl technique that JSC mentioned in their docs (which we
> I think don't want to do).

Hm, I meant masking as in bug 1430051 comment 0?

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8942666 [details] [diff] [review]
Patch

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

r=me for the non-JS engine changes if you fix this to apply to workers, assuming that's something you want to do. You should get baku or another DOM worker peer to review that, as I have no idea how it is supposed to be done.

::: js/xpconnect/src/XPCJSContext.cpp
@@ +866,5 @@
>  #ifdef DEBUG
>      JS_SetGlobalJitCompilerOption(cx, JSJITCOMPILER_FULL_DEBUG_CHECKS, fullJitDebugChecks);
>  #endif
> +
> +    JS_SetGlobalJitCompilerOption(cx, JSJITCOMPILER_SPECTRE_INDEX_MASKING, spectreIndexMasking);

I think this only enables the option for the main thread JS engine. Presumably you want this on workers, too.

::: modules/libpref/init/all.js
@@ +1531,5 @@
>  
>  pref("javascript.options.throw_on_debuggee_would_run", false);
>  pref("javascript.options.dump_stack_on_debuggee_would_run", false);
>  
> +// Spectre mitigations.

Please change this to something like "Spectre security vulnerability mitigations.". What you have right now is a bit too brief for such a broadly used file.

Comment 7

[Luke Wagner [:luke]]

(In reply to Jan de Mooij [:jandem] from comment #5)
Oh hah, right, there is a bitmask here too, so yes, "index masking" is a good name in any interpretation :)

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> I think this only enables the option for the main thread JS engine.
> Presumably you want this on workers, too.

Unlike most other JS options, JIT options are process-wide flags, so this will work for all runtimes.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> Unlike most other JS options, JIT options are process-wide flags, so this
> will work for all runtimes.

Ah, alright.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a98f615965d7
Add prefs for index masking Spectre mitigations. r=luke,mccr8

Comment 11

[Jan de Mooij [:jandem]]

It would be great to fuzz shell builds with --spectre-mitigations=on and --spectre-mitigations=off

Right now this flag is a no-op (and the default is "off") but that will change soon.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d03e11f55a
followup - Use this-> in Cell::as() to work around a weird intermittent compiler error. r=me

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a98f615965d7
https://hg.mozilla.org/mozilla-central/rev/e7d03e11f55a

Comment 14

[Christian Holler (:decoder)]

(In reply to Jan de Mooij [:jandem] from comment #11)
> It would be great to fuzz shell builds with --spectre-mitigations=on and
> --spectre-mitigations=off
> 
> Right now this flag is a no-op (and the default is "off") but that will
> change soon.

I've put both flags into production for LangFuzz.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Support for the flag will be added as part of funfuzz PR #157.