Add browser pref for Spectre bounds check mitigations

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8942633 - Flags: review?(luke)
Attachment #8942633 - Flags: review?(continuation)
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.
Posted patch PatchSplinter Review
The pref is now *.index_masking as suggested.
Attachment #8942633 - Attachment is obsolete: true
Attachment #8942633 - Flags: review?(luke)
Attachment #8942633 - Flags: review?(continuation)
Attachment #8942666 - Flags: review?(luke)
Attachment #8942666 - Flags: review?(continuation)
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).
Attachment #8942666 - Flags: review?(luke) → review+
(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 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.
Attachment #8942666 - Flags: review?(continuation) → review+
(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 :)
(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.
(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.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a98f615965d7
Add prefs for index masking Spectre mitigations. r=luke,mccr8
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.
Flags: needinfo?(gary)
Flags: needinfo?(choller)
Pushed by jandemooij@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/a98f615965d7
https://hg.mozilla.org/mozilla-central/rev/e7d03e11f55a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(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.
Flags: needinfo?(choller)
Support for the flag will be added as part of funfuzz PR #157.
Flags: needinfo?(nth10sd)
You need to log in before you can comment on or make changes to this bug.