Page MenuHomePhabricator

Add -mmanual-endbr switch to allow manual selection of control-flow protection
Needs ReviewPublic

Authored by gftg on Jan 27 2022, 5:38 AM.

Details

Summary

GCC has plans [1] to add a new switch that enables finer-grained control
of the insertion of CET stuff in generated code. This patch duplicates
their implementation within LLVM, in the hope that it can also be used
by Xen maintainers.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102953

---8<---
With -fcf-protection=branch, clang automatically adds control-flow
protection to indirect calls and jumps. On X86, this translates to ENDBR
instructions being added to the prologues of functions.

This patch adds a new switch, '-mmanual-endbr', which tells the compiler
that, even though -fcf-protection is in use, functions should not get
the instrumentation automatically. Instead, it allows users to manually
add the new attribute, 'cf_check', to functions that require it.

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.

Thus, when the backend detects that a function could be reached from an
indirect jump (e.g. when it has its address taken, or belongs to the
exported set of functions), a diagnostic warning is emitted, which
should help developers find missing occurrences of the 'cf_check'
attribute.

Depends on D118052.

Diff Detail

Event Timeline

gftg created this revision.Jan 27 2022, 5:38 AM
gftg requested review of this revision.Jan 27 2022, 5:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 27 2022, 5:38 AM
gftg added a comment.EditedJan 27 2022, 5:48 AM

This patch as-is doesn't build. Building it requires another change that I know is wrong, so I'm posting it below to ask for your help:

    [RFC] How to add more bits to the Type class?

    After reading https://reviews.llvm.org/D50630, I learned that keeping
    the size of the 'Type' class to a minimum (currently 24 bytes, where 8
    bytes are due to the various Bit Field classes) is desired. However,
    while trying to add a new function attribute (see the precious patches
    in this series), the new bit caused FunctionTypeBitfields to become 4
    bytes larger (previously 8 bytes), which becomes the larger member of
    the Bit Field union. The whole Type class increased in size to 32 bytes
    up from 24 (an 8-byte increase).

    This patch does the easy workaround of letting the size of the Type
    class grow to 32 bytes. However, I would like to know how to avoid this
    while still being able to add the new function attribute. I know it's a
    lot to ask, but could somebody teach me how to do this "the proper way"?

    Cheers,
    Gabriel

diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 7a00c7d2be8c..4d359d08a522 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1824,7 +1824,7 @@ protected:
   Type(TypeClass tc, QualType canon, TypeDependence Dependence)
       : ExtQualsTypeCommonBase(this,
                                canon.isNull() ? QualType(this_(), 0) : canon) {
-    static_assert(sizeof(*this) <= 8 + sizeof(ExtQualsTypeCommonBase),
+    static_assert(sizeof(*this) <= 16 + sizeof(ExtQualsTypeCommonBase),
                   "changing bitfields changed sizeof(Type)!");
     static_assert(alignof(decltype(*this)) % sizeof(void *) == 0,
                   "Insufficient alignment!");

I realize that sending extra diffs here is probably not how this community works, but please bear with me while I get used to it (I'm a newcomer).

ormris removed a subscriber: ormris.Jan 27 2022, 10:02 AM
martong removed a subscriber: martong.Thu, May 5, 2:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, May 5, 2:43 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
xiangzhangllvm added inline comments.Thu, May 5, 6:07 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
156–161

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}
 }
}
joaomoreira added inline comments.Thu, May 5, 11:26 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
156–161

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

xiangzhangllvm added inline comments.Fri, May 6, 5:43 PM
llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
156–161

Thanks! Ok, for "limit number of ENDBR instructions to reduce program size" here the code logic is make sense.