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

Fri Apr 29 06:15:44 PDT 2016

aaron.ballman 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))
----------------
ariccio wrote:
> 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.
> There's no special reason to harden the compiler more than any other application.

I strongly disagree with this assertion. For starters, not everyone thinks about security as much as they should, so if it costs us nothing in terms of performance to turn this flag on, we make a more secure product that can be more safely used everywhere (including on a server) *out of the box*. That's sufficient special reason to harden any application, including our suite of tools.

================
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))
----------------
aaron.ballman wrote:
> ariccio wrote:
> > 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.
> > There's no special reason to harden the compiler more than any other application.
> 
> I strongly disagree with this assertion. For starters, not everyone thinks about security as much as they should, so if it costs us nothing in terms of performance to turn this flag on, we make a more secure product that can be more safely used everywhere (including on a server) *out of the box*. That's sufficient special reason to harden any application, including our suite of tools.
> 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.

I will have to study this in more detail but at first glance, I'm not certain this has the same level of utility has /GS. However, that's not to say that this switch doesn't have utility.

> How do I boostrap it on Windows? I wish there was a programmatic way to test performance regressions, kinda like chromium has.

Hmm, good point, I don't know if we can bootstrap from MSVC-built Clang. @rnk may know that answer better. If bootstrapping isn't possible, even just timing a large corpus of code (such as our test suite) may provide the answer we need. I know that such a test won't be highly accurate, but I'm not concerned about performance regressions that are in the noise, I'm more worried about performance regressions that are measurable and noticeable. So you could time it with Measure-Command in powershell, or download a utility similar to bash's "time" command to measure the performance of running the suite.

http://reviews.llvm.org/D19519