[PATCH] D62609: [LLD][ELF][AArch64] Support for BTI and PAC

Mon Jun 3 08:41:33 PDT 2019

peter.smith marked 14 inline comments as done.
peter.smith added a comment.

Thanks very much for the comments and questions. I'll upload a new patch in a second.

================
Comment at: ELF/Arch/AArch64.cpp:537
+void AArch64BtiPac::writePltHeader(uint8_t *Buf) const {
+  if (Config->AndFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
+    writePltHeaderBti(Buf);
----------------
MaskRay wrote:
> Should the condition be ANDed with `!Config->Shared`? Currently with `--force-bti` a shared object may 
No. PLT[n] does an indirect branch "br x17" the destination of which will be PLT[0] in the case of lazy loading. I guess with -zbind-now this could be done.

Standard PLT entry for reference.
```
adrp x16, Page(&(.plt.got[n]))
ldr  x17, [x16, Offset(&(.plt.got[n]))]
add  x16, x16, Offset(&(.plt.got[n]))
br   x17  // Indirect branch, possibly to PLT[0]
```

================
Comment at: ELF/Arch/AArch64.cpp:543
+
+void AArch64BtiPac::writePltBti(uint8_t *Buf, uint64_t GotPltEntryAddr,
+                                uint64_t PltEntryAddr, int32_t Index,
----------------
MaskRay wrote:
> Have you considered the alternative: rather than define 4 writePlt*, define a unified one:
> 
> ```
> if Bti
>   write bti c
> write adrp/ldr/add
> if Pac
>   write autiax16x17
> write br x17
> write padding nop(s)
> ```
I did try that at first, the disadvantage, particularly for the BTI is that the offsets for the relocations change and it gets a bit messy. Not overly so, but more difficult to follow on first reading. I'm happy to do that if you'd prefer?

================
Comment at: ELF/Arch/AArch64.cpp:568
+      0x10, 0x02, 0x00, 0x91, // add  x16, x16, Offset(&(.plt.got[n]))
+      0x9f, 0x21, 0x03, 0xd5, // autiax16x17
+      0x20, 0x02, 0x1f, 0xd6, // br   x17
----------------
MaskRay wrote:
> Is this `autia1716`?
Yes it is thanks. The 1716, means authorize x17 in the context of x16, my brain still wants to write 16 before 17.

================
Comment at: ELF/Arch/AArch64.cpp:587
+      0x10, 0x02, 0x00, 0x91, // add  x16, x16, Offset(&(.plt.got[n]))
+      0x9f, 0x21, 0x03, 0xd5, // autiax16x17
+      0x20, 0x02, 0x1f, 0xd6  // br   x17
----------------
MaskRay wrote:
> Is this `autia1716`?
Yes it is thanks.

================
Comment at: ELF/Arch/AArch64.cpp:617
+
+static TargetInfo *getTargetInfo() {
+  if (Config->AndFeatures & (GNU_PROPERTY_AARCH64_FEATURE_1_BTI |
----------------
MaskRay wrote:
> `Config->AndFeatures` hasn't been computed when `getTargetInfo` is called.
> 
> ```
> Target = getTarget();
> ...
> link<ELF64LE>(Args);  // Config->AndFeatures is computed inside this function
> ```
> 
At the moment getTarget() is called in two places. The first time will get the AArch64, the second will get AArch64BtiPac. I think that this is ok as the only difference is the PLT generation and that only matters later in the link. The same will be the case for the Intel CET patch D59780

Not sure if there is a good way around this yet. 

================
Comment at: ELF/Driver.cpp:340
     error("--require-cet may not be used with -z retpolineplt");
 }

----------------
MaskRay wrote:
> What about adding
> 
>     if (Config->EMachine != EM_AARCH64) {
>       if (Config->PacPlt) error(...);
>       if (Config->ForceBTI) error(...);
>     }
> 
> here? Then, below, 
> 
>     if (Config->EMachine == EM_AARCH64 && Config->ForceBTI &&
> 
> The `Config->EMachine == EM_AARCH64` check can be removed.
Good idea, thanks.

================
Comment at: ELF/Driver.cpp:1624
+  if (Config->EMachine == EM_AARCH64 && Config->PacPlt)
+      Ret |= GNU_PROPERTY_AARCH64_FEATURE_1_PAC;
+
----------------
MaskRay wrote:
> not 2-space indented.
> 
> I have a question: does `DT_AARCH64_BTI_PLT` make sense for a shared object? If not, you may apply the following here:
> 
> ```
> if (!Config->Shared)
>   Ret &= ~GNU_PROPERTY_AARCH64_FEATURE_1_BTI;
> ```
> 
> If yes, in `getTargetInfo()`, you may want to check `!Config->Shared` beside `GNU_PROPERTY_AARCH64_FEATURE_1_BTI`.
Fixed indentation, thanks for the spot. 

We definitely need to propagate GNU_PROPERTY_AARCH64_FEATURE_1_BTI for shared objects, this communicates that all the input relocatable objects and any linker generated content has BTI instructions in the right place.

The PLT[0] sequence does change for shared objects as PLT[0] requires a BTI c (PLT[n] indirectly calls PLT[0]) so even for a Shared Object getTargetInfo() needs to do something different. 

I guess it is possible that DT_AARCH64_BTI_PLT doesn't add a lot of value here although the wording in the ABI https://developer.arm.com/docs/ihi0056/latest/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q1-documentation requires it so I'd prefer to stick to that and ld.bfd.

================
Comment at: ELF/SyntheticSections.cpp:1348
+    if (Config->AndFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
+      addInt(DT_AARCH64_BTI_PLT, 0);
+    if (Config->AndFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_PAC)
----------------
MaskRay wrote:
> The ABI chooeses two `DT_*` tags (and with a value of 0! not 1) instead of a single `DT_*` with 2 bits seems a bit wasteful...
Possibly although there is quite a range to choose from [0x70000000, 0x80000000). My limited understanding is that two were chosen as it is faster for a dynamic loader to check for the presence, rather than check the value as well although I don't know how significant this is in practice.

================
Comment at: docs/ld.lld.1:188
 reported if there is an input object file not compatible with CET.
+.It Fl --force-bti
+Force enable AArch64 BTI in PLT, warn if Input ELF file does not have FEATURE_1_BTI property.
----------------
MaskRay wrote:
> `.Fl` is a mdoc macro that prepends a dash., so this should be`.It Fl -force-bti` (single dash). 
Thanks, for pointing that out, now fixed.

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

https://reviews.llvm.org/D62609