[lldb][AArch64] Fix corefile memory reads when there are non-address bits

[DavidSpickett]

Summary

Previously if you read a code/data mask before there was a valid thread
you would get the top byte mask. This meant the value was "valid" as in,
don't read it again.

When using a corefile we ask for the data mask very early on and this
meant that later once you did have a thread it wouldn't read the
register to get the rest of the mask.

This fixes that and adds a corefile test generated from the same program
as in my previous change on this theme.

Depends on D118794

[DavidSpickett]

This smells a little bit like a hack but in some ways it makes sense that an address mask value couldn't possibly be valid until we have a thread setup.

[labath]

Can we make the core file smaller? Since it's essentially static data, do you really need all the mmap, debug info and everything? Could you just take an address that is known to be in the core file (smallest you can make), and then do a memory read 0xwhatever ?

lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py

191

even for a core file?

[DavidSpickett]

• Don't require pointer auth hardware for the corefile test.
• Remove the program file since all we need to do is read memory from the corefile.

[DavidSpickett]

Can we make the core file smaller?

Yes we can.

I need to find a good way to do that, my first instinct was to strip segments from it but perhaps I'm barking up the wrong tree there. Is there a usual way to make a minimal core?

[DavidSpickett]

• Corefile down to 24k with some tweaking of coredump_filter
• Add some prints so when you generate the corefile you know what addresses to test

I also found another bug testing this where memory read works but printing
doesnt, like:

(lldb) p (char*)0x0000ffff90027000
(char *) $1 = 0x0000ffff90027000 "LLDB"
(lldb) p (char*)0xff0afffff7ff9000
(char *) $2 = 0xff0afffff7ff9000 ""

I think this may use the number of addressable bits setting (as opposed to the masks), that could be missing
in a corefile. Planning changes to work on that and add tests for it.

[labath]

In D122411#3408204, @DavidSpickett wrote:

• Corefile down to 24k with some tweaking of coredump_filter

24k is pretty good. At this point, if you wanted to go down further, you'd probably have to ditch libc, which may not be as hard if you don't need mmap (the comment indicates that is because of lldb reading globals eagerly, but it can't read globals without having access to the exe file).

[DavidSpickett]

Fix "expression"/"p" result by removing non-address bits in processElfCore::ReadMemory.
I missed that it overrides this. (it was nothing to do with the target addressable bits setting)

Add tests for that and process/target API use with the corefile.

I did try to avoid the mmap call to reduce the corefile further but couldn't find
any corefile filter that would still include the buffer without being > 300k.

[omjavaid]

Very corner case but Is there a possibility of masks changing positions during the life-time of an inferior ? So this makes me think that code/data masks are per process and cant be changes once set. Should we worry about it?AFAIK this is fine but just asking to clear any ambiguities.

[DavidSpickett]

The current assumption is that once you've got a valid (currently anything non-zero) mask then that's it, it's not going to change. I think a future extension could change the meaning of the existing non-address bits at runtime, but that wouldn't be a problem unless you're something like the tagging commands. 99% of the time we want to remove all non-address bits no matter what they mean.

Changing the virtual address size is the real problem if it ever happens but given that the stack is assumed to be at the top of memory, it would be difficult. Shrinking means cutting off/moving the stack down (with some kind of position independent stack?) and growing it means you can't make top of stack == end of memory assumptions. You could do it with some kind of fork or restart but I think lldb would just handle that fine like it normally does.

Side note: the "0 means not read yet" pattern seems like it could lead to some waste with a target that genuinely has no mask but that's not relevant to this change.

[DavidSpickett]

There's some argument that the thing asking for the mask shouldn't ask so early but I think the logic here is still an improvement regardless. If we don't have a thread yet we can't have a mask.

[DavidSpickett]

If we don't have a thread yet we can't have a mask.

Though this isn't a watertight argument since I think Mach corefiles can have a note that tells you the number of addressable bits. In our case there's nothing like that.

[omjavaid]

LGTM. With a minor nit fix about FixAnyAddress vs FixDataAddress if required.

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp

288

Is this a candidate for FixAnyAddress ?

[DavidSpickett]

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp

288

Absolutely. I intend to get FixAny in first, then update this and the stacked patch to use it.

[DavidSpickett]

Use FixAnyAddress.

lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp

 lldb::addr_t ABISysV_arm64::FixAddress(addr_t pc, addr_t mask) {
   lldb::addr_t pac_sign_extension = 0x0080000000000000ULL;
   return (pc & pac_sign_extension) ? pc | mask : pc & (~mask);
 }
 
 // Reads code or data address mask for the current Linux process.
 static lldb::addr_t ReadLinuxProcessAddressMask(lldb::ProcessSP process_sp,
                                                 llvm::StringRef reg_name) {
+  // 0 means there isn't a mask or it has not been read yet.
+  // We do not return the top byte mask unless thread_sp is valid.
+  // This prevents calls to this function before the thread is setup locking
+  // in the value to just the top byte mask, in cases where pointer
+  // authentication might also be active.
+  uint64_t address_mask = 0;
+  lldb::ThreadSP thread_sp = process_sp->GetThreadList().GetSelectedThread();
+  if (thread_sp) {
     // Linux configures user-space virtual addresses with top byte ignored.
     // We set default value of mask such that top byte is masked out.
-  uint64_t address_mask = ~((1ULL << 56) - 1);
+    address_mask = ~((1ULL << 56) - 1);
     // If Pointer Authentication feature is enabled then Linux exposes
     // PAC data and code mask register. Try reading relevant register
     // below and merge it with default address mask calculated above.
-  lldb::ThreadSP thread_sp = process_sp->GetThreadList().GetSelectedThread();
-  if (thread_sp) {
     lldb::RegisterContextSP reg_ctx_sp = thread_sp->GetRegisterContext();
     if (reg_ctx_sp) {
       const RegisterInfo *reg_info =
           reg_ctx_sp->GetRegisterInfoByName(reg_name, 0);
       if (reg_info) {
         lldb::addr_t mask_reg_val = reg_ctx_sp->ReadRegisterAsUnsigned(
             reg_info->kinds[eRegisterKindLLDB], LLDB_INVALID_ADDRESS);
         if (mask_reg_val != LLDB_INVALID_ADDRESS)

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp

 
 #include <memory>
 #include <mutex>
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
+#include "lldb/Target/ABI.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"

 
 // Process Queries
 
 bool ProcessElfCore::IsAlive() { return true; }
 
 // Process Memory
 size_t ProcessElfCore::ReadMemory(lldb::addr_t addr, void *buf, size_t size,
                                   Status &error) {
+  if (lldb::ABISP abi_sp = GetABI())
+    addr = abi_sp->FixAnyAddress(addr);
+
   // Don't allow the caching that lldb_private::Process::ReadMemory does since
   // in core files we have it all cached our our core file anyway.
   return DoReadMemory(addr, buf, size, error);
 }
 
 Status ProcessElfCore::DoGetMemoryRegionInfo(lldb::addr_t load_addr,
                                              MemoryRegionInfo &region_info) {
   region_info.Clear();
   const VMRangeToPermissions::Entry *permission_entry =

lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py

def test_non_address_bit_memory_caching(self):
         self.runCmd("p buf_with_non_address")
         # This will read from the cache since the two pointers point to the
         # same place.
         self.runCmd("p buf")
 
         # Open log ignoring utf-8 decode errors
         with open(log_file, 'r', errors='ignore') as f:
             read_packet = "send packet: $x{:x}"
+
+            # Since we allocated a 4k page that page will be aligned to 4k, which
+            # also fits the 512 byte line size of the memory cache. So we can expect
+            # to find a packet with its exact address.
             read_buf_packet = read_packet.format(buf)
             read_buf_with_non_address_packet = read_packet.format(buf_with_non_address)
 
             # We expect to find 1 and only 1 read of buf.
             # We expect to find no reads using buf_with_no_address.
             found_read_buf = False
             for line in f:
                 if read_buf_packet in line:
                     if found_read_buf:
                         self.fail("Expected 1 read of buf but found more than one.")
                     found_read_buf = True
 
                 if read_buf_with_non_address_packet in line:
                     self.fail("Unexpected read of buf_with_non_address found.")
 
             if not found_read_buf:
                 self.fail("Did not find any reads of buf.")
+
+    @skipIfLLVMTargetMissing("AArch64")
+    def test_non_address_bit_memory_corefile(self):
+        self.runCmd("target create --core corefile")
+
+        self.expect("thread list", substrs=['stopped',
+                                            'stop reason = signal SIGSEGV'])
+
+        # No caching (the program/corefile are the cache) and no writing
+        # to memory. So just check that tagged/untagged addresses read
+        # the same location.
+
+        # These are known addresses in the corefile, since we won't have symbols.
+        buf = 0x0000ffffa75a5000
+        buf_with_non_address = 0xff0bffffa75a5000
+
+        expected = ["4c 4c 44 42", "LLDB"]
+        self.expect("memory read 0x{:x}".format(buf), substrs=expected)
+        self.expect("memory read 0x{:x}".format(buf_with_non_address), substrs=expected)
+
+        # This takes a more direct route to ReadMemory. As opposed to "memory read"
+        # above that might fix the addresses up front for display reasons.
+        self.expect("expression (char*)0x{:x}".format(buf), substrs=["LLDB"])
+        self.expect("expression (char*)0x{:x}".format(buf_with_non_address), substrs=["LLDB"])
+
+        def check_reads(addrs, method, num_bytes, expected):
+            error = lldb.SBError()
+            for addr in addrs:
+                if num_bytes is None:
+                    got = method(addr, error)
+                else:
+                    got = method(addr, num_bytes, error)
+
+            self.assertTrue(error.Success())
+            self.assertEqual(expected, got)
+
+        addr_buf = lldb.SBAddress()
+        addr_buf.SetLoadAddress(buf, self.target())
+        addr_buf_with_non_address = lldb.SBAddress()
+        addr_buf_with_non_address.SetLoadAddress(buf_with_non_address, self.target())
+        check_reads([addr_buf, addr_buf_with_non_address], self.target().ReadMemory,
+                    4, b'LLDB')
+
+        addrs = [buf, buf_with_non_address]
+        check_reads(addrs, self.process().ReadMemory, 4, b'LLDB')
+        check_reads(addrs, self.process().ReadCStringFromMemory, 5, "LLDB")
+        check_reads(addrs, self.process().ReadUnsignedFromMemory, 4, 0x42444c4c)
+        check_reads(addrs, self.process().ReadPointerFromMemory, None, 0x0000000042444c4c)

lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c

 #include <linux/mman.h>
 #include <sys/mman.h>
 #include <unistd.h>
 
 int main(int argc, char const *argv[]) {
   size_t page_size = sysconf(_SC_PAGESIZE);
   // Note that we allocate memory here because if we used
   // stack or globals lldb might read it in the course of
   // running to the breakpoint. Before the test can look
   // for those reads.
   char *buf = mmap(0, page_size, PROT_READ | PROT_WRITE,
                    MAP_ANONYMOUS | MAP_SHARED, -1, 0);
   if (buf == MAP_FAILED)
     return 1;
 
+  // Some known values to go in the corefile, since we cannot
+  // write to corefile memory.
+  buf[0] = 'L';
+  buf[1] = 'L';
+  buf[2] = 'D';
+  buf[3] = 'B';
+
 #define sign_ptr(ptr) __asm__ __volatile__("pacdza %0" : "=r"(ptr) : "r"(ptr))
 
   // Set top byte to something.
   char *buf_with_non_address = (char *)((size_t)buf | (size_t)0xff << 56);
   sign_ptr(buf_with_non_address);
   // Address is now:
   // <8 bit top byte tag><pointer signature><virtual address>
 
+  // Uncomment this line to crash and generate a corefile.
+  // Prints so we know what fixed address to look for in testing.
+  // printf("buf: %p\n", buf);
+  // printf("buf_with_non_address: %p\n", buf_with_non_address);
+  // *(char*)0 = 0;
+
   return 0; // Set break point at this line.
 }