Support ObjC Method call in enforce_tcb checks

[pdherbemont]

Summary

Before this change, calling an ObjC method from a C function marked with
the enforce_tcb attribute would not produce a warning.

After this change, we'll get a proper TCB warning when calling an ObjC method from a TCB. Also, this change enables ObjC methods to participate in a TCB.

1. Allow ObjC Methods to be decorated with the enforce_tcb attribute
2. Change the CheckTCBEnforcement() function to accept a NamedDecl (instead of the more specialized FunctionDecl) and a SourceLocation (instead of a CallExpr).
3. Call CheckTCBEnforcement() from CheckObjCMethodCall()
4. Add a test.

[NoQ]

Looks great! It might make sense to add tests to test/Sema/attr-enforce-tcb-errors.cpp to ensure that attribute conflicts are resolved correctly on Objective-C methods but also it's not like we've changed anything there. I have only one tiny nitpick.

clang/lib/Sema/SemaChecking.cpp

17371

This should be a plain cast<> because we already know that the object is of the right type so there's nothing dynamic about it.

[pdherbemont]

Removed the useless dyn_cast.

[pdherbemont]

Added clang/test/Sema/attr-enforce-tcb-errors.m based on clang/test/Sema/attr-enforce-tcb-errors.cpp.

[pdherbemont]

In D122343#3403714, @NoQ wrote:

Looks great! It might make sense to add tests to test/Sema/attr-enforce-tcb-errors.cpp to ensure that attribute conflicts are resolved correctly on Objective-C methods but also it's not like we've changed anything there. I have only one tiny nitpick.

Thanks for the review! I added a dedicated test/Sema/attr-enforce-tcb-errors.m. I thought it might be better to keep ObjC aside. Let me know if you want a different approach.

[NoQ]

Looks great, thanks! I'll wait a bit to see if other folks have something to say about this and then commit the patch for you.

[aaronpuchert]

Just some suggestions, feel free to ignore them.

clang/include/clang/Sema/Sema.h

12991–12992

Maybe Call[Expr]Loc or InvocationLoc so that we're clear what the expression is?

clang/lib/Sema/SemaChecking.cpp

5500–5502

You could check for builtins here, then the cast down there wouldn't be needed.

17370–17371

I'd suggest to hoist that check into CheckFunctionCall, then we don't need to cast at all. But feel free to keep it as is.

[aaron.ballman]

LGTM, though I agree with the suggestions from @aaronpuchert.

[pdherbemont]

Integrated Aaron review feedbacks:

• Use more expressive name CallExprLoc.
• Moved the builtin check one level up to avoid casting.

Thank you Aaron!

[pdherbemont]

Fixed clang-format.

[aaronpuchert]

Feel free to fix that on commit.

clang/include/clang/Sema/Sema.h

12991–12992

The source file calls this CallExprLoc, I think you forgot to rename it here.

[pdherbemont]

Updated CheckTCBEnforcement definition.

[pdherbemont]

clang/include/clang/Sema/Sema.h

12991–12992

The source file calls this CallExprLoc, I think you forgot to rename it here.

indeed!

[NoQ]

Great! I'll commit.

clang/include/clang/Basic/Attr.td

