This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Remove non-address bits from read/write addresses in lldb
ClosedPublic

Authored by DavidSpickett on Feb 2 2022, 7:45 AM.

Details

Summary

Non-address bits are not part of the virtual address in a pointer.
So they must be removed before passing to interfaces like ptrace.

Some of them we get way with not removing, like AArch64's top byte.
However this is only because of a hardware feature that ignores them.

This change updates all the Process/Target Read/Write memory methods
to remove non-address bits before using addresses.

Doing it in this way keeps lldb-server simple and also fixes the
memory caching when differently tagged pointers for the same location
are read.

Removing the bits is done at the ReadMemory level not DoReadMemory
because particualrly for process, many subclasses override DoReadMemory.

Tests have been added for read/write at the command and API level,
for process and target. This includes variants like
Read<sometype>FromMemory. Commands are tested to make sure we remove
at the command and API level.

"memory find" is not included because:

  • There is no API for it.
  • It already has its own address handling tests.

Software breakpoints do use these methods but they are not tested
here because there are bigger issues to fix with those. This will
happen in another change.

Diff Detail

Event Timeline

DavidSpickett created this revision.Feb 2 2022, 7:45 AM
DavidSpickett requested review of this revision.Feb 2 2022, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 7:45 AM
DavidSpickett added a reviewer: omjavaid.EditedFeb 2 2022, 7:52 AM

Without this change any memory read/write of a pointer with a signature would fail. I found it trying to extend memory find and using a pointer with both a signature and a memory tag.

There's a couple of things I could do but not sure if worth it right now given that AArch64 Linux is the only user of non-address bits aside from Mac OS (which has debugserver so that may already be doing this). Those are:

  • Sink the application of top byte ignore into NativeRegisterContextLinux_AArch64
  • Raise the whole masking process up to NativeProcessProtocol

The latter would implement this logic for every NativeProcess at once but has the issues that:

  • most won't have a named register like linux has, so we end up adding an OS specific branch anyway
  • Many more functions at NativeProcessProtocol level take an address. The current way lets us fix the few ptrace calls all those member functions actually end up using.

Or in other words: this is a bit hacky but perhaps it should stay that way until it's more widely needed.

I also thought about fixing this from the lldb end but it seems logical to make the server act correctly even without having lldb clean the addresses up front. (though in a some cases, like memory tags, it will do but for its own reasons)

Also if anyone does know how or if debugserver handles this situation, that would be good to know.

labath added a subscriber: labath.Feb 3 2022, 1:00 AM

I haven't been following the pointer authentication work very much, but I am somewhat surprised that this part is done in lldb-server. I would have expected that lldb would strip these tags before the address makes it's way over here (I'm pretty sure it needs to do that for other uses anyway). Why is that not happening?

I would have expected that lldb would strip these tags before the address makes it's way over here (I'm pretty sure it needs to do that for other uses anyway). Why is that not happening?

So far we remove non-address bits for commands that need to diff pointers, memory read the obvious example. So memory write currently does not. memory region does but we don't send the address to lldb-server in that case, so it's not quite the same situation.

I'm not wedded to doing it in lldb-server, but it does have the advantage of being at a low level to catch all the possible accesses. At the expense of more complication server side. But you're right that in a lot of cases we'd need to do it in lldb for other reasons. I will try removing non-address bits purely in lldb and see how it compares.

labath added a comment.Feb 7 2022, 7:02 AM

Also if anyone does know how or if debugserver handles this situation, that would be good to know.

I don't know about debugserver, but it would definitely be interesting to check what gdb(server) does here. If it's doing this clientside, then sooner or later somebody will come along wanting to add gdbserver compatibility, and we'll end up doing it in both places.

One side-effect of doing it server-side is that this would interfere with our memory caching code, and we could end up re-reading the same piece of memory (with different tags) multiple times. May not make a big difference in practice, but I think it's a sign that this is done at the wrong level.

Doing this at the level of Target/Process::ReadMemory should still be fairly centralized, and it would not interfere with caching.

DavidSpickett planned changes to this revision.Feb 7 2022, 7:37 AM

check what gdb(server) does here

Will do.

One side-effect of doing it server-side is that this would interfere with our memory caching code

Yes I also wanted to fix the memory cache in lldb but you're right it needs the same logic applying and no point doing it twice if we don't need to. I'm working on changes to the lldb side instead now.

DavidSpickett updated this revision to Diff 417960.EditedMar 24 2022, 9:42 AM

Switch to removing non-address bits in lldb instead of lldb-server.

The breakpoint issues I mention only really happen if you try to break on a tagged
function pointer. Which is pretty niche, but I hope to address it later anyway.

On the issue of whether to use FixData vs FixCode there's 2 maybe 3 ways to go:

  • Assume that they're the same, which does work for Linux, for now.
  • Add a method that does both fixes, on the assumption that the virtual address size for code and data is the same so no harm done and all bits will be removed either way.
  • Extensively track whether addresses refer to code or data. In some situations this is possible (looking at the exec bits of a memory mapping for example) but I don't have a great idea what that looks like at this time.

Option 2 seems like a good way to go for now.

Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2022, 9:42 AM
DavidSpickett retitled this revision from [lldb][AArch64] Remove non-address bits from addresses passed to ptrace on Linux to [lldb] Remove non-address bits from read/write addresses in lldb.Mar 24 2022, 9:43 AM
DavidSpickett edited the summary of this revision. (Show Details)

Check that the "expression" command also treats pointers as equivalent.

Switch to removing non-address bits in lldb instead of lldb-server.

The breakpoint issues I mention only really happen if you try to break on a tagged
function pointer. Which is pretty niche, but I hope to address it later anyway.

On the issue of whether to use FixData vs FixCode there's 2 maybe 3 ways to go:

  • Assume that they're the same, which does work for Linux, for now.
  • Add a method that does both fixes, on the assumption that the virtual address size for code and data is the same so no harm done and all bits will be removed either way.
  • Extensively track whether addresses refer to code or data. In some situations this is possible (looking at the exec bits of a memory mapping for example) but I don't have a great idea what that looks like at this time.

Option 2 seems like a good way to go for now.

So on the topic of separate code/data address masks (Linux specific). I dont recall if the actual position of the mask in the address changes or not? It may be the case that we have separate code and address masks but their position in the address bits is fixed for both. Which will mean we actually dont need two separate functions. I tried fidning it out in Linux documentation but it only says "Separate masks are exposed for data pointers and instruction pointers". It doesnt specifically says if the location of the both can be different or not.
Do you have any explanation on this from AARM

Do you have any explanation on this from AARM

Yes I do.

linux arch/arm64/kernel/ptrace.c:
  /*
   * The PAC bits can differ across data and instruction pointers
   * depending on TCR_EL1.TBID*, which we may make use of in future, so
   * we expose separate masks.
   */
  unsigned long mask = ptrauth_user_pac_mask();
  struct user_pac_mask uregs = {
    .data_mask = mask,
    .insn_mask = mask,
  };

So currently we'll only ever see one value, in both masks. The control bit this refers to is:

D13.2.131 TCR_EL1, Translation Control Register (EL1)

TBID0, bit [51]

0b0 TCR_EL1.TBI0 applies to Instruction and Data accesses.
0b1 TCR_EL1.TBI0 applies to Data accesses only.

This is talked about earlier in the docs:

Supported PAC field and relation to the use of address tagging

When address tagging is used
The PAC field is Xn[54:bottom_PAC_bit].

When address tagging is not used
The PAC field is Xn[63:56, 54:bottom_PAC_bit].

The upshot of that is that you could have top byte ignore and PAC for data, but only PAC for instruction addresses.

PAC itself is all or nothing, at the hardware level it's on or off. If you wanted to not use it for one of code or data
your runtime simply chooses not to sign any pointers. Like arm64e appears to do for data
(https://developer.apple.com/documentation/security/preparing_your_app_to_work_with_pointer_authentication).

The current masks that lldb shows, which have top byte ignore included already:

(lldb) process status --verbose
<...>
Addressable code address mask: 0xff7f000000000000
Addressable data address mask: 0xff7f000000000000

So the end result is the same for us. What could happen is a future extension that isn't top byte ignore could use
those bits instead of PAC, making the PAC specific mask 0x007f...

Though I don't know how Linux would reconsile enabling TBI for userspace then doing that. Maybe the amount of top byte
use is small enough it could be changed (especially top byte of code addresses). But chances are slim it seems to me.

So back to my ideas in the previous comment.

Assume that they're the same, which does work for Linux, for now.

Would work fine for Linux for now and probably for a long time given that changing the TBI setting would be seen as an ABI issue.
And if someone decided to disable TBI completely and only use PAC, this still works because PAC extends into the top byte.

If they do decide to disable TBI for instructions then we're still fine given that the mask to extract the virtual address remains
the same. Yes the PAC mask has changed but the debugger is looking to remove *all* non-address bits.

E.g. If we disable TBI for instruction accesses the mask is 0xff7f000000000000 because PAC claims the top byte.
Then the mask for data accesses is 0x007f000000000000 but we add TBI to get 0xff7f000000000000. Same result in the end.

So we could just pick one of the methods and standardise on that for sitautions where you don't know for sure it'll be a code address.
This will have to be FixDataAddress due to Arm Thumb's mode bit 0. We don't want to be aligning all reads to 2 bytes.
(FWIW this matches what I've done so far, though that was unintentional)

Perhaps we add a third method to make that clear (name subject to change) FixAnyAddress. Then the Arm code can forward that to fixdata and AArch64
can pick either data or code. For situations where you're sure you can pick code or data e.g. code breakpoint on an address.

Add a method that does both fixes, on the assumption that the virtual address size for code and data is the same so no harm done and all bits will be removed either way.

The Arm Thumb problem means this is not going to work. (not that those targets are likely to care about non-address bits but these Fix calls are made from generic code
so it does still matter)

Extensively track whether addresses refer to code or data

Isn't realistic a lot of the time. Though there are some clear situations where FixCode or FixData makes more sense so we can do some of this, just not an lldb wide tracking
framework sort of thing.

So my suggestion for a solution would be to add a FixAnyAddress alongside FixCode and FixData, and use that whenever it could be either. Tricky things like Arm Thumb can
then choose what the most "safe" fix is.

Tell me if that logic makes sense.

Which will mean we actually dont need two separate functions.

At the ABI plugin level we do simply due to Arm Thumb existing. Lower down yeah you could get away with reading just one of the PAC masks but it's not much of a saving.

Thanks for good detailed explanation. I think from the code readability point of view, we may use FixAddress function which i believe already exists in ABI and if not then introduce FixAnyAddress may be. We can put all the comments about PAC/TBI code vs data address bits there in AArch64 ABI code instead of putting a comment about code/data address everytime we use FixDataAddress in generic code.

DavidSpickett planned changes to this revision.Apr 8 2022, 5:38 AM

Cool. I will apply this to existing code first, in another change.

Use FixAnyAddress instead of FixDataAddress.

omjavaid accepted this revision.May 17 2022, 8:46 PM
This revision is now accepted and ready to land.May 17 2022, 8:46 PM

Correct "addr" -> "fixed_addr" in a couple of comments.