[PATCH] D71019: [AArch64] Emit PAC/BTI .note.gnu.property flags

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Apologies for the delay in responding. I've got a query about the alignment of the .note.gnu.property sections. As I understand it they should be 8-byte aligned for a 64-bit machine. I also think it would be helpful to warn that if at least one, but not all functions have the branch-target-enforcement attribute.

Other that that I think the change looks good.

Some links for other reviewers:

- for definition of .note.gnu.property https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf
- for AArch64 definitions of GNU_PROPERTY_AARCH64_FEATURE_1_PAC et-al. https://developer.arm.com/docs/ihi0056/f/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q2-documentation
- for X86_64 equivalent of BTI (IBT) https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf



================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:190
+  // Assemble feature flags that may require creation of a note section.
+  unsigned Flags = ELF::GNU_PROPERTY_AARCH64_FEATURE_1_BTI |
+                   ELF::GNU_PROPERTY_AARCH64_FEATURE_1_PAC;
----------------
There is a similar implementation in X86AsmPrinter that uses FeatureFlagsAnd rather than Flags, it may be worth using a similar naming convention as these are equivalent features. This isn't a strong opinion by any means, just a suggestion.


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:193
+
+  if (any_of(M, [](const Function &F) {
+        return !F.hasFnAttribute("branch-target-enforcement");
----------------

The case where at least one, but not all functions have the branch-target-enforcement attribute is somewhat problematic. 

At the moment BTI can only be enabled per link-unit, and if there is any object not marked with the .note.gnu.property section the linker will refuse to propagate the property into the image (without a command-line --force-bti). With that in mind clearing the property clears it for the entire link-unit, rather negating the point of using it in the first place. On the other hand not-clearing the property means that not all functions have branch-target-enforcement so claiming that they do could result in a run-time error if they are indirectly called.

I think it would be worth a warning if at least one, but not all functions have branch-target-protection.

The same case exists for PAC, below, however as I understand it, it is less important for the whole link-unit to be covered as it is with BTI.
 


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:214
+  OutStreamer->SwitchSection(Nt);
+
+  // Emit the note header.
----------------
On 64-bit architectures SHT_NOTE sections have 8-byte alignment. For .note.gnu.property there has been some discussion about this as the actual section contents don't require 8-byte alignment. My understanding is that the consensus came out as 8-byte alignment. This is what has been implemented in the X86AsmPrinter::EmitStartOfAsmFile. The binutils AArch64 assembler tests have 8-byte alignment. see https://patches-gcc.linaro.org/patch/16268/ property-bti-pac1.s

In summary I think we need to double check what GCC has done here, and add EmitAlignment directives if they are necessary. 



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

https://reviews.llvm.org/D71019