def Builtin : InheritableAttr {
   let Args = [UnsignedArgument<"ID">];
   let Subjects = SubjectList<[Function]>;
   let SemaHandler = 0;
   let Documentation = [Undocumented];
 }
 
 def EnforceTCB : InheritableAttr {
   let Spellings = [Clang<"enforce_tcb">];
-  let Subjects = SubjectList<[Function]>;
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
   let Args = [StringArgument<"TCBName">];
   let Documentation = [EnforceTCBDocs];
   bit InheritEvenIfAlreadyPresent = 1;
 }
 
 def EnforceTCBLeaf : InheritableAttr {
   let Spellings = [Clang<"enforce_tcb_leaf">];
-  let Subjects = SubjectList<[Function]>;
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
   let Args = [StringArgument<"TCBName">];
   let Documentation = [EnforceTCBLeafDocs];
   bit InheritEvenIfAlreadyPresent = 1;
 }
 
 def Error : InheritableAttr {
   let Spellings = [GCC<"error">, GCC<"warning">];
   let Accessors = [Accessor<"isError", [GCC<"error">]>,
                    Accessor<"isWarning", [GCC<"warning">]>];
   let Args = [StringArgument<"UserDiagnostic">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [ErrorAttrDocs];
 }

clang/include/clang/Sema/Sema.h

private:
   /// Check if the given expression contains 'break' or 'continue'
   /// statement that produces control flow different from GCC.
   void CheckBreakContinueBinding(Expr *E);
 
   /// Check whether receiver is mutable ObjC container which
   /// attempts to add itself into the container
   void CheckObjCCircularContainer(ObjCMessageExpr *Message);
 
-  void CheckTCBEnforcement(const CallExpr *TheCall, const FunctionDecl *Callee);
+  void CheckTCBEnforcement(const SourceLocation CallExprLoc,
+                           const NamedDecl *Callee);
 
   void AnalyzeDeleteExprMismatch(const CXXDeleteExpr *DE);
   void AnalyzeDeleteExprMismatch(FieldDecl *Field, SourceLocation DeleteLoc,
                                  bool DeleteWasArrayForm);
 public:
   /// Register a magic integral constant to be used as a type tag.
   void RegisterTypeTagForDatatype(const IdentifierInfo *ArgumentKind,
                                   uint64_t MagicValue, QualType Type,

clang/lib/Sema/SemaChecking.cpp

checkCall(FDecl, Proto, ImplicitThis, llvm::makeArrayRef(Args, NumArgs),
             TheCall->getCallee()->getSourceRange(), CallType);
 
   IdentifierInfo *FnInfo = FDecl->getIdentifier();
   // None of the checks below are needed for functions that don't have
   // simple names (e.g., C++ conversion functions).
   if (!FnInfo)
     return false;
 
-  CheckTCBEnforcement(TheCall, FDecl);
+  // Enforce TCB except for builtin calls, which are always allowed.
+  if (FDecl->getBuiltinID() == 0)
+    CheckTCBEnforcement(TheCall->getExprLoc(), FDecl);
 
   CheckAbsoluteValueFunction(TheCall, FDecl);
   CheckMaxUnsignedZero(TheCall, FDecl);
 
   if (getLangOpts().ObjC)
     DiagnoseCStringFormatDirectiveInCFAPI(*this, FDecl, Args, NumArgs);
 
   unsigned CMId = FDecl->getMemoryFunctionKind();
bool Sema::CheckObjCMethodCall(ObjCMethodDecl *Method, SourceLocation lbrac,
                                ArrayRef<const Expr *> Args) {
   VariadicCallType CallType =
       Method->isVariadic() ? VariadicMethod : VariadicDoesNotApply;
 
   checkCall(Method, nullptr, /*ThisArg=*/nullptr, Args,
             /*IsMemberFunction=*/false, lbrac, Method->getSourceRange(),
             CallType);
 
+  CheckTCBEnforcement(lbrac, Method);
+
   return false;
 }
 
 bool Sema::CheckPointerCall(NamedDecl *NDecl, CallExpr *TheCall,
                             const FunctionProtoType *Proto) {
   QualType Ty;
   if (const auto *V = dyn_cast<VarDecl>(NDecl))
     Ty = V->getType().getNonReferenceType();
ExprResult Sema::SemaBuiltinMatrixColumnMajorStore(CallExpr *TheCall,
 
   return CallResult;
 }
 
 /// \brief Enforce the bounds of a TCB
 /// CheckTCBEnforcement - Enforces that every function in a named TCB only
 /// directly calls other functions in the same TCB as marked by the enforce_tcb
 /// and enforce_tcb_leaf attributes.
-void Sema::CheckTCBEnforcement(const CallExpr *TheCall,
-                               const FunctionDecl *Callee) {
-  const FunctionDecl *Caller = getCurFunctionDecl();
-
-  // Calls to builtins are not enforced.
-  if (!Caller || !Caller->hasAttr<EnforceTCBAttr>() ||
-      Callee->getBuiltinID() != 0)
+void Sema::CheckTCBEnforcement(const SourceLocation CallExprLoc,
+                               const NamedDecl *Callee) {
+  const NamedDecl *Caller = getCurFunctionOrMethodDecl();
+
+  if (!Caller || !Caller->hasAttr<EnforceTCBAttr>())
     return;
 
   // Search through the enforce_tcb and enforce_tcb_leaf attributes to find
   // all TCBs the callee is a part of.
   llvm::StringSet<> CalleeTCBs;
   for_each(Callee->specific_attrs<EnforceTCBAttr>(),
            [&](const auto *A) { CalleeTCBs.insert(A->getTCBName()); });
   for_each(Callee->specific_attrs<EnforceTCBLeafAttr>(),
            [&](const auto *A) { CalleeTCBs.insert(A->getTCBName()); });
 
   // 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_each(
       Caller->specific_attrs<EnforceTCBAttr>(),
       [&](const auto *A) {
         StringRef CallerTCB = A->getTCBName();
         if (CalleeTCBs.count(CallerTCB) == 0) {
-          this->Diag(TheCall->getExprLoc(),
-                     diag::warn_tcb_enforcement_violation) << Callee
-                                                           << CallerTCB;
+          this->Diag(CallExprLoc, diag::warn_tcb_enforcement_violation)
+              << Callee << CallerTCB;
         }
       });
 }

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: Convergent (SubjectMatchRule_function)
 // CHECK-NEXT: DLLExport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: DLLImport (SubjectMatchRule_function, SubjectMatchRule_variable, SubjectMatchRule_record, SubjectMatchRule_objc_interface)
 // CHECK-NEXT: Destructor (SubjectMatchRule_function)
 // CHECK-NEXT: DiagnoseAsBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: DisableSanitizerInstrumentation (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: DisableTailCalls (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: EnableIf (SubjectMatchRule_function)
-// CHECK-NEXT: EnforceTCB (SubjectMatchRule_function)
-// CHECK-NEXT: EnforceTCBLeaf (SubjectMatchRule_function)
+// CHECK-NEXT: EnforceTCB (SubjectMatchRule_function, SubjectMatchRule_objc_method)
+// CHECK-NEXT: EnforceTCBLeaf (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum)
 // CHECK-NEXT: Error (SubjectMatchRule_function)
 // CHECK-NEXT: ExcludeFromExplicitInstantiation (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_implementation, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
 // CHECK-NEXT: FlagEnum (SubjectMatchRule_enum)
 // CHECK-NEXT: Flatten (SubjectMatchRule_function)
 // CHECK-NEXT: GNUInline (SubjectMatchRule_function)
 // CHECK-NEXT: HIPManaged (SubjectMatchRule_variable)

clang/test/Sema/attr-enforce-tcb-errors.m

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int foo();
+
+__attribute__((objc_root_class))
+@interface AClass
+- (void)bothTCBAndTCBLeafOnSeparateRedeclarations __attribute__((enforce_tcb("x"))); // expected-note{{conflicting attribute is here}}
+
+- (void)bothTCBAndTCBLeafOnSeparateRedeclarationsOppositeOrder __attribute__((enforce_tcb_leaf("x"))); // expected-note{{conflicting attribute is here}}
+
+- (void)bothTCBAndTCBLeafButDifferentIdentifiersOnSeparateRedeclarations __attribute__((enforce_tcb("x")));
+
+- (void)bothTCBAndTCBLeafButDifferentIdentifiersOnSeparateRedeclarationsOppositeOrder __attribute__((enforce_tcb_leaf("x")));
+
+- (void)onInterfaceOnly __attribute__((enforce_tcb("test")));
+@end
+
+@interface AClass (NoImplementation)
+- (void)noArguments __attribute__((enforce_tcb)); // expected-error{{'enforce_tcb' attribute takes one argument}}
+
+- (void)tooManyArguments __attribute__((enforce_tcb("test", 12))); // expected-error{{'enforce_tcb' attribute takes one argument}}
+
+- (void)wrongArgumentType __attribute__((enforce_tcb(12))); // expected-error{{'enforce_tcb' attribute requires a string}}
+
+- (void)noArgumentsLeaf __attribute__((enforce_tcb_leaf)); // expected-error{{'enforce_tcb_leaf' attribute takes one argument}}
+
+- (void)tooManyArgumentsLeaf __attribute__((enforce_tcb_leaf("test", 12))); // expected-error{{'enforce_tcb_leaf' attribute takes one argument}}
+
+- (void)wrongArgumentTypeLeaf __attribute__((enforce_tcb_leaf(12))); // expected-error{{'enforce_tcb_leaf' attribute requires a string}}
+@end
+
+@implementation AClass
+- (void)onInterfaceOnly {
+  foo(); // expected-warning{{calling 'foo' is a violation of trusted computing base 'test'}}
+}
+
+- (void)bothTCBAndTCBLeaf
+    __attribute__((enforce_tcb("x")))
+    __attribute__((enforce_tcb_leaf("x"))) // expected-error{{attributes 'enforce_tcb_leaf("x")' and 'enforce_tcb("x")' are mutually exclusive}}
+{
+  foo(); // no-warning
+}
+
+- (void)bothTCBAndTCBLeafOnSeparateRedeclarations
+    __attribute__((enforce_tcb_leaf("x"))) // expected-error{{attributes 'enforce_tcb_leaf("x")' and 'enforce_tcb("x")' are mutually exclusive}}
+{
+  // Error recovery: no need to emit a warning when we didn't
+  // figure out our attributes to begin with.
+  foo(); // no-warning
+}
+
+- (void)bothTCBAndTCBLeafOppositeOrder
+    __attribute__((enforce_tcb_leaf("x")))
+    __attribute__((enforce_tcb("x"))) // expected-error{{attributes 'enforce_tcb("x")' and 'enforce_tcb_leaf("x")' are mutually exclusive}}
+{
+  foo(); // no-warning
+}
+
+- (void)bothTCBAndTCBLeafOnSeparateRedeclarationsOppositeOrder
+    __attribute__((enforce_tcb("x"))) // expected-error{{attributes 'enforce_tcb("x")' and 'enforce_tcb_leaf("x")' are mutually exclusive}}
+{
+  foo(); // no-warning
+}
+
+- (void)bothTCBAndTCBLeafButDifferentIdentifiers
+    __attribute__((enforce_tcb("x")))
+    __attribute__((enforce_tcb_leaf("y"))) // no-error
+{
+  foo(); // expected-warning{{calling 'foo' is a violation of trusted computing base 'x'}}
+}
+
+- (void)bothTCBAndTCBLeafButDifferentIdentifiersOppositeOrder
+    __attribute__((enforce_tcb_leaf("x")))
+    __attribute__((enforce_tcb("y"))) // no-error
+{
+  foo(); // expected-warning{{calling 'foo' is a violation of trusted computing base 'y'}}
+}
+
+- (void)bothTCBAndTCBLeafButDifferentIdentifiersOnSeparateRedeclarations
+    __attribute__((enforce_tcb_leaf("y"))) // no-error
+{
+  foo(); // expected-warning{{calling 'foo' is a violation of trusted computing base 'x'}}
+}
+
+- (void)bothTCBAndTCBLeafButDifferentIdentifiersOnSeparateRedeclarationsOppositeOrder
+    __attribute__((enforce_tcb("y"))) {
+  foo(); // expected-warning{{calling 'foo' is a violation of trusted computing base 'y'}}
+}
+
+- (void)errorRecoveryOverIndividualTCBs
+    __attribute__((enforce_tcb("y")))
+    __attribute__((enforce_tcb("x")))
+    __attribute__((enforce_tcb_leaf("x"))) // expected-error{{attributes 'enforce_tcb_leaf("x")' and 'enforce_tcb("x")' are mutually exclusive}}
+{
+  // FIXME: Ideally this should warn. The conflict between attributes
+  // for TCB "x" shouldn't affect the warning about TCB "y".
+  foo(); // no-warning
+}
+
+@end

clang/test/Sema/attr-enforce-tcb.m

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define PLACE_IN_TCB(NAME) __attribute__((enforce_tcb(NAME)))
+#define PLACE_IN_TCB_LEAF(NAME) __attribute__((enforce_tcb_leaf(NAME)))
+
+__attribute__((objc_root_class))
+@interface AClass
+@property(readonly) id propertyNotInAnyTCB;
+@end
+
+@implementation AClass
+- (void)inTCBFoo PLACE_IN_TCB("foo") {
+  [self notInAnyTCB]; // expected-warning{{calling 'notInAnyTCB' is a violation of trusted computing base 'foo'}}
+}
+- (void)inTCBFooAsLeaf PLACE_IN_TCB_LEAF("foo") {
+  [self notInAnyTCB]; // no-warning
+}
+- (void)notInAnyTCB {
+}
++ (void)classMethodNotInAnyTCB {
+}
++ (void)classMethodInTCBFoo PLACE_IN_TCB("foo") {
+  [self inTCBFoo];       // no-warning
+  [self inTCBFooAsLeaf]; // no-warning
+  [self notInAnyTCB];    // expected-warning{{calling 'notInAnyTCB' is a violation of trusted computing base 'foo'}}
+}
+@end
+
+PLACE_IN_TCB("foo")
+void call_objc_method(AClass *object) {
+  [object inTCBFoo];                // no-warning
+  [object inTCBFooAsLeaf];          // no-warning
+  [object notInAnyTCB];             // expected-warning{{calling 'notInAnyTCB' is a violation of trusted computing base 'foo'}}
+  [AClass classMethodNotInAnyTCB];  // expected-warning{{calling 'classMethodNotInAnyTCB' is a violation of trusted computing base 'foo'}}
+  [AClass classMethodInTCBFoo];     // no-warning
+  (void)object.propertyNotInAnyTCB; // expected-warning{{calling 'propertyNotInAnyTCB' is a violation of trusted computing base 'foo'}}
+}