[PATCH] D91898: [attributes] Add a facility for defining and enforcing a Trusted Computing Base.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D91898#2413004 <https://reviews.llvm.org/D91898#2413004>, @NoQ wrote:

>> if the TCB-based functions are a small subset of the code then this attribute is better
>
> Yes, that's exactly the case. We want most functions to be untrusted by default but also have no restrictions imposed or warnings emitted for them when they do their usual function things.

Excellent, thank you both for considering it!



================
Comment at: clang/include/clang/Basic/Attr.td:3585
+def EnforceTCB : InheritableAttr {
+  let Spellings = [GCC<"enforce_tcb">];
+  let Subjects = SubjectList<[Function]>;
----------------
I don't think GCC supports these attributes, so this spelling should probably be `Clang` rather than `GCC`.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5497
+  called from a function with the enforce_tcb attribute. A function may be
+  a part of multiple TCBs. Invocations of function pointers and C++ methods
+  are not checked. Builtins are considered to a part of every TCB.
----------------
Are there plans to support this on function pointers or other kinds of callable things?

Also, it seems rather odd that C++ methods are not checked. I could somewhat understand not checking virtual functions, but why not member functions where the target is known? Why not static member functions?


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1246
+
+def TCBEnforcement : DiagGroup<"tcb-enforcement">;
----------------
Are you planning on adding more diagnostics under this group? If not, I'd suggest defining it inline (see below).


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11052
+def warn_tcb_enforcement_violation : Warning<
+  "TCB [%0] has been violated by calling a function [%1] that is not in the TCB.">,
+  InGroup<TCBEnforcement>;
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11053
+  "TCB [%0] has been violated by calling a function [%1] that is not in the TCB.">,
+  InGroup<TCBEnforcement>;
 } // end of sema component.
----------------
...assuming the group won't be reused by too many diagnostics.


================
Comment at: clang/include/clang/Sema/Sema.h:12454
 
+  void CheckTCBEnforcement(CallExpr *TheCall,  FunctionDecl *Callee);
+
----------------
Can you make these `const` pointers?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16070
+void Sema::CheckTCBEnforcement(CallExpr *TheCall,  FunctionDecl *Callee) {
+  FunctionDecl *Caller = getCurFunctionDecl();
+
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:16072
+
+  // Calls to builtins are not enforced
+  if (!Caller || !Caller->hasAttr<EnforceTCBAttr>() || Callee->getBuiltinID() != 0) {
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:16073-16075
+  if (!Caller || !Caller->hasAttr<EnforceTCBAttr>() || Callee->getBuiltinID() != 0) {
+    return;
+  }
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:16079-16084
+  for (const auto *Attr : Callee->specific_attrs<EnforceTCBAttr>()) {
+    CalleeTCBs.insert(Attr->getTCBName());
+  }
+  for (const auto *Attr : Callee->specific_attrs<EnforceTCBLeafAttr>()) {
+    CalleeTCBs.insert(Attr->getTCBName());
+  }
----------------
Pretty sure you can remove the manual loops here with something like this.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16086
+
+  // Go through the TCBs the caller is a part of and emit warnings if Caller is in a TCB that the Callee is not
+  for (const auto *Attr : Caller->specific_attrs<EnforceTCBAttr>()) {
----------------
Can you be sure to run the patch through clang-format, this looks like it's over the 80 col limit.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:16088
+  for (const auto *Attr : Caller->specific_attrs<EnforceTCBAttr>()) {
+    llvm::StringRef CallerTCB = Attr->getTCBName();
+
----------------



================
Comment at: clang/lib/Sema/SemaChecking.cpp:16090-16092
+    if (CalleeTCBs.count(CallerTCB) == 0) {
+      Diag(TheCall->getExprLoc(), diag::warn_tcb_enforcement_violation) << CallerTCB << Callee->getName();
+    }
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7339
 
+template<typename Attr>
+static void handleEnforceTCBAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7344
+    return;
+  D->addAttr(Attr::Create(S.Context, Argument, AL));
+}
----------------



================
Comment at: clang/test/Sema/attr-enforce-tcb.c:1
+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
----------------



================
Comment at: clang/test/Sema/attr-enforce-tcb.c:15
+void foo9 (void);
+
+void
----------------
Some more interesting tests:
```
// Ensure that attribute merging works as expected across redeclarations.
void foo10() PLACE_IN_TCB("baz");
void foo10() PLACE_IN_TCB("bar");
void foo10() PLACE_IN_TCB("quux");

// Ensure that attribute instantiation works as expected.
template <typename Ty>
void foo11(Ty) PLACE_IN_TCB("bar");

// Basic checks
void foo12(void) __attribute__((enforce_tcb(12))); // wrong arg type
[[clang::enforce_tcb("oops")]] int foo13; // Wrong subject type
void foo14(void) __attribute__((enforce_tcb)); // Wrong number of arguments
void foo15(void) __attribute__((enforce_tcb("test", 12)); // Wrong number of arguments
// Add similar basic checks for enforce_tcb_leaf.
```


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

https://reviews.llvm.org/D91898