Closed Bug 1434230 Opened 6 years ago Closed 6 years ago

Spectre mitigations for strings

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

      No description provided.
Depends on: 1434263
Depends on: 1434267
* In loadStringChars, we use a conditional move instead of a branch to load inline/non-inline chars. This is actually a small perf win when the branch is unpredictable (in the predictable case perf is similar to a branch).

* In loadStringChars, if the string is a rope, we use a conditional move to zero the string register. This should only affect speculative execution. I tried a bitmasking scheme for this but it was a bit slower than the cmov.

* In loadStringChar, we do something similar to (2) when we load rope->leftChild.

* The patch adds JitOptions.spectreStringMitigations and a javascript.options.spectre.string_mitigations browser pref

TODO still: fixing the Latin1/TwoByte branch.
Attachment #8947099 - Flags: review?(nicolas.b.pierron)
Attachment #8947099 - Flags: review?(luke)
(In reply to Jan de Mooij [:jandem] from comment #1)
> TODO still: fixing the Latin1/TwoByte branch.

Also, there are other callers of JSString::offsetOf* methods -- I think we should use masm methods for all of these and then we can easily audit/fix them.
Comment on attachment 8947099 [details] [diff] [review]
Part 1 - loadStringChars

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

Nice.
Attachment #8947099 - Flags: review?(luke) → review+
Comment on attachment 8947099 [details] [diff] [review]
Part 1 - loadStringChars

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

::: js/src/jit/MacroAssembler.cpp
@@ +1398,5 @@
> +        // Zero the output register if the input was not a rope.
> +        loadPtr(Address(str, JSRope::offsetOfLeft()), scratch);
> +        movePtr(ImmWord(0), dest);
> +        movePtrTest32Zero(Address(str, JSString::offsetOfFlags()), Imm32(JSString::LINEAR_BIT),
> +                          scratch, dest);

nit: loadPtrTest32Zero ?  In which case there is no need for the extra scratch register.
Attachment #8947099 - Flags: review?(nicolas.b.pierron) → review+
Attached patch Part 2 - Use masm methods (obsolete) — Splinter Review
This makes the offsetOf* JSString methods that are potentially Spectre-unsafe private and adds MacroAssembler as friend class.

This way we force all JIT code to use the masm abstractions to access these fields and there we can do our Spectre magic.
Attachment #8947442 - Flags: review?(luke)
Attachment #8947442 - Attachment is obsolete: true
Attachment #8947442 - Flags: review?(luke)
Attachment #8947443 - Flags: review?(luke)
There's one part left: I want to add a CharEncoding argument to MacroAssembler::loadStringChars - then when we load TwoByte chars, we can add an extra cmov to protect it from reading OOB Latin1 chars. I have this mostly working, except I need to add cmovnz to the assembler backend.

The good news is that AFAICS these 3 patches have no/negligible perf overhead on SunSpider/Octane/Kraken. A charCodeAt micro-benchmark is about 15-20% slower, but that's really worst-case, and a micro-benchmark mixing inline/non-inline is actually faster with the mitigations due to the use of CMOVcc for loading non-inline chars.
Comment on attachment 8947443 [details] [diff] [review]
Part 2 - Use masm methods

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

::: js/src/jit/MacroAssembler.cpp
@@ +1403,5 @@
> +                          dest, str);
> +
> +        // Load non-inline chars if the inline-chars bit is not set.
> +        loadPtrTest32Zero(Address(str, JSString::offsetOfFlags()), Imm32(JSString::INLINE_CHARS_BIT),
> +                          Address(str, JSString::offsetOfNonInlineChars()), dest);

From irc: perhaps we could fuse the two checks so there was a single cmov (here or in a followup opt patch).
Attachment #8947443 - Flags: review?(luke) → review+
Depends on: 1435249
Priority: -- → P1
The last part; when loading TwoByte chars we now prevent speculation if the string has Latin1 chars.

This also combines multiple cmoves into one as suggested before.
Attachment #8949456 - Flags: review?(luke)
Comment on attachment 8949456 [details] [diff] [review]
Part 3 - Check character encoding

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

Nice job, looks good.
Attachment #8949456 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9ba4938b08
part 1 - Some Spectre mitigations for loadStringChars. r=luke,nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/6598194588d7
part 2 - Add masm methods for string access, more Spectre mitigations. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f67769bbbd8
part 3 - Fix Spectre issues related to string character encoding. r=luke
Bah, this only affects 32-bit x86. I'll fix.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/893bb948cb93
part 1 - Some Spectre mitigations for loadStringChars. r=luke,nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba257675f53
part 2 - Add masm methods for string access, more Spectre mitigations. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/766c0dd5ed3d
part 3 - Fix Spectre issues related to string character encoding. r=luke
Flags: needinfo?(jdemooij)
Attachment #8949685 - Flags: review?(luke)
Comment on attachment 8949685 [details] [diff] [review]
Part 4 - Enable by default

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

Great work here too.
Attachment #8949685 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9848ae936a3d
part 4 - Enable Spectre string mitigations by default. r=luke
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: