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

[DavidSpickett]

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.

[DavidSpickett]

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)

[DavidSpickett]

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

[labath]

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?

[DavidSpickett]

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]

In D118794#3290552, @DavidSpickett wrote:

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]

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]

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.

[DavidSpickett]

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

[omjavaid]

In D118794#3405724, @DavidSpickett wrote:

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

[DavidSpickett]

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.

[omjavaid]

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]

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

[DavidSpickett]

Use FixAnyAddress instead of FixDataAddress.

[DavidSpickett]

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

lldb/source/Plugins/Process/Linux/NativeProcessLinux.h

 //===-- NativeProcessLinux.h ---------------------------------- -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
private:
   Status PopulateMemoryRegionCache();
 
   /// Manages Intel PT process and thread traces.
   IntelPTManager m_intel_pt_manager;
 
   // Handle a clone()-like event.
   bool MonitorClone(NativeThreadLinux &parent, lldb::pid_t child_pid,
                     int event);
+
+  // Remove any bits that are not virtual address bits. For example on AArch64
+  // this includes the top byte, memory tags and pointer signature.
+  lldb::addr_t RemoveNonAddressBits(lldb::addr_t addr);
 };
 
 } // namespace process_linux
 } // namespace lldb_private
 
 #endif // #ifndef liblldb_NativeProcessLinux_H_

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

 //===-- NativeProcessLinux.cpp --------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
llvm::Expected<uint64_t> Result =
       Syscall({mmap_data->SysMunmap, addr, it->second});
   if (!Result)
     return Result.takeError();
 
   m_allocated_memory.erase(it);
   return llvm::Error::success();
 }
 
