[PATCH] D118355: Add -mmanual-endbr switch to allow manual selection of control-flow protection

[Joao Moreira via Phabricator via llvm-commits]

joaomoreira added inline comments.


================
Comment at: llvm/lib/Target/X86/X86IndirectBranchTracking.cpp:156-161
   if (needsPrologueENDBR(MF, M)) {
-    auto MBB = MF.begin();
-    Changed |= addENDBR(*MBB, MBB->begin());
+    if (!ManualENDBR || MF.getFunction().doesCfCheck()) {
+      auto MBB = MF.begin();
+      Changed |= addENDBR(*MBB, MBB->begin());
+    } else {
+      // When -mmanual-endbr is in effect, the compiler does not
----------------
xiangzhangllvm wrote:
> I am a little puzzle here. Let me copy your patch description here:
> 
> ```
> >When -mmanual-endbr is set, llvm refrains from automatically adding
> >ENDBR instructions to functions' prologues, which would have been
> >automatically added by -fcf-protection=branch. Although this works
> >correctly, missing ENDBR instructions where they are actually needed
> >could lead to broken binaries, which would fail only in running time.
> ```
> I think the purpose of "-mmanual-endbr" should be "Supplementary Way" for the cases which the CET can't correctly insert endbr in automatic way.
> Here (in if (needsPrologueENDBR(MF, M)) ) the automatic way will insert the endbr. So I think the job for "-mmanual-endbr" should be done in parallel with the old condition. Some like:
> ```
> if (ManualENDBR ){
>   do something
> }else { // automatic
>   if (needsPrologueENDBR(MF, M)) {insert endbr}
>  }
> }
> ```
I don't think the idea of -mmanual-endbr is to have a supplementary way for corner cases where CET misses automatic placement. In my understanding the goal is to set the compiler to not emit ENDBRs unless the attribute cf_check is used, thus providing a way to manually reduce the number of valid targets.

For reference, here is a link for -mmanual-endbr on gcc, https://gcc.gnu.org/legacy-ml/gcc-patches/2018-12/msg00713.html and here are patches on xen that use the feature (and that also mention this review): https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg114160.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118355