[clang] Do not instrument relative vtables under hwasan

[leonardchan]

Summary

Full context in https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=107017.

Instrumenting hwasan with globals results in a linker error under the relative vtables abi:

ld.lld: error: libunwind.cpp:(.rodata..L_ZTVN9libunwind12UnwindCursorINS_17LocalAddressSpaceENS_15Registers_arm64EEE.hwasan+0x8): relocation R_AARCH64_PLT32 out of range: 6845471433603167792 is not in [-2147483648, 2147483647]; references libunwind::AbstractUnwindCursor::~AbstractUnwindCursor()
>>> defined in libunwind/src/CMakeFiles/unwind_shared.dir/libunwind.cpp.obj

This is because the tag is included in the vtable address when calculating the offset between the vtable and virtual function. A temporary solution until we can resolve this is to just disable hwasan instrumentation on relative vtables specifically, which can be done in the frontend.

[mcgrathr]

lgtm.
You might add some comments in CGVTables.cpp about why this is done and what the alternatives might be in the future.

[leonardchan]

In D132425#3743276, @mcgrathr wrote:

lgtm.
You might add some comments in CGVTables.cpp about why this is done and what the alternatives might be in the future.

Done.

[hctim]

Glad to see that refactoring the sanitizer metadata made someone's life easier ;) (now allowing for disabling hwasanificiation of globals)

Patch looks reasonable to me. Can you please add the negative test (that vtables under the vanilla ABI still have hwasan)?

I wans't fully aware of the relative vtables ABI, and it may have some implications about MTE globals tagging (draft abi). Because logical tags are synthesized at runtime into a synthetic GOT entry - dynamic relocations I believe would be forced (removing any benefit of the relative vtables ABI), so for now it seems like MTE globals and relative vtables are mutually exclusive. Another option would be to disable MTE globals for relative vtables as well. No action needed on your part, just putting some wordso n paper that this might need some consideration at a later date if Fuchsia wants to support MTE globals.

clang/test/CodeGenCXX/RelativeVTablesABI/relative-vtables-hwasan.cpp

2

Can you add a note here that -triple=aarch64-unknown-fuchsia has implicit relative vtables

[leonardchan]

In D132425#3752468, @hctim wrote:

Glad to see that refactoring the sanitizer metadata made someone's life easier ;) (now allowing for disabling hwasanificiation of globals)

Patch looks reasonable to me. Can you please add the negative test (that vtables under the vanilla ABI still have hwasan)?

Yup. Will add a followup patch.

I wans't fully aware of the relative vtables ABI, and it may have some implications about MTE globals tagging (draft abi). Because logical tags are synthesized at runtime into a synthetic GOT entry - dynamic relocations I believe would be forced (removing any benefit of the relative vtables ABI), so for now it seems like MTE globals and relative vtables are mutually exclusive. Another option would be to disable MTE globals for relative vtables as well. No action needed on your part, just putting some wordso n paper that this might need some consideration at a later date if Fuchsia wants to support MTE globals.

Thanks for bringing this up. For relative vtables, the important bit is that the vtable is populated with statically known offsets between the vtable and its virtual functions. The particular linker error this patch resolves occurs because the hwasan pass happens to replace references to the vtable (within the vtable) with the alias, so the tag causes an overflow when resolving the relocation.

Now if I'm understanding the draft correctly, the way MTE will work with globals is that all references to globals *must* go through the GOT because the dynamic linker will place the tagged address there, so the potential conflict with relative vtables is that we wouldn't know the full tagged address of the vtable at link time, hence the dynamic relocation. If so, simply disabling MTE on relative vtables would also be a short term solution.

