* 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.

(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.

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.

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.

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).

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.

Comment on attachment 8949456 [details] [diff] [review] Part 3 - Check character encoding Review of attachment 8949456 [details] [diff] [review]: ----------------------------------------------------------------- Nice job, looks good.

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

Backed out for assertion failures on MacroAssembler.cpp Push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7f67769bbbd8c696518e92a2752a8a5861f408d5 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/a20dc29e502d8b74a1fca9195154d895d7de9ec0 Log: https://treeherder.mozilla.org/logviewer.html#?job_id=161180048&repo=mozilla-inbound&lineNumber=1881

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

https://hg.mozilla.org/mozilla-central/rev/893bb948cb93 https://hg.mozilla.org/mozilla-central/rev/3ba257675f53 https://hg.mozilla.org/mozilla-central/rev/766c0dd5ed3d

Comment on attachment 8949685 [details] [diff] [review] Part 4 - Enable by default Review of attachment 8949685 [details] [diff] [review]: ----------------------------------------------------------------- Great work here too.

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9848ae936a3d part 4 - Enable Spectre string mitigations by default. r=luke