Page MenuHomePhabricator

[AArch64][PAC] Lower auth/resign into checked sequence.
Needs ReviewPublic

Authored by ab on Aug 22 2022, 9:02 AM.

Details

Summary

A lot of this is boilerplate isel; the trapping/checking lowering is the interesting bit. As a platform decision, we emit the trapping sequence by default, but this is obviously something that can be changed for other platforms (as well as based on later hardware features, most interestingly FPAC).

This also uses x16/x17 as "safe registers" purely because of another platform decision: on Darwin we have additional hardening in the kernel for these registers. We use those because 1) they're already used for interesting control flow in linker stubs, and 2) these auth/resign sequences don't have calls, so they're not clobbered by said linker stubs.

The expansion is done as late as possible (in AsmPrinter), to avoid any risks of late backend passes breaking the sequence.

Diff Detail

Event Timeline

ab created this revision.Aug 22 2022, 9:02 AM
Herald added a project: Restricted Project. · View Herald Transcript
ab requested review of this revision.Aug 22 2022, 9:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 9:02 AM
apazos added inline comments.Aug 23 2022, 12:53 PM
llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
1227

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?

peter.smith added inline comments.
llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
1227

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

kristof.beyls added inline comments.Aug 24 2022, 5:43 AM
llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
1227

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."?

kristof.beyls added inline comments.Aug 24 2022, 8:19 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
1408–1411

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....

peter.smith added inline comments.Aug 25 2022, 2:31 AM
llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
1227

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.

ab added inline comments.Aug 30 2022, 2:06 PM
llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
1227

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)

llvm/lib/Target/AArch64/AArch64InstrInfo.td
1408–1411

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 ;)

I finally found time to go through the patch from beginning to end. I only have 2 more - hopefully minor questions.
Apart from that, LGTM.

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
1278

I wonder if this function attribute is already documented as part of another patch?
If not, maybe this patch should contain some LLVM-IR LangRef documentation for it?

1371–1376

I would, maybe naively, assume that returning the stripped pointer is worse than returning the failed-authenticated pointer. The stripped pointer is a valid raw pointer; the failed-authenticated pointer is an invalid raw pointer, i.e. should trigger a memory access fault when dereferenced.
Therefore, why is it better to return the stripped pointer?