We have a generic long term solution for hwasan+RV which I think might also be applicable for MTE+RV. For hwasan, since it's mainly the IR pass that converts usages of the vtable (within the vtable itself) to use tagged aliases, the ideal solution is to just have hwasan ignore these specific references in the vtable such that offset calculation can continue to use the untagged address allowing the relocation arithmetic to not overflow. Now for llvm, I'm assuming it's an instrumentation pass like memtagsanitizer that will ensure all references to globals go through the GOT by replacing all global references with the appropriate IR that gets lowered to this GOT reference. If this is the case, then I *think* a similar solution can be done here where particular references to the vtable continue to use the original vtable address and avoid instrumentation.

[hctim]

In D132425#3753065, @leonardchan wrote:

We have a generic long term solution for hwasan+RV which I think might also be applicable for MTE+RV. For hwasan, since it's mainly the IR pass that converts usages of the vtable (within the vtable itself) to use tagged aliases, the ideal solution is to just have hwasan ignore these specific references in the vtable such that offset calculation can continue to use the untagged address allowing the relocation arithmetic to not overflow. Now for llvm, I'm assuming it's an instrumentation pass like memtagsanitizer that will ensure all references to globals go through the GOT by replacing all global references with the appropriate IR that gets lowered to this GOT reference. If this is the case, then I *think* a similar solution can be done here where particular references to the vtable continue to use the original vtable address and avoid instrumentation.

HWASan and MTE have a nice invariant that helps - functions aren't tagged (phew). IIUC, For HWASan, it seems like you could just use an _NC relocation and truncate off the tag bits when materializing a function pointer from the relative vtable. For MTE, taking the address of the vtable would be indirect (as it has to be grabbed from the GOT), and applying the offset would result in a tagged function pointer. Because code pages aren't mapped as PROT_MTE, seems like this would succeed (maybe unwinders would have to be taught to truncate any tag bits, but that seems about it).

Either way, I don't think we should worry about it right this instant, and any problems would be easily detected during experimentation.

Didn't actually realise this was submitted. Appreciate the follow-up patch for non-relative-vtables + hwasan :).

clang/lib/CodeGen/CGVTables.h

public:
   llvm::Type *getVTableType(const VTableLayout &layout);
 
   /// Generate a public facing alias for the vtable and make the vtable either
   /// hidden or private. The alias will have the original linkage and visibility
   /// of the vtable. This is used for cases under the relative vtables ABI
   /// when a vtable may not be dso_local.
   void GenerateRelativeVTableAlias(llvm::GlobalVariable *VTable,
                                    llvm::StringRef AliasNameRef);
+
+  /// Specify a vtable should not be instrumented with hwasan.
+  void RemoveHwasanMetadata(llvm::GlobalValue *VTable);
 };
 
 } // end namespace CodeGen
 } // end namespace clang
 #endif

clang/lib/CodeGen/CGVTables.cpp

llvm::GlobalVariable *CodeGenVTables::GenerateConstructionVTable(
 
   // Set properties only after the initializer has been set to ensure that the
   // GV is treated as definition and not declaration.
   assert(!VTable->isDeclaration() && "Shouldn't set properties on declaration");
   CGM.setGVProperties(VTable, RD);
 
   CGM.EmitVTableTypeMetadata(RD, VTable, *VTLayout.get());
 
-  if (UsingRelativeLayout && !VTable->isDSOLocal())
+  if (UsingRelativeLayout) {
+    RemoveHwasanMetadata(VTable);
+    if (!VTable->isDSOLocal())
       GenerateRelativeVTableAlias(VTable, OutName);
+  }
 
   return VTable;
 }
 
