[PATCH] D19519: Enable "Optimized Debugging" and Enable "Control Flow Guard" in MSVC builds

[Alexander Riccio via llvm-commits]

ariccio added inline comments.

================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:380-381
@@ +379,4 @@
+
+  # /guard (Enable Control Flow Guard) was introduced with Visual Studio 
+  # 2015.
+  if (NOT (MSVC_VERSION LESS 1900))
----------------
rnk wrote:
> aaron.ballman wrote:
> > ariccio wrote:
> > > aaron.ballman wrote:
> > > > rnk wrote:
> > > > > I don't think we should enable this by default. Unless you are deploying clang online in a place like https://gcc.godbolt.org/, you should generally consider it to be exploitable and shouldn't run it on attacker controlled source code. Users like godbolt can enable this flag manually the old fashioned way with CMAKE_C_FLAGS.
> > > > I don't disagree with this logic, but would also ask: if there's no impact to performance, is it harmful to enable? (That's why I was wondering about performance measurements.)
> > > I actually //do// disagree with this logic. /GS stack checking is a similar hardening measure, but we always use it (despite the small perf loss) because code should //always// be hardened. I think of it like a seatbelt: I always wear a seatbelt; not just when I distrust the driver; even when I do trust the driver.
> > > 
> > > Performance wise, I have some indirect evidence of an only very small effect:
> > > 
> > > 
> > >   # [[ https://blogs.msdn.microsoft.com/vcblog/2014/12/08/visual-studio-2015-preview-work-in-progress-security-feature/ | The compiler optimizes some calls out ]]
> > >   # [[ http://blog.trendmicro.com/trendlabs-security-intelligence/windows-10s-new-browser-microsoft-edge-improved-but-also-new-risks/ | All major browsers use CFG ]]
> > >   # [[ https://labs.bromium.com/2015/09/28/an-interesting-detail-about-control-flow-guard/ | All Windows system libraries are built with CFG ]]
> > >   # The generated code isn't much different than `/GS` stack checking code, which inserts a call to `__security_check_cookie`, amd CFG calls `___guard_check_icall_fptr`.
> > > 
> > > 
> > > ...if there's a negligible impact to performance, then there's no harm to be done. Like `/GS`, it also protects against some bugs that are not being exploited.
> > The logic I don't disagree with is that this hardening is of far greater interest to scenarios where clang is being used on a server. The stack checking has some extra correctness benefits (such as alerting you to when you've stomped the stack) which is why I think it makes sense to always enable /GS regardless of its (negligible) performance impact. My understanding of /guard:cf is that this is *only* useful against purposeful hijacking of the binary and it will not give extra protection for incorrect code (unlike /GS). That's why I think it's good to discuss the actual impact on Clang and LLVM itself. If the impact is negligible, then I think we should enable it because it is a safety belt of sorts. However, if it has noticeable impact, I am less convinced of its utility for our general use case.
> > 
> > So to be more precise, before we enable this flag, I would like to understand how it impacts the performance of Clang and LLVM (I'm less interested in how it affects web browsers or the OS, though those make me hopeful that this has no real performance impact). Can you run some timing tests to see the comparison between a build with this flag set and one without it set, for instance, by bootstrapping the compiler (or building some other large code base)?
> I think we should just do whatever the CMake default is. There's no special reason to harden the compiler more than any other application. Eventually, /GS appeared everywhere and MSVC made it the VS default. CMake followed suit. If and when that happens for CF guard, then we'll get it for free.
> My understanding of /guard:cf is that this is *only* useful against purposeful hijacking of the binary and it will not give extra protection for incorrect code (unlike /GS).

I //think// this is an example of clang's version of CFG detecting incorrect code: https://bugs.chromium.org/p/chromium/issues/detail?id=538952

...It's not //exactly// the same, but it's the same basic idea.

> Can you run some timing tests to see the comparison between a build with this flag set and one without it set, for instance, by bootstrapping the compiler (or building some other large code base)?

How do I boostrap it on Windows? I wish there was a programmatic way to test performance regressions, kinda [[ https://www.chromium.org/developers/telemetry/performance-try-bots | like chromium has ]].

> There's no special reason to harden the compiler more than any other application. Eventually, /GS appeared everywhere and MSVC made it the VS default. CMake followed suit. If and when that happens for CF guard, then we'll get it for free.

We may just have to agree to disagree with our philosophical differences here. I think it's a pretty bad idea to compare LLVM's hardening against a generic other program, because most programs have terrible security. I think that we'd be quite silly to ignore a zero-effort switch like this.

That said, I'd be ok dropping this patch if there isn't enough support for it.


http://reviews.llvm.org/D19519