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

[Gabriel F. T. Gomes via Phabricator via cfe-commits]

gftg created this revision.
gftg added reviewers: xiangzhangllvm, pengfei, erichkeane, joaomoreira.
Herald added subscribers: ormris, dexonsmith, dang, jdoerfert, steven_wu, 
martong, hiraditya.
Herald added a reviewer: aaron.ballman.
gftg requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

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 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118355

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/AST/TypeProperties.td
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/CodeGen/X86/x86-mmanual-endbr.c
  clang/test/CodeGen/attributes.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-cf_check.c
  clang/test/Sema/attr-cf_check.cpp
  clang/test/Sema/cf_check_attr_not_allowed.c
  llvm/bindings/go/llvm/ir_test.go
  llvm/docs/BitCodeFormat.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/AsmParser/LLToken.h
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/include/llvm/IR/Function.h
  llvm/include/llvm/IR/InstrTypes.h
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86IndirectBranchTracking.cpp
  llvm/lib/Transforms/Utils/CodeExtractor.cpp
  llvm/test/CodeGen/X86/cf_check.ll
  llvm/test/CodeGen/X86/missing_cf_check.ll

Index: llvm/test/CodeGen/X86/missing_cf_check.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/missing_cf_check.ll
@@ -0,0 +1,71 @@
+; RUN: llc -mtriple=x86_64-unknown-unknown -x86-indirect-branch-tracking < %s 2>&1 | FileCheck %s
+
+
+;; Test that the compiler generates diagnostic messages for function pointer  ;;
+;; assignments in -mmanual-endbr mode.;;
+;; This program has been generated from the following source: ;;
+;;;;
+;; __attribute__((nocf_check)) void foo(void) {}  ;;
+;; __attribute__((cf_check)) void bar(void) {};;
+;; void baz(void) {}  ;;
+;;;;
+;; void foobar(void) {;;
+;;   void (*fooptr)(void) = &foo; /* Should always warn. */   ;;
+;;   void (*barptr)(void) = &bar; /* Should never warn. */;;
+;;   void (*bazptr)(void) = &baz; /* Should warn with -mmanual-endbr. */  ;;
+;;   (*fooptr)(); ;;
+;;   (*barptr)();

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

[Gabriel F. T. Gomes via Phabricator via cfe-commits]

gftg added a comment.

This patch as-is doesn't build. To build it needs 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118355

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits