This is an archive of the discontinued LLVM Phabricator instance.

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

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



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.


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'

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, 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"?


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.May 5 2022, 2:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2022, 2:43 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
xiangzhangllvm added inline comments.May 5 2022, 6:07 PM

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.May 5 2022, 11:26 PM

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, and here are patches on xen that use the feature (and that also mention this review):

xiangzhangllvm added inline comments.May 6 2022, 5:43 PM

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