[PAC][ELF] Place AUTH constants in .data.rel.ro for position dependent code

[kovdan01]

For AArch64 target machine, effective relocation model on Windows and Darwin is always PIC, while for ELF targets Static is used when DynamicNoPIC is requested (see getEffectiveRelocModel in AArch64TargetMachine.cpp).

This resulted in using .rodata section for AUTH constants, which is wrong since these are filled with AUTH dynamic relocs and require the section to be writeable during dynamic relocation resolving.

This patch adds a check ensuring if the constant itself or one of the nested constants are AUTH ones. If so, use .data.rel.ro section.

[kovdan01]

This fixes the issue reported by @pcc in pauth channel in Discord: https://discord.com/channels/636084430946959380/1133112394701348895/1395957067847700630

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

Author: Daniil Kovalev (kovdan01)

Changes

For AArch64 target machine, effective relocation model on Windows and Darwin is always PIC, while for ELF targets Static is used when DynamicNoPIC is requested (see getEffectiveRelocModel in AArch64TargetMachine.cpp).

This resulted in using .rodata section for AUTH constants, which is wrong since these are filled with AUTH dynamic relocs and require the section to be writeable during dynamic relocation resolving.

This patch adds a check ensuring if the constant itself or one of the nested constants are AUTH ones. If so, use .data.rel.ro section.

---

Full diff: https://github.com/llvm/llvm-project/pull/150418.diff

2 Files Affected:

• (modified) llvm/lib/Target/TargetLoweringObjectFile.cpp (+17-2)
• (added) llvm/test/CodeGen/AArch64/ptrauth-global-no-pic.ll (+51)

diff --git a/llvm/lib/Target/TargetLoweringObjectFile.cpp b/llvm/lib/Target/TargetLoweringObjectFile.cpp
index 9b03e85ca45bf..246509b28cd9c 100644
--- a/llvm/lib/Target/TargetLoweringObjectFile.cpp
+++ b/llvm/lib/Target/TargetLoweringObjectFile.cpp
@@ -17,6 +17,7 @@
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
+#include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/IR/Module.h"
@@ -220,6 +221,20 @@ void TargetLoweringObjectFile::emitPseudoProbeDescMetadata(MCStreamer &Streamer,
   }
 }
 
+static bool containsConstantPtrAuth(const Constant *C) {
+  if (isa<ConstantPtrAuth>(C))
+    return true;
+
+  if (isa<BlockAddress>(C) || isa<GlobalValue>(C))
+    return false;
+
+  for (const Value *Op : C->operands())
+    if (containsConstantPtrAuth(cast<Constant>(Op)))
+      return true;
+
+  return false;
+}
+
 /// getKindForGlobal - This is a top-level target-independent classifier for
 /// a global object.  Given a global variable and information from the TM, this
 /// function classifies the global in a target independent manner. This function
