[PATCH] D99078: [LLD] Implement /guard:[no]ehcont

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: lld/COFF/DriverUtils.cpp:104-111
     else if (arg.equals_lower("nolongjmp"))
-      config->guardCF = GuardCFLevel::NoLongJmp;
-    else if (arg.equals_lower("cf") || arg.equals_lower("longjmp"))
-      config->guardCF = GuardCFLevel::Full;
+      config->guardCF &= ~GuardCFLevel::LongJmp;
+    else if (arg.equals_lower("noehcont"))
+      config->guardCF &= ~GuardCFLevel::EHCont;
+    else if (arg.equals_lower("cf"))
+      config->guardCF = GuardCFLevel::CF;
+    else if (arg.equals_lower("longjmp"))
----------------
rnk wrote:
> pengfei wrote:
> > alvinhochun wrote:
> > > pengfei wrote:
> > > > alvinhochun wrote:
> > > > > This changed the behaviour of `/guard:cf`. Before this change, `/guard:cf` is equivalent to `/guard:cf,longjmp` by default, but after this change one must explicitly pass `/guard:longjmp` to`lld-link for it to generate the guard long jump table.
> > > > > 
> > > > > This also does not match the behaviour of MSVC's link.exe. From my quick testing, `/guard:cf`, `/guard:longjmp` and `/guard:cf,longjmp` all do the same thing for it to enable both CFGuard and the long jump table. Only when you pass `/guard:cf,nolongjmp` is the jump table not present in the final image.
> > > > Thanks for raising the problem. I don't find any description about the `longjmp` option in official document. Unless there's explicit document about the relationship between `/guard:cf`, `/guard:longjmp` and `/guard:cf,longjmp`, I don't think we need to match the exact behavior with MSVC. The single `/guard:cf` has ambiguity since we are supporting `ehcont`. So I'm not sure if the current MSVC behavior is constant in future versions.
> > > I just think that, long jump guard seems to be a rather cheap hardening measure when control flow guard is already in use. Not enabling it by default perhaps seems a bit odd, especially since this feels like an unintentional change. But fine with me either way (I don't really use MSVC or Clang with MSVC target, just noticed the difference when investigating this LLD feature.)
> > Thanks for the point. I'm not a MSVC/clang-cl user either. So I couldn't weight how cheap the long jump guard is. I designed it in the perspective of a junior user, to whom it's hard to understand the `longjmp` is on by default for some reason and need `nolongjmp` to disable it, while `ehcont` is on the contrary.
> > It would have value from the perspective of backward compatibility if we chose this way at the beginning. But the patch was merged more than one year... So let's keep it as is :)
> I agree, longjump protections should be implied by /guard:cf. The nolongjump flag exists merely as a backwards compatibility escape hatch. Users should generally not specify `/guard:longjump`.
Also, FWIW, even if the exact behaviour of Microsoft's tools isn't documented, such a detail like this would most probably not change randomly from one version to another. I also would prefer that we match link.exe's behaviour exactly on this point, instead of making up our own rules for how these flags interact.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99078/new/

https://reviews.llvm.org/D99078