Page MenuHomePhabricator

[LLDB] AArch64 Linux and elf-core PAC stack unwinder support
ClosedPublic

Authored by omjavaid on Apr 6 2021, 4:32 AM.

Details

Summary

This patch builds on D100521 and other related patches to add support
for unwinding stack on AArch64 systems with pointer authentication
feature enabled.

We override FixCodeAddress and FixDataAddress function in ABISysV_arm64
class. We now try to calculate and set code and data masks after reading
data_mask and code_mask registers exposed by AArch64 targets running Linux.

This patch utilizes core file linux-aarch64-pac.core for testing that
LLDB can successfully unwind stack frames in the presence of signed
return address after masking off ignored bits.

This patch also includes a AArch64 Linux native test case to demonstrate
successful back trace calculation in presence of pointer authentication
feature.

Diff Detail

Event Timeline

omjavaid created this revision.Apr 6 2021, 4:32 AM
omjavaid requested review of this revision.Apr 6 2021, 4:32 AM
labath added a comment.Apr 8 2021, 6:35 AM

Setting the address size from the register context is a bit awkward. We have one register context per thread, and they all will be competing to set the value (it should always be the same value under normal circumstances, but it still makes for a strange relationship). It would be better if all of this could be contained in the ABI class. The ABI classes have intimate knowledge of the registers anyway, so it would not be unusual for it to access a specific register to get the mask (here I am assuming that the mask is actually stored in a register).

Then we'd have no need for Process::GetAddressBitsInUse and worry whether it has been initialized, as the abi class could fetch the value when it gets constructed, or something...

Do you envision using this in some other way than through the ABI class?

lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
41–42

I think this should be an assert (here, or even somewhere earlier).

43–45

return llvm::SignExtend64(pc, address_shift_width)

Hi Omair, sorry for the delay in looking at this. It seems like we have two overlapping patches here, @justincohen patch in https://reviews.llvm.org/D98886 and this one.

I'm not convinced that we'll see a target where we have noncontiguous bits used for addressing (the best reason for using a mask), but Justin points out that Linux ARM's NT_ARM_PAC_MASK (gettable by ptrace PTRACE_GETREGSET) is expressed as a mask (and googling around, it looks like you can have a different mask for code pointers and data pointers, so Process would really need to store both, but it may be that for crashpad only the code mask needs to be tracked in Process).

lldb needs to allow for users to set the number of bits used for addressing in terms of a number -- 52 in your example -- but given that Linux wants to give us a mask via the ptrace call, I think we should preserve and use that mask. Darwin's API also provide the value in terms of the number of bits used for addressing, and as a user setting I think that's the most natural way of handling it and we should allow it. But internal to Process, we should convert this to a CodeAddress mask and down in the ABI's where we support AArch v8.3 ptrauth, we use bit 55 to clear/set the non-addressing high bits.

Omair, Justin, what do you think here? I don't think it's especially hard to accept this in terms of # of bits OR a mask, and we should use the more general internal rep in lldb. Another alternative would be "the mask should be converted to the # of bits in addressing and stored in Process in those terms".

(Honestly the idea of an address mask makes me nervous all over - if the rule is "take bit 55 and set all non-addressing bits to 0 or 1 depending on that", then we're implicitly saying "the mask only masks off high bits". If it ever masked off bit 0, for instance, on a code address, then we're going to set that to 0 or 1 when we set/clear non-addressing bits? Obviously not. So it's not REALLY a mask, is it, it's just the # of bits used for addressing in another more flexible format that you can't actually flex or it all breaks. :)

Omair, Justin, what do you think here? I don't think it's especially hard to accept this in terms of # of bits OR a mask, and we should use the more general internal rep in lldb. Another alternative would be "the mask should be converted to the # of bits in addressing and stored in Process in those terms".

From a minidump/crashpad perspective, we are fine with either approach. We plan to a use a mask within the minidump format, but we can convert it to # of bits as necessary.

I think this will come down to, who wants to implement a patch that can set/get either form through Process API. The mask might be a more general format, so if Justin is willing to update his patch to that we can add Omair's test case to his patch?

Hi Omair, sorry for the delay in looking at this. It seems like we have two overlapping patches here, @justincohen patch in https://reviews.llvm.org/D98886 and this one.

I'm not convinced that we'll see a target where we have noncontiguous bits used for addressing (the best reason for using a mask), but Justin points out that Linux ARM's NT_ARM_PAC_MASK (gettable by ptrace PTRACE_GETREGSET) is expressed as a mask (and googling around, it looks like you can have a different mask for code pointers and data pointers, so Process would really need to store both, but it may be that for crashpad only the code mask needs to be tracked in Process).

lldb needs to allow for users to set the number of bits used for addressing in terms of a number -- 52 in your example -- but given that Linux wants to give us a mask via the ptrace call, I think we should preserve and use that mask. Darwin's API also provide the value in terms of the number of bits used for addressing, and as a user setting I think that's the most natural way of handling it and we should allow it. But internal to Process, we should convert this to a CodeAddress mask and down in the ABI's where we support AArch v8.3 ptrauth, we use bit 55 to clear/set the non-addressing high bits.

Omair, Justin, what do you think here? I don't think it's especially hard to accept this in terms of # of bits OR a mask, and we should use the more general internal rep in lldb. Another alternative would be "the mask should be converted to the # of bits in addressing and stored in Process in those terms".

(Honestly the idea of an address mask makes me nervous all over - if the rule is "take bit 55 and set all non-addressing bits to 0 or 1 depending on that", then we're implicitly saying "the mask only masks off high bits". If it ever masked off bit 0, for instance, on a code address, then we're going to set that to 0 or 1 when we set/clear non-addressing bits? Obviously not. So it's not REALLY a mask, is it, it's just the # of bits used for addressing in another more flexible format that you can't actually flex or it all breaks. :)

Frankly I am also not too sure about keeping single mask for both code and data masks. For now Linux kernel ensures both are the same but they can differ in future. For user-space Linux apps top-byte is used for tags and bit 55 only gives us a hint on whether we mask the top byte only or also need to mask off bits (54:VA_Bits). For unwinder code mask is relevant but we would also like to use the mask for clearing top-bits off watchpoint addresses in-case of a tagged data pointer and if we have a separate data_mask we ll essentially need to store that information separately.

On linux masks are readable through code_mask and data_mask registers. We dont really need to keep this information in process as @labath pointed out but that is only fine for user-space apps. In case we intend to debug linux kernel or any bare-metal app utilizing the top byte(s) which i am not aware of any user of LLDB right now using it to debug the Linux kernel but I dont wanna introduce anything here which makes LLDB user-app specific so thats why one of the option is to send the information over gdb-remote protocol qHostInfo but that also restricts LLDB to lldb-server only.

In any case we ll need atleast one mask for sure and I dont mind @justincohen writing that patch and I ll rebase this patch on top of it once its merged.

JDevlieghere added a subscriber: JDevlieghere.

Coincidentally, I was just in need of something similar. Jason pointed me to this discussion so I took the liberty of implementing his suggestion in D100515

omjavaid updated this revision to Diff 340686.Apr 26 2021, 4:48 PM
omjavaid edited the summary of this revision. (Show Details)

Moved mask calculation to ABISysV_arm64 class.

omjavaid retitled this revision from [LLDB] AArch64 PAC elf-core stack unwinder support to [LLDB] AArch64 Linux and elf-core PAC stack unwinder support.May 3 2021, 4:30 AM
omjavaid edited the summary of this revision. (Show Details)
omjavaid updated this revision to Diff 342357.May 3 2021, 4:33 AM

This merges separate patch D99947 for Linux into this one. Linux specific changes are no longer required as we have moved mask calculation into ABISysV_arm64 class.

The code LGTM but others should decide if this fits with their efforts.

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/Makefile
4

Nit: armv8.3-a would also be fine here

DavidSpickett requested changes to this revision.May 19 2021, 3:51 AM

Realised that you've got an empty program file and no core file. I compiled main.c with:

$ aarch64-unknown-linux-gnu-gcc -g -march=armv8.3-a -mbranch-protection=pac-ret+leaf main.c -o linux-aarch64-pac.out -nostdlib -static

Then generated a core dump in qemu and after fixing up the PID it works as expected.

This revision now requires changes to proceed.May 19 2021, 3:51 AM
omjavaid updated this revision to Diff 347847.May 25 2021, 10:13 PM

This add skipped linux-aarch64-pac.out file.

omjavaid updated this revision to Diff 347848.May 25 2021, 10:36 PM

Uploading linux-aarch64-pac.out using arc

differential was not uploading binary file properly using arc to do the same.

Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2021, 10:36 PM

I realised my mistake, I thought this was adding a new core file but in fact it's using the one you added for the register tests. So now the outfile is there the test passes.

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py
42

Strange indent here

lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c
6

I'd rather use armv8.3-a here just for correctness.

DavidSpickett resigned from this revision.May 26 2021, 3:17 AM

Resigning to remove my requested changes, if that works. Looks good from my point of view.

@labath You had earlier questions about using the register context, and a few comments that have gotten disconnected from the original diff somehow. Those concerns still relevant?

This revision was not accepted when it landed; it landed in state Needs Review.Jun 15 2021, 2:10 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.