1429807 - Investigate /d2guardspecload flag in MSVC 15.5.3

Status: RESOLVED WONTFIX

(Core :: Security, enhancement, P5)

Description

[Tom Ritter [:tjr]]

MSVC just put out an undocumented /d2guardspecload flag.

We're starting to dig into it, we found some cases where it applies and other cases where it doesn't, and have an email out to them to try and get more information.

We'd like to investigate this, beginning with perf hit. Unfortunately, PGO is broken in 15.5.2 (Bug 1424281) so we won't be able to get a perfect comparison, but we can compare no-PGO 15.5.3 with and without the flag and see what it looks like.

Marking this bug private for now since this flag doesn't seem to be publicized (and Microsoft explicitly said they're not going to talk about it for now.) 
https://www.reddit.com/r/cpp/comments/7p9l8x/is_msvc_also_being_updated_to_include_the/

Comment 1

[Tom Ritter [:tjr]]

Okay, Ryan did the hard part, I'm pushing the try buttons.

Control Run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fa7796527e003c61a6c7b73742b176295943b3b
With Flag Run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97eb2d8e9bab5ef138f60097e430ca7e67ca263a

If those build, I'll do retriggers for Talos.

Comment 2

[Tom Ritter [:tjr]]

Okay, after trying a few things, I haven't gotten PGO to work with this flag. I used the new flags :dmajor suggested, and still no dice:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d2167f73daa1bdfc735e87d6010c8c5c7a64f6e&selectedJob=155753649

I emailed Microsoft about this.



BUT the good news is that the data for an opt <-> opt comparison looks pretty good so far! I expected some absurd numbers, but it's generally pretty low.  I queued up more runs to get more confidence.  
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8fa7796527e003c61a6c7b73742b176295943b3b&newProject=try&newRevision=97eb2d8e9bab5ef138f60097e430ca7e67ca263a&framework=1

Comment 3

[Tom Ritter [:tjr]]

For fun, I decided to count the lfence instructions that were added to xul.dll (and only xul.dll).

I (re-?)learned that disassembling xul.dll in IDA is... not efficient. But through the highly technical method of "Copy all the disassembled instructions into a test file and Control+f" I found 2160 occurrences of lfence.  And 10 of mfence although I'm not sure if this compiler flag added those or not.

Comment 4

[Tom Ritter [:tjr]]

Performance seems like it might be within bounds? Adding Joel who I usually ask about these things.  

Joel: I don't know if we want to use this (we'd need to get PGO fixed for one) but what do you think about these preliminary results? Are they low enough we should investigate PGO further, or high enough you have reservations?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Assuming base vs new are done properly, there is just one concerning issue with the a11y benchmark, specifically on a subtest for table mutations:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=8fa7796527e003c61a6c7b73742b176295943b3b&newProject=try&newRevision=97eb2d8e9bab5ef138f60097e430ca7e67ca263a&originalSignature=85f5ef6d0216a483f05e71cacf2bd040b7af6558&newSignature=85f5ef6d0216a483f05e71cacf2bd040b7af6558&framework=1

I know PGO will change things- maybe we are optimistic to think this 2.7% change in a11y will be reduced :)  If this is one test and we get other security gains, it is probably worth going forward with this work.

Comment 6

[Tom Ritter [:tjr]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #5)
> Assuming base vs new are done properly

I realized in the car this morning they're totally not.  I'm comparing with and without the flag, but we also bumped compilers in this.


-central vs new compiler:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=8fa7796527e0&framework=1&selectedTimeRange=172800

This one we have PGO for.

-central vs new compiler + flag:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=97eb2d8e9bab&framework=1&selectedTimeRange=172800


Still looks within the realm of possibility though?

Comment 7

[Tom Ritter [:tjr]]

Attachment: Add compiler flags

Comment 8

[Tom Ritter [:tjr]]

Attachment: Flip new PGO Switchs to see if it fixes things

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

this looks good-thanks for checking this out.

Comment 10

[Tom Ritter [:tjr]]

Okay, let's collect all the links.



15.5.3 Compiler, no Spectre, Opt and PGO:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fa7796527e003c61a6c7b73742b176295943b3b&selectedJob=156015626

15.5.3 compiler, Spectre, Opt:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97eb2d8e9bab5ef138f60097e430ca7e67ca263a

15.5.3 compiler, Spectre, PGO:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d7bae3f7da56ccb8662367576603df5bc2ddabd&selectedJob=156015801


Perf comparison of 15.5.3 No Spectre to -central:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=8fa7796527e0&framework=1&selectedTimeRange=172800

Perf comparison of 15.5.3 Spectre Opt to -central:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=97eb2d8e9bab&framework=1&selectedTimeRange=172800

And the new one and most important one!
Perf comparison of 15.5.3 Spectre PGO to -central:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=4d7bae3f7da56ccb8662367576603df5bc2ddabd&framework=1&selectedTimeRange=172800


Performance looks.... great! Adding Joel just so he can see.




So we're not 100% certain we want to enable this flag though. Its instrumentation is very limited, and we (well, Luke) thinks the most attackable locations are "where the C++ does the bounds check, but the speculated value is returned to JS. a clear example being the C++ typed array getter, or, maybe simpler, DataView.prototype.getInt32".  The flag does not follow across functions, it's just pattern matching inside a single function.

So how would it look if we turned it on, then decided it wasn't helpful at all and turned it off?

But still, we're considering this for 59, so it'd be good to get the PGO issues put to bed.

The try links above include (broken) 32 bit PGO builds, and Ryan's patch (https://hg.mozilla.org/try/rev/58d7d51e221c0cf689a9ee63b4594df3ced07c0f) that fixes the 64 bit PGO build.  There was speculation that Bug 1412889 may resolve the 32 bit PGO issue, but dmajor tried it and said it didn't work.  (I also tried it before I saw that, here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa000f8a31a656309dd561b6f99dabaa5af25ba2)

:dmajor is off next week (1/15) so we'll see what we can see without his help :)

Comment 11

[Tom Ritter [:tjr]]

Attachment: Add compiler flags

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

this looks great- a few wins!

Comment 13

[Tom Ritter [:tjr]]

Unhiding this now that MSFT has published https://blogs.msdn.microsoft.com/vcblog/2018/01/15/spectre-mitigations-in-msvc/

Comment 14

[Luke Wagner [:luke]]

From the post, this is useful: 'If you know that a particular block of your code is performance-critical (say, in a tight loop) and does not need the mitigation applied, you can selectively disable the mitigation with  __declspec(spectre(nomitigation)).'

Comment 15

[Tom Ritter [:tjr]]

After discussion, the usefulness of this is limited enough that we're not going to flip it right now, but we'll keep an eye on this to see if MSFT improves it.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Note that in the long term, this switch will be renamed to /Qspectre:

https://blogs.msdn.microsoft.com/vcblog/2018/01/15/spectre-mitigations-in-msvc/

Comment 17

[Tom Ritter [:tjr]]

The new version that Ryan posted (15.5.6) added 1417 calls to lfence in xul. (Using /Qspectre)

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9bd3fc6acb735a198d31b1b17d092a93c4339d5&selectedJob=160025518

Comment 18

[Tom Ritter [:tjr]]

We don't ship msvc builds anymore.