[LLDB] Fix code breakpoints on tagged addresses

[DavidSpickett]

Summary

This allows you to break on the value of a function pointer
with non-address bits in it. For example your ABI might mandate
function pointer signing.

Previously it would either try to write to an address that was
incorrect, or try to set a hardware register incorrectly.

I have placed the ABI check before GetBreakableLoadAddress
on the logic that that function probably works best with a
fixed address.

This is somewhat academic because the only user of
GetBreakableLoadAddress is MIPS where we don't support any
form of address tagging.

[DavidSpickett]

Add a release note.

[DavidSpickett]

Tests will run only for armv8.3-a hardware so I've tested them with qemu-system. Existing tests pass on Armv8 Linux as before.

Some of these PAC tests could run on Apple Silicon now that I think about it. That's for another time, needs extra work.

[DavidSpickett]

If you want to understand the motivation beyond "this should work why doesn't it", here it is.

If your ABI mandated signing all function pointers you might store a callback in a struct somewhere. Not knowing what that callback points to you would like to be able to do breakpoint set -a mystruct.callback and not have to first print it, then break on the function name or manually fix the address.

[DavidSpickett]

Completely forgot about this, ping!

[jasonmolenda]

Hi David, thanks for the ping sorry for not replying earlier to this. I looked at this a little bit today but want to look at how I solved the same issue a while ago (but never upstreamed, which is my bad, I need to clean those up). (I did solved this in OptionArgParser::ToAddress and I haven't looked at this enough to argue for mine over your approach -- but mine gets you any address expression command like source lookup -a and image lookup -a etc. I'll look at this more closely this weekend and follow up though.

(I got distracted way too much when I tried to make a repo test case, and my function pointer variable was called f and it would never work. I looked at it with Jim and we were both confused mightily, until we noticed that OptionArgParser::ToAddress throws things through a "see if it can be parsed as base16 even without a prefix" so you can't use a variable name that consists of [a-f][0-9a-f]* lol. Jim or I are definitely going to be removing that little bit of too-cleverness.)

At first I was wondering if the ABI should save the ArchSpec it is created with so it can call the Architecture's GetBreakableLoadAddress instead of requiring all the callers to call both of these, but I see from the ppc64 description of what that method does, it would not be applicable to most code addresses, only function entry, and only when we were calling into this function I think?

[jasonmolenda]

I have a little example of a function pointer using ptrath in an arm64e (ARMv8.3) program, to show what I was talking about,

* thread #1, queue = 'com.apple.main-thread', stop reason = instruction step into
    frame #0: 0x0000000100003fa0 a.out`main + 40 at a.c:6
   1   	int foo () { return 5; }
   2   	
   3   	int main()
   4   	{
   5   	  int (*func)() = foo;
-> 6   	  return func();
   7   	}
Target 0: (a.out) stopped.

(lldb) p func
(int (*)(...)) $0 = 0xb868000100003f70 (actual=0x0000000100003f70 a.out`foo at a.c:1)

(lldb) reg read x8
      x8 = 0xb868000100003f70 (0x0000000100003f70) a.out`foo at a.c:1

(lldb) ima loo -a func
      Address: a.out[0x0000000100003f70] (a.out.__TEXT.__text + 0)
      Summary: a.out`foo at a.c:1

(lldb) br s -a func
Breakpoint 2: where = a.out`foo at a.c:1, address = 0x0000000100003f70

(lldb) mem read -c 1 -s 4 -f x func
0x100003f70: 0x528000a0

(lldb) x/1i func
    0x100003f70: 0x528000a0   mov    w0, #0x5
(lldb)

(nb out of the box installs of macOS will not run arm64e binaries like this, the ABI is not finalized -- it's meant to prevent people from sharing builds that may stop working some day when the ABI changes)

This process isn't using TBI, but the high bits end up being used for signing so it's the same effect. I located this in OptionArgParser::ToAddress() because we'll need this same behavior in

if (expr_result == eExpressionCompleted) {
  if (valobj_sp)
    valobj_sp = valobj_sp->GetQualifiedRepresentationIfAvailable(
        valobj_sp->GetDynamicValueType(), true);
  // Get the address to watch.
  if (valobj_sp)
    addr = valobj_sp->GetValueAsUnsigned(fail_value, &success);
  if (success) {
    if (error_ptr)
      error_ptr->Clear();
    if (abi_sp)
      addr = abi_sp->FixCodeAddress (addr);
    return addr;
  } else {

What do you think about locating this change in ToAddress instead of Target::GetBreakableLoadAddress? It looks like the one caller to GetBreakableLoadAddress is Target::CreateBreakpoint(addr_t addr) - which is probably called by an SBTarget method if we want to think of the most general purpose use case. I think stripping non-addressable bits in OptionArgParser::ToAddress is the right thing to do - but I don't have an opinion about whether it should be done in Target::GetBreakableLoadAddress or not.

One thing to note is that I also have an SBValue method that needs to be upstreamed, SBValue::GetValueAsAddress(). So if someone has an SBValue of a function pointer and they want to take the value of that func ptr and call SBTarget::CreateBreakpoint on it, I would say "well yes, you need to use GetValueAsAddress before you pass that to CreateBreakpoint".

[DavidSpickett]

What do you think about locating this change in ToAddress instead of Target::GetBreakableLoadAddress? It looks like the one caller to GetBreakableLoadAddress is Target::CreateBreakpoint(addr_t addr) - which is probably called by an SBTarget method if we want to think of the most general purpose use case. I think stripping non-addressable bits in OptionArgParser::ToAddress is the right thing to do - but I don't have an opinion about whether it should be done in Target::GetBreakableLoadAddress or not.

I had not checked the other lookup commands, so yeah this sounds good to me. Doing this fixing in fewer places is always better. Put up your change and I'll check the test from this patch against it.

One thing to note is that I also have an SBValue method that needs to be upstreamed, SBValue::GetValueAsAddress(). So if someone has an SBValue of a function pointer and they want to take the value of that func ptr and call SBTarget::CreateBreakpoint on it, I would say "well yes, you need to use GetValueAsAddress before you pass that to CreateBreakpoint".

If I understand correctly, this would effectively run FixCodeAddress on the value? One thing that I found doing the SB API for MTE (which was shelved, but besides the point) is that there was no way to fix addresses via the API (or get access to the ABI plugin in general). I ended up just adding FixDataAddress to SBProcess as a temporary thing. So something like ValueAsAddress could be very useful.

We have this distinction (that doesn't mean much) between code and data fixes. For now a single method will work fine, I'm hoping the code/data difference never gets to a point where we have to be really careful about it.
(as you say "This process isn't using TBI, but the high bits end up being used for signing so it's the same effect.", so FixCode and FixData are the same thing in that case)

[jasonmolenda]

In D136938#4005138, @DavidSpickett wrote:

What do you think about locating this change in ToAddress instead of Target::GetBreakableLoadAddress? It looks like the one caller to GetBreakableLoadAddress is Target::CreateBreakpoint(addr_t addr) - which is probably called by an SBTarget method if we want to think of the most general purpose use case. I think stripping non-addressable bits in OptionArgParser::ToAddress is the right thing to do - but I don't have an opinion about whether it should be done in Target::GetBreakableLoadAddress or not.

I had not checked the other lookup commands, so yeah this sounds good to me. Doing this fixing in fewer places is always better. Put up your change and I'll check the test from this patch against it.

OK sounds good, I'll put up a phab. I can't test it on macOS platforms because the bots won't be able to build & run arm64e (ARMv8.3 w/ ptrauth) binaries. :/

One thing to note is that I also have an SBValue method that needs to be upstreamed, SBValue::GetValueAsAddress(). So if someone has an SBValue of a function pointer and they want to take the value of that func ptr and call SBTarget::CreateBreakpoint on it, I would say "well yes, you need to use GetValueAsAddress before you pass that to CreateBreakpoint".

If I understand correctly, this would effectively run FixCodeAddress on the value? One thing that I found doing the SB API for MTE (which was shelved, but besides the point) is that there was no way to fix addresses via the API (or get access to the ABI plugin in general). I ended up just adding FixDataAddress to SBProcess as a temporary thing. So something like ValueAsAddress could be very useful.

Yeah my SBValue::GetValueAsAddress() simply passed it through FixCodeAddress which was correct enough for our current environment, but SBValue may have access to type information so maybe it could pick the correct Fix method to call given that information?

I've had some scripting people who also want methods to clear non-addressable bits in SBProcess, as they handle signed pointers in their own scripts, it's something I've been meaning to do for them -- or give them a way to get the address fixing bitmasks.

[jasonmolenda]

I think we both agree that OptionArgParser::ToAddress needs to fix the address, but I don't have an opinion about whether we should also do it in GetBreakableLoadAddress. I'm fine if you think we should do it here too, for other codepaths that can try to set a breakpoint on a signed function pointer address.

[DavidSpickett]

I can't test it on macOS platforms because the bots won't be able to build & run arm64e (ARMv8.3 w/ ptrauth) binaries. :/

Well our bots can't either but I've got QEMU locally is what I mean. We can run top byte ignore tests but top byte ignore being ignored by hardware sometimes makes tests pass anyway :)

I don't have an opinion about whether we should also do it in GetBreakableLoadAddress

I'll make a list of the paths to setting a breakpoint, to see which layer it makes the most sense to fix in. (which I'll get time to do sometime in January)

[DavidSpickett]

Superseded by https://reviews.llvm.org/D141629.

lldb/source/Target/Target.cpp

 //===-- Target.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
 //
 //===----------------------------------------------------------------------===//
 

 lldb::addr_t Target::GetOpcodeLoadAddress(lldb::addr_t load_addr,
                                           AddressClass addr_class) const {
   auto arch_plugin = GetArchitecturePlugin();
   return arch_plugin ? arch_plugin->GetOpcodeLoadAddress(load_addr, addr_class)
                      : load_addr;
 }
 
 lldb::addr_t Target::GetBreakableLoadAddress(lldb::addr_t addr) {
-  auto arch_plugin = GetArchitecturePlugin();
-  return arch_plugin ? arch_plugin->GetBreakableLoadAddress(addr, *this) : addr;
+  lldb::ABISP abi_plugin = m_process_sp ? m_process_sp->GetABI() : nullptr;
+  if (abi_plugin)
+    addr = abi_plugin->FixCodeAddress(addr);
+
+  if (auto arch_plugin = GetArchitecturePlugin())
+    addr = arch_plugin->GetBreakableLoadAddress(addr, *this);
+
+  return addr;
 }
 
 SourceManager &Target::GetSourceManager() {
   if (!m_source_manager_up)
     m_source_manager_up = std::make_unique<SourceManager>(shared_from_this());
   return *m_source_manager_up;
 }
 

lldb/test/API/linux/aarch64/tagged_code_break/Makefile

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

lldb/test/API/linux/aarch64/tagged_code_break/TestAArch64LinuxTaggedCodeBreak.py

+"""
+Test that "breakpoint set -a" uses the ABI plugin to remove non-address bits
+before attempting to set a breakpoint.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxTaggedCodeBreak(TestBase):
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def do_tagged_break(self, hardware):
+        if not self.isAArch64PAuth():
+            self.skipTest('Target must support pointer authentication.')
+
+        self.build()
+        self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+        lldbutil.run_break_set_by_file_and_line(self, "main.c",
+            line_number('main.c', '// Set break point at this line.'),
+            num_expected_locations=1)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        if self.process().GetState() == lldb.eStateExited:
+            self.fail("Test program failed to run.")
+
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+            substrs=['stopped',
+                     'stop reason = breakpoint'])
+
+        cmd = "breakpoint set -a fnptr"
+        # LLDB only has the option to force hardware break, not software.
+        # It prefers sofware break when it can and this will be one of those cases.
+        if hardware:
+            cmd += " --hardware"
+        self.expect(cmd)
+
+        self.runCmd("continue")
+        self.assertEqual(self.process().GetState(), lldb.eStateStopped)
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+            substrs=['stopped', '`foo at main.c', 'stop reason = breakpoint'])
+
+    # AArch64 Linux always enables the top byte ignore feature
+    @skipUnlessArch("aarch64")
+    @skipUnlessPlatform(["linux"])
+    def test_software_break(self):
+        self.do_tagged_break(False)
+
+    @skipUnlessArch("aarch64")
+    @skipUnlessPlatform(["linux"])
+    def test_hardware_break(self):
+        self.do_tagged_break(True)
 No newline at end of file

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

+#include <stdint.h>
+
+void foo(void) {}
+typedef void (*FooPtr)(void);
+
+int main() {
+  FooPtr fnptr = foo;
+  // Set top byte.
+  fnptr = (FooPtr)((uintptr_t)fnptr | (uintptr_t)0xff << 56);
+  // Then apply a PAuth signature to it.
+  __asm__ __volatile__("pacdza %0" : "=r"(fnptr) : "r"(fnptr));
+  // fnptr is now:
+  // <8 bit top byte tag><pointer signature><virtual address>
+
+  foo(); // Set break point at this line.
+
+  return 0;
+}