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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