@@ -327,9 +342,9 @@ SectionKind TargetLoweringObjectFile::getKindForGlobal(const GlobalObject *GO,
       // mergable section, because the linker doesn't take relocations into
       // consideration when it tries to merge entries in the section.
       Reloc::Model ReloModel = TM.getRelocationModel();
-      if (ReloModel == Reloc::Static || ReloModel == Reloc::ROPI ||
+      if ((ReloModel == Reloc::Static || ReloModel == Reloc::ROPI ||
           ReloModel == Reloc::RWPI || ReloModel == Reloc::ROPI_RWPI ||
-          !C->needsDynamicRelocation())
+          !C->needsDynamicRelocation()) && !containsConstantPtrAuth(C))
         return SectionKind::getReadOnly();
 
       // Otherwise, the dynamic linker needs to fix it up, put it in the
diff --git a/llvm/test/CodeGen/AArch64/ptrauth-global-no-pic.ll b/llvm/test/CodeGen/AArch64/ptrauth-global-no-pic.ll
new file mode 100644
index 0000000000000..651f384ef6497
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/ptrauth-global-no-pic.ll
@@ -0,0 +1,51 @@
+; RUN: llc -mtriple aarch64-elf --relocation-model=static         -mattr=+pauth -filetype=asm -o - %s | FileCheck %s
+; RUN: llc -mtriple aarch64-elf --relocation-model=dynamic-no-pic -mattr=+pauth -filetype=asm -o - %s | FileCheck %s
+
+;; A constant value, use .rodata
+; CHECK:         .section        .rodata,"a",@progbits
+; CHECK:         .globl  Const
+; CHECK: Const:
+; CHECK:         .xword  37
+
+;; An AUTH reloc is needed, use .data.rel.ro
+; CHECK:         .section        .data.rel.ro,"aw",@progbits
+; CHECK:         .globl  PtrAuthExtern
+; CHECK: PtrAuthExtern:
+; CHECK:         .xword  ConstExtern@AUTH(da,0)
+
+; CHECK:         .globl  PtrAuth
+; CHECK: PtrAuth:
+; CHECK:         .xword  Const@AUTH(da,0)
+
+; CHECK:         .globl  PtrAuthExternNested1
+; CHECK: PtrAuthExternNested1:
+; CHECK:         .xword  ConstExtern@AUTH(da,0)
+
+;; The address could be filled statically, use .rodata
+; CHECK:         .section        .rodata,"a",@progbits
+; CHECK:         .globl  PtrAuthExternNested2
+; CHECK: PtrAuthExternNested2:
+; CHECK:         .xword  PtrAuthExtern
+
+;; An AUTH reloc is needed, use .data.rel.ro
+; CHECK:         .section        .data.rel.ro,"aw",@progbits
+; CHECK:         .globl  PtrAuthNested1
+; CHECK: PtrAuthNested1:
+; CHECK:         .xword  Const@AUTH(da,0)
+
+;; The address could be filled statically, use .rodata
+; CHECK:         .section        .rodata,"a",@progbits
+; CHECK:         .globl  PtrAuthNested2
+; CHECK: PtrAuthNested2:
+; CHECK:         .xword  PtrAuth
+
+@ConstExtern = external global i64
+@Const       = constant i64 37
+
+@PtrAuthExtern = constant ptr ptrauth (ptr @ConstExtern, i32 2)
+@PtrAuth       = constant ptr ptrauth (ptr @Const,  i32 2)
+
+@PtrAuthExternNested1 = constant { ptr } { ptr ptrauth (ptr @ConstExtern, i32 2) }
+@PtrAuthExternNested2 = constant { ptr } { ptr @PtrAuthExtern }
+@PtrAuthNested1       = constant { ptr } { ptr ptrauth (ptr @Const, i32 2) }
+@PtrAuthNested2       = constant { ptr } { ptr @PtrAuth }

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[atrosinenko]

As far as I can see, -filetype=asm is the default in llc. Furthermore, it is advised to pass the input to opt via redirection - not sure if it has any relation to llc (except for uniformity), but using < %s makes -o - useless, too.

[kovdan01]

Thanks for suggestion! Fixed in d85f8cf. Sorry for a delay

[kovdan01]

Would be glad to see everyone's feedback on the changes.

[kovdan01]

Would be glad to see everyone's feedback on the changes.

[kovdan01]

Would be glad to see everyone's feedback on the changes.

[kovdan01]

Would be glad to see everyone's feedback on the changes.

[kovdan01]

This fixes the issue reported by @pcc in pauth channel in Discord: https://discord.com/channels/636084430946959380/1133112394701348895/1395957067847700630

@pcc Could you please take a look at this and check if this fixes the issue for you (since this PR was inspired by your issue report)?

[kovdan01]

This fixes the issue reported by @pcc in pauth channel in Discord: https://discord.com/channels/636084430946959380/1133112394701348895/1395957067847700630

@pcc Could you please take a look at this and check if this fixes the issue for you (since this PR was inspired by your issue report)?

Everyone else interested is also welcome to leave feedback :)

[atrosinenko]

IIUC this would make containsConstantPtrAuth return false for global variable like this

@global_var = global ptr ptrauth (ptr @f, i32 0)

which is still "constant" (as a constant pointer to global_var). This doesn't seem to hurt in case of this PR, but possibly a bit unexpected w.r.t. function name vs. its behavior.

[atrosinenko]

On the other hand, I would expect containsConstantPtrAuth to return false for @ptr_to_global_var = constant ptr @global_var no matter how @global_value is defined - I guess recursively inspecting GlobalValues would make containsConstantPtrAuth incorrectly return true in such cases. Thus, the original "issue" should be considered as a matter of comments or naming, if at all.

[atrosinenko]

[nit] Maybe it would be more readable to organize the checks along the lines

// ...
  } else {
    // First of all, the dynamic linker always needs to fix PtrAuth relocations up.
    if (containsPtrAuth(C))
      return SectionKind::getReadOnlyWithRel();

    // In static, ROPI and RWPI relocation models, the linker will resolve
    // ...
    if (ReloModel == ...)
      return SectionKind::getReadOnly();

    // Otherwise, the dynamic linker needs to fix it up, put it in the
    // writable data.rel section.
    return SectionKind::getReadOnlyWithRel();

[pcc]

I've always found it a bit confusing that Constant::needsDynamicRelocation really means "needs dynamic relocation in non-static/R*PI relocation models". Would it make sense to move the check for relocation model as well as the ptrauth constant detection (without the reloc model check of course) into needsDynamicRelocation? Then this could end up being:

if (!C->needsDynamicRelocation(TM.getRelocationModel()))
  return SectionKind::getReadOnly();

[kovdan01]

[nit] Maybe it would be more readable to organize the checks along the lines

@atrosinenko Applied your suggestion in cafc9df, thanks!

[kovdan01]

Would it make sense to move the check for relocation model as well as the ptrauth constant detection (without the reloc model check of course) into needsDynamicRelocation?

@pcc While I like the idea of simplifying things and making naming less confusing, I'm not sure if such a change would make logic for all the users of needsDynamicRelocation simpler. Also, it looks like that some callers do not have easy access to TargetMachine object, so they can't easily call getRelocationModel() and pass that to new needsDynamicRelocation updated accordingly to your suggestion.

We might think of such an improvement, but I guess this is out of scope of this PR and it's better not to combine actual bug fixes and refactoring.

Can we postpone applying your suggestion? And could you please tell if there is any other feedback on this PR or this can be merged? Thanks!