+lldb::addr_t NativeProcessLinux::RemoveNonAddressBits(lldb::addr_t addr) {
+  NativeRegisterContextLinux &context =
+      GetCurrentThread()->GetRegisterContext();
+  const RegisterInfo *reg_info = context.GetRegisterInfoByName("data_mask");
+  const addr_t original = addr;
+  addr_t mask = 0;
+
+  if (reg_info) {
+    // If this fails, inverse of 0 keeps all the bits intact.
+    mask = context.ReadRegisterAsUnsigned(reg_info, 0);
+  }
+
+  const lldb_private::ArchSpec &arch = GetArchitecture();
+  // AArch64 Linux always has top byte ignore enabled for userspace.
+  if (arch.IsValid() && arch.GetTriple().isAArch64())
+    addr |= (addr_t)0xFF << 56;
+
+  addr &= ~mask;
+
+  Log *log = GetLog(POSIXLog::Memory);
+  LLDB_LOG(log,
+           "Removing non address bits from address {0:x} using mask {1:x}, new "
+           "address is {2:x}",
+           original, mask, addr);
+
+  return addr;
+}
+
 Status NativeProcessLinux::ReadMemoryTags(int32_t type, lldb::addr_t addr,
                                           size_t len,
                                           std::vector<uint8_t> &tags) {
+  Log *log = GetLog(POSIXLog::Memory);
+  LLDB_LOG(log, "type: {0} addr: {1:x} len: {2}", type, addr, len);
+
+  addr = RemoveNonAddressBits(addr);
+
   llvm::Expected<NativeRegisterContextLinux::MemoryTaggingDetails> details =
       GetCurrentThread()->GetRegisterContext().GetMemoryTaggingDetails(type);
   if (!details)
     return Status(details.takeError());
 
   // Ignore 0 length read
   if (!len)
     return Status();
Status NativeProcessLinux::ReadMemoryTags(int32_t type, lldb::addr_t addr,
   }
 
   return Status();
 }
 
 Status NativeProcessLinux::WriteMemoryTags(int32_t type, lldb::addr_t addr,
                                            size_t len,
                                            const std::vector<uint8_t> &tags) {
+  Log *log = GetLog(POSIXLog::Memory);
+  LLDB_LOG(log, "type: {0} addr: {1:x} len: {2}", type, addr, len);
+
+  addr = RemoveNonAddressBits(addr);
+
   llvm::Expected<NativeRegisterContextLinux::MemoryTaggingDetails> details =
       GetCurrentThread()->GetRegisterContext().GetMemoryTaggingDetails(type);
   if (!details)
     return Status(details.takeError());
 
   // Ignore 0 length write
   if (!len)
     return Status();
case llvm::Triple::arm:
     }
   default:
     return NativeProcessProtocol::GetSoftwareBreakpointTrapOpcode(size_hint);
   }
 }
 
 Status NativeProcessLinux::ReadMemory(lldb::addr_t addr, void *buf, size_t size,
                                       size_t &bytes_read) {
+  Log *log = GetLog(POSIXLog::Memory);
+  LLDB_LOG(log, "addr = {0:x}, buf = {1:x}, size = {2}", addr, buf, size);
+
+  addr = RemoveNonAddressBits(addr);
+
   if (ProcessVmReadvSupported()) {
     // The process_vm_readv path is about 50 times faster than ptrace api. We
     // want to use this syscall if it is supported.
 
     const ::pid_t pid = GetID();
 
     struct iovec local_iov, remote_iov;
     local_iov.iov_base = buf;
if (ProcessVmReadvSupported()) {
     // else the call failed for some reason, let's retry the read using ptrace
     // api.
   }
 
   unsigned char *dst = static_cast<unsigned char *>(buf);
   size_t remainder;
   long data;
 
-  Log *log = GetLog(POSIXLog::Memory);
-  LLDB_LOG(log, "addr = {0}, buf = {1}, size = {2}", addr, buf, size);
-
   for (bytes_read = 0; bytes_read < size; bytes_read += remainder) {
     Status error = NativeProcessLinux::PtraceWrapper(
         PTRACE_PEEKDATA, GetID(), (void *)addr, nullptr, 0, &data);
     if (error.Fail())
       return error;
 
     remainder = size - bytes_read;
     remainder = remainder > k_ptrace_word_size ? k_ptrace_word_size : remainder;

 
 Status NativeProcessLinux::WriteMemory(lldb::addr_t addr, const void *buf,
                                        size_t size, size_t &bytes_written) {
   const unsigned char *src = static_cast<const unsigned char *>(buf);
   size_t remainder;
   Status error;
 
   Log *log = GetLog(POSIXLog::Memory);
-  LLDB_LOG(log, "addr = {0}, buf = {1}, size = {2}", addr, buf, size);
+  LLDB_LOG(log, "addr = {0:x}, buf = {1:x}, size = {2}", addr, buf, size);
+
+  addr = RemoveNonAddressBits(addr);
 
   for (bytes_written = 0; bytes_written < size; bytes_written += remainder) {
     remainder = size - bytes_written;
     remainder = remainder > k_ptrace_word_size ? k_ptrace_word_size : remainder;
 
     if (remainder == k_ptrace_word_size) {
       unsigned long data = 0;
       memcpy(&data, src, k_ptrace_word_size);

lldb/test/API/tools/lldb-server/memory-non-address-bits/Makefile

+C_SOURCES := main.c
+CFLAGS_EXTRAS := -march=armv8.5-a+memtag
+
+include Makefile.rules

lldb/test/API/tools/lldb-server/memory-non-address-bits/TestGdbRemoteMemoryNonAddressBits.py

+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+"""
+Check that lldb-server correctly removes the non-address bits
+required to make ptrace calls on Linux.
+"""
+
+class TestGdbRemoteMemoryNonAddressBits(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def prep_test(self):
+        self.build()
+        self.set_inferior_startup_launch()
+        procs = self.prep_debug_monitor_and_inferior()
+
+        # We don't use isAArch64MTE here because we cannot do runCmd in an
+        # lldb-server test. Instead we run the example and skip if it fails
+        # to allocate an MTE buffer.
+        # Pointer signing is done with a hint space instruction so the test
+        # can run on pre armv8.3-a CPUs. (though it will not be doing any useful
+        # checking in that case)
+
+        # Run the process
+        self.test_sequence.add_log_lines(
+            [
+                # Start running after initial stop
+                "read packet: $c#63",
+                # Match the plain and signed pointers
+                {"type": "output_match",
+                 "regex": self.maybe_strict_output_regex(r"buf: (.+) buf_with_non_address: (.+)\r\n"),
+                 "capture": {1: "buf", 2: "buf_with_non_address"}},
+                # Now stop the inferior
+                "read packet: {}".format(chr(3)),
+                # And wait for the stop notification
+                {"direction": "send", "regex": r"^\$T[0-9a-fA-F]{2}thread:[0-9a-fA-F]+;"}],
+            True)
+
+        # Run the packet stream
+        context = self.expect_gdbremote_sequence()
+        self.assertIsNotNone(context)
+
+        buf = context.get("buf")
+        self.assertIsNotNone(buf)
+        buf_with_non_address = context.get("buf_with_non_address")
+        self.assertIsNotNone(buf_with_non_address)
+
+        # nil means we couldn't set up a tagged page because the
+        # target doesn't support it.
+        if buf == "(nil)":
+            self.skipTest("Target must support MTE.")
+
+        buf = int(buf, 16)
+        buf_with_non_address = int(buf_with_non_address, 16)
+
+        return buf, buf_with_non_address
+
+    def check_response(self, packet, expected):
+        self.test_sequence.add_log_lines(["read packet: ${}#00".format(packet),
+                                          "send packet: ${}#01".format(expected),
+                                          ],
+                                         True)
+        self.expect_gdbremote_sequence()
+
+    @skipUnlessArch("aarch64")
+    @skipUnlessPlatform(["linux"])
+    @skipUnlessAArch64MTELinuxCompiler
+    def test_non_address_bit_read_write(self):
+        """ Test that lldb-server removes non-address bits as needed for ptrace
+            operations where a virtual address is used."""
+        buf, buf_with_non_address = self.prep_test()
+
+        self.check_response("x{:x},1".format(buf), "\x00")
+        self.check_response("x{:x},1".format(buf_with_non_address), "\x00")
+
+        self.check_response("M{:x},1:01".format(buf), "OK")
+        self.check_response("M{:x},1:01".format(buf_with_non_address), "OK")
+
+        self.check_response("qMemTags:{:x},1:1".format(buf), "m00")
+        self.check_response("qMemTags:{:x},1:1".format(buf_with_non_address), "m00")
+
+        self.check_response("QMemTags:{:x},1:1:00".format(buf), "OK")
+        self.check_response("QMemTags:{:x},1:1:00".format(buf_with_non_address), "OK")

lldb/test/API/tools/lldb-server/memory-non-address-bits/main.c

+#include <arm_acle.h>
+#include <asm/hwcap.h>
+#include <linux/mman.h>
+#include <stdio.h>
+#include <sys/auxv.h>
+#include <sys/mman.h>
+#include <sys/prctl.h>
+#include <unistd.h>
+
+int print_pointers(char *buf, char *buf_with_non_address) {
+  // This output is picked up by the test.
+  printf("buf: %p buf_with_non_address: %p\n", buf, buf_with_non_address);
+
+  // Exit after some time, so we don't leave a zombie process
+  // if the test framework lost track of us.
+  sleep(60);
+  return 0;
+}
+
+int main(int argc, char const *argv[]) {
+  // We need to allocate a memory tagged page to know whether
+  // our memory tag read/write packets succeeded.
+  if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE | PR_MTE_TCF_SYNC, 0,
+            0, 0)) {
+    return print_pointers(NULL, NULL);
+  }
+
+  size_t page_size = sysconf(_SC_PAGESIZE);
+  char *buf = mmap(0, page_size, PROT_READ | PROT_WRITE | PROT_MTE,
+                   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (buf == MAP_FAILED)
+    return print_pointers(NULL, NULL);
+
+  // Set top byte including where memory tag bits would go.
+  char *buf_with_non_address = (char *)((size_t)buf | (size_t)0xff << 56);
+  // Sign it using a hint space instruction in case the core doesn't have
+  // pointer authentication.
+  __asm__ __volatile__("pacdza %0"
+                       : "=r"(buf_with_non_address)
+                       : "r"(buf_with_non_address));
+  // Address is now:
+  // <4 bit user tag><4 bit memory tag><pointer signature><virtual address>
+
+  return print_pointers(buf, buf_with_non_address);
+}