+void CodeGenVTables::RemoveHwasanMetadata(llvm::GlobalValue *VTable) {
+  if (CGM.getLangOpts().Sanitize.has(SanitizerKind::HWAddress)) {
+    llvm::GlobalValue::SanitizerMetadata Meta;
+    if (VTable->hasSanitizerMetadata())
+      Meta = VTable->getSanitizerMetadata();
+    Meta.NoHWAddress = true;
+    VTable->setSanitizerMetadata(Meta);
+  }
+}
+
 // If the VTable is not dso_local, then we will not be able to indicate that
 // the VTable does not need a relocation and move into rodata. A frequent
 // time this can occur is for classes that should be made public from a DSO
 // (like in libc++). For cases like these, we can make the vtable hidden or
 // private and create a public alias with the same visibility and linkage as
 // the original vtable type.
 void CodeGenVTables::GenerateRelativeVTableAlias(llvm::GlobalVariable *VTable,
                                                  llvm::StringRef AliasNameRef) {

clang/lib/CodeGen/ItaniumCXXABI.cpp

if (!VTable->isDeclarationForLinker() ||
     // @llvm.compiler.used so that it isn't deleted before whole program
     // analysis.
     if (VTable->isDeclarationForLinker()) {
       assert(CGM.getCodeGenOpts().WholeProgramVTables);
       CGM.addCompilerUsedGlobal(VTable);
     }
   }
 
-  if (VTContext.isRelativeLayout() && !VTable->isDSOLocal())
+  if (VTContext.isRelativeLayout()) {
+    CGVT.RemoveHwasanMetadata(VTable);
+    if (!VTable->isDSOLocal())
       CGVT.GenerateRelativeVTableAlias(VTable, VTable->getName());
   }
+}
 
 bool ItaniumCXXABI::isVirtualOffsetNeededForVTableField(
     CodeGenFunction &CGF, CodeGenFunction::VPtr Vptr) {
   if (Vptr.NearestVBase == nullptr)
     return false;
   return NeedsVTTParameter(CGF.CurGD);
 }
 

clang/test/CodeGenCXX/RelativeVTablesABI/relative-vtables-hwasan.cpp

+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -S -o - -emit-llvm -fsanitize=hwaddress | FileCheck %s
+
+/// The usual vtable will have default visibility. In this case, the actual
+/// vtable is hidden and the alias is made public. With hwasan enabled, we want
+/// to ensure the local one self-referenced in the hidden vtable is not
+/// hwasan-instrumented.
+// CHECK-DAG: @_ZTV1A.local = private unnamed_addr constant { [3 x i32] } { [3 x i32] [i32 0, i32 trunc (i64 sub (i64 ptrtoint (ptr @_ZTI1A.rtti_proxy to i64), i64 ptrtoint (ptr getelementptr inbounds ({ [3 x i32] }, ptr @_ZTV1A.local, i32 0, i32 0, i32 2) to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr dso_local_equivalent @_ZN1A3fooEv to i64), i64 ptrtoint (ptr getelementptr inbounds ({ [3 x i32] }, ptr @_ZTV1A.local, i32 0, i32 0, i32 2) to i64)) to i32)] }, no_sanitize_hwaddress, align 4
+// CHECK-DAG: @_ZTV1A = unnamed_addr alias { [3 x i32] }, ptr @_ZTV1A.local
+
+class A {
+public:
+  virtual void foo();
+};
+
+void A::foo() {}
+
+void A_foo(A *a) {
+  a->foo();
+}
+
+/// If the vtable happens to be hidden, then the alias is not needed. In this
+/// case, the original vtable struct itself should be unsanitized.
+// CHECK-DAG: @_ZTV1B = hidden unnamed_addr constant { [3 x i32] } { [3 x i32] [i32 0, i32 trunc (i64 sub (i64 ptrtoint (ptr @_ZTI1B.rtti_proxy to i64), i64 ptrtoint (ptr getelementptr inbounds ({ [3 x i32] }, ptr @_ZTV1B, i32 0, i32 0, i32 2) to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint (ptr dso_local_equivalent @_ZN1B3fooEv to i64), i64 ptrtoint (ptr getelementptr inbounds ({ [3 x i32] }, ptr @_ZTV1B, i32 0, i32 0, i32 2) to i64)) to i32)] }, no_sanitize_hwaddress, align 4
+
+class __attribute__((visibility("hidden"))) B {
+public:
+  virtual void foo();
+};
+
+void B::foo() {}
+
+void B_foo(B *b) {
+  b->foo();
+}