[PATCH] D132386: [AArch64][PAC] Lower auth/resign into checked sequence.

Tue Aug 30 14:06:37 PDT 2022

ab added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:1227
+
+  // We can expand AUT/AUTPAC into 3 possible sequences:
+  // - unchecked:
----------------
peter.smith wrote:
> kristof.beyls wrote:
> > peter.smith wrote:
> > > apazos wrote:
> > > > Should the PAuth ABI for ELF documentation in 
> > > > https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst 
> > > > have a note on x16/x17 being used for lowering auth/resign sequences?
> > > Not had a chance to reason through all of this in detail. My instinct is that this is already covered in the PCS (see below) if there are additional restrictions that are needed in the PAuthABI then it will be worth raising a github issue (or a pull request).
> > > 
> > > The x16/x17 registers are mentioned in the procedure call standard as corruptible by intra-procedure calls so compilers already have to assume that they may be corrupted on a function call https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#611general-purpose-registers 
> > My understanding of the reason to use X16 and X17 here is that there is a platform guarantee that anything that interrupts the execution of the program in an arbitrary location, such as a process context switch, will make sure that when it saves/restores the register context, the registers X16 and X17 will not be written to memory as is.
> > My understanding is that the goal is for only registers X16 or X17 to potentially contain unsigned/raw pointers because of the resigning sequences implemented here.
> > If these registers would be written to memory on a context switch as is, that leaves an attack vector for an attacker that can overwrite memory - they could simply overwrite the raw pointer.
> > 
> > I think it's worthwhile to consider if we should add anything related to this to the Arm PAuth ABI. Maybe a good first step would be to raise a ticket at https://github.com/ARM-software/abi-aa/issues describing what the guarantee or guideline is that we'd want there?
> > 
> > From the compiler perspective, I'm expecting the guideline might be something like "whenever the compiler for some reason has to convert a signed pointer into a raw (unsigned) pointer, where that raw pointer is not requested to be stored in memory according to the source code, only registers X16 or X17 should be used to keep that raw pointer."?
> I've raised https://github.com/ARM-software/abi-aa/issues/168 so we don't forget about it. Not sure the ELF extensions are a good place to put it, but it is the closest we have right now.
Yeah, to be clear: it's okay to use other registers here for non-Darwin, but I don't think it would ever hurt to use these particular ones.  For ELF it seems fine to me to treat this as an implementation detail of clang, and not as an ABI requirement?

But for further hardening, the fact that x16/x17 are already described as the "intra-procedure-call scratch registers" makes these registers special already, as they presumably can hold unsigned/raw pointers in PLT code (they do in MachO linker stubs).
So, provided:
- lr, pc, sp, nzcv hold values that can be used to hijack control flow most of the time
- x16/x17 can hold unsigned code pointers some of the time
- other registers, if used to hold code pointers, only ever hold signed code pointers
under a threat model where the kernel is writeable, as long as the architectural keys (`APIAKey` and all) are not trivially accessible, the saved register values in the kernel become an interesting attack surface.

We would have protected x16/x17 anyway because of the stubs, so we might as well piggy-back on that for ptrauth sequences.  (We go on to use these registers in other interesting sequences, e.g., materializing signed pointers with adrp+add+pac, or non-ptrauth-but-still-hardened jump table dispatch.)

Of course, one could argue it'd be ideal to protect all saved registers at all times, but it's safe to assume protecting these 6 is faster than all 32+, and that's important when it's done at every context switch.

FWIW, somewhat tangential: we further exploit this register assignment to help with debugging: we use the special `brk` encoding below, and we taught various tools to look at x16/x17 when trapping there, as they're known to contain the resigned/authed value of interest. E.g., in lldb: https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp#L110
I'm not sure anything defines `brk` assignments anywhere, but I assume non-Darwin users would jump straight to FPAC, and wouldn't need these horrible sequences anyway?  (which reminds me: this is missing FPAC support, I'll pull that in)

================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:1408-1411
+  // AUT and re-PAC a value, using different keys/data.
+  // This directly manipulates x16/x17, which are the only registers the OS
+  // guarantees are safe to use for sensitive operations.
+  def AUTPAC
----------------
kristof.beyls wrote:
> I find the "AUTPAC" name not very descriptive of what it does.
> I would think one of the following names might be more descriptive:
> - PAC_RESIGN
> - PAC_AUTH_SIGN
> 
> (I think that it may be useful to have a common prefix for all pseudo-ops related to pointer authentication, and chose "PAC_" here. I'd be happy with a different prefix too).
> 
> If we ended up agreeing on using "PAC_" as the common prefix, that would mean the "AUT" pseudo would be renamed to "PAC_AUT". I personally would prefer "PAC_AUTH" with an H but realize I'm well into bike shedding land with that....
> 
Heh, yeah that seems reasonable, but the existence of the non-pseudo ISA `AUT` instructions is what made me go with the admittedly funky `AUT`/`AUTPAC`: if we already have `AUTIA`, and we already have `PACIA`, I guess `AUT` and `AUTPAC` pseudos just match that.

I did the same elsewhere, notably for an `LDRAA`/`LDRAB` pseudo, that I called `LDRA`.  In a way the PAC relationship is even more obscure there, but it is what it is ;)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132386