[libunwind] Support rtld-c18n as the runtime linker. #731

dstolfa · 2024-03-04T23:25:30Z

This is an initial take on a possible implementation to support the c18n runtime linker ABI in libunwind. The last commit message should describe the approach. I will note that we could consider putting the CompartmentInfo data in the unwind cursor, but my worry is that whatever is calling it will have access to that data, which would leak other compartments' restricted stack capabilities. Perhaps there's a way to reliably limit access to that part of the capability that I'm not aware of? A temporary "memory leak" sounded like a better idea otherwise (as described in the commit message), even though it makes me a bit uneasy.

This depends on CTSRD-CHERI/cheribsd#2032 in order to actually work with the c18n runtime linker, but libunwind should continue to work as normal without that patch, it just won't be able to unwinding compartments. The two are bundled together in CTSRD-CHERI/cheribsd#2003 for ease of building.

dstolfa · 2024-03-18T20:06:47Z

Updated the review to address a comment from @jrtc27 on another PR noting that concurrent unwinding of multiple contexts and cursors from the same thread is a part of the current libunwind API. Moved the pointer to unwind information for compartments to the caller stack, but optionally (_LIBUNWIND_SANDBOX_HARDENED) seal the pointer so that the caller cannot dereference or modify it. Since the change was already ABI breaking for both the context and cursor, this is probably okay to do as well.

Note that we use the heap to allocate the actual hash table as it essentially quadruples the size of the unwind cursor on the stack otherwise (going from ~1K to 4K per cursor).

jrtc27 · 2024-03-18T22:21:17Z

Doesn't this leak the tables? I don't think the destructor of UnwindCursor, and thus of CompartmentInfo, is normally called, given it's normally just a stack-allocated unw_cursor_t which is reinterpreted as an UnwindCursor.

dstolfa · 2024-03-19T02:39:16Z

Doesn't this leak the tables? I don't think the destructor of UnwindCursor, and thus of CompartmentInfo, is normally called, given it's normally just a stack-allocated unw_cursor_t which is reinterpreted as an UnwindCursor.

You're right, it does. I'd completely glossed over the fact that the destructor wouldn't be called. This makes the problem slightly harder because I don't see an easy way to use the heap here, which then makes me concerned about cases where we have lots of compartments that we need to unwind through.

Obviously for backtraces and exception handling we could clean up in the respective functions, but it doesn't address the wider problem of libunwind APIs. I'll think about it a bit more as I'd really like to avoid both introducing limitations on how many compartments we can unwind and introducing any new API to libunwind.

dpgao · 2024-03-21T15:26:21Z

libunwind/src/DwarfInstructions.hpp

+        uintcap_t csp = registers.getUnsealedECSP(sealer);
+	CompartmentInfo &CI = CompartmentInfo::sThisCompartmentInfo;
+        for (;;) {
+          if (isEndOfExecutiveStack(csp, CI)) {


Is it possible that we are at the end of the Executive stack and yet still have frames to unwind? For example, we are in RTLD and have not made any compartment transitions, so the Executive stack would contain no frame.

To handle situations like this, I guess we can do something like

while (!isEndOfExecutiveStack(...) && isTrampoline(...)) { returnAddress = ... csp = ... }

Is that a situation we would ever end up in with libunwind? For us to begin unwinding, someone would have to have called unw_getcontext and unw_init_local, presumably in one of the compartments.

libunwind/src/UnwindCursor.hpp

libunwind/src/Registers.hpp

libunwind/src/DwarfInstructions.hpp

libunwind/src/UnwindRegistersRestore.S

dstolfa · 2024-03-26T13:11:36Z

Address some of the above comments and import changes from the demo branch.

dstolfa · 2024-03-26T22:19:15Z

Updated to support the changes in rtld.

libunwind/src/CompartmentInfo.hpp

libunwind/src/DwarfInstructions.hpp

bsdjhb

I think it would be easier to review (and easier to re-read in the future for history purposes) if the optional bits were split out into follow-up commits. That is, first a commit to add the base c18n unwinder, then a commit to add the first option (and include the cmake build glue as part of that commit), then a third commit to add the second option.

libunwind/src/Registers.hpp

libunwind/src/DwarfInstructions.hpp

dstolfa · 2024-04-29T18:43:26Z

Rebased on dev, merged #732 into this code and addressed comments from @dpgao and @bsdjhb. This code now depends on changes from c18n-nobot in CheriBSD which I believe are contained in this review: CTSRD-CHERI/cheribsd#2090.

dstolfa · 2024-06-01T10:57:46Z

With CTSRD-CHERI/cheribsd#2090 having landed, this should now be good to go.

jrtc27 · 2024-06-03T21:56:16Z

libunwind/src/unwind_cheri.h

+#define __UNWIND_CHERI_H__
+
+#ifdef __CHERI_PURE_CAPABILITY__
+#define	_LIBUNWIND_CHERI_PERM_GLOBAL             (1 << 0) /* 0x00000001 */


This is for Morello specifically, not CHERI; the permissions are called different things, have different values and even the set of permissions that exists varies across architectures

I'm not a big fan of this header existing in the first place, but perhaps for now just #if defined(__CHERI_PURE_CAPABILITY__) && defined(_LIBUNWIND_TARGET_AARCH64) will do?

Why does it even exist? I can't see any references to these defines.

jrtc27 · 2024-06-03T22:03:50Z

libunwind/src/Registers.hpp

@@ -4544,6 +4607,14 @@ class _LIBUNWIND_HIDDEN Registers_riscv {
  void        setSP(reg_t value) { _registers[2] = value; }
  reg_t       getIP() const { return _registers[0]; }
  void        setIP(reg_t value) { _registers[0] = value; }
+#if defined(__CHERI_PURE_CAPABILITY__) && defined(_LIBUNWIND_SANDBOX_OTYPES)
+  reg_t       getUnsealedECSP(uintptr_t sealer) { return getSP(); }


This makes no sense. CHERI-RISC-V has no such thing as an executive mode, so it should not have methods involving an executive stack pointer.

Yeah it was shim to make it build on CHERI-RISC-V. It's called ECSP because it was originally made for Morello, but would probably work similarly in CHERI-RISC-V (assuming otypes end up in there and we make use of them in libunwind). I'll rename usage of ECSP to TrustedStack or something along those lines to relay what it's supposed to mean. Alternatively, if we really don't want to make use of otypes I could just gate all calls behind a Morello ifdef.

jrtc27 · 2024-06-03T22:04:22Z

libunwind/src/Registers.hpp

+#ifdef _LIBUNWIND_SANDBOX_HARDENED
+  void        unsealSP(uintptr_t sealer) {}
+  void        unsealFP(uintptr_t sealer) {}
+  void        unsealCalleeSavedRegisters(uintptr_t sealer) {}


One should not be able to enable _LIBUNWIND_SANDBOX_OTYPES / _LIBUNWIND_SANDBOX_HARDENED on CHERI-RISC-V, it doesn't make sense to do when there is no c18n for it.

I'll add a guard as I make the other changes.

jrtc27 · 2024-06-03T22:06:02Z

libunwind/CMakeLists.txt

@@ -50,6 +50,8 @@ option(LIBUNWIND_IS_BAREMETAL "Build libunwind for baremetal targets." OFF)
 option(LIBUNWIND_USE_FRAME_HEADER_CACHE "Cache frame headers for unwinding. Requires locking dl_iterate_phdr." OFF)
 option(LIBUNWIND_REMEMBER_HEAP_ALLOC "Use heap instead of the stack for .cfi_remember_state." OFF)
 option(LIBUNWIND_INSTALL_HEADERS "Install the libunwind headers." OFF)
+option(LIBUNWIND_SANDBOX_OTYPES "Use a libunwind implementation that assumes a c18n RTLD using otypes." ON)
+option(LIBUNWIND_SANDBOX_HARDENED "Harden the current sandbox implementation." ON)


I would rather not have more knobs than we need. We should have one that is "build libunwind for c18n", whatever that means, and whether that's "supports and requires c18n" or just "supports c18n if in use at run time".

Also bear in mind libunwind isn't CheriBSD-specific, it gets used on Morello Linux too, which won't have support for any of this, and should remain unchanged.

That was the plan, but through discussion with Robert there was a desire to be able to turn off any usage of otypes in libunwind itself. I gated it behind LIBUNWIND_SANDBOX_HARDENED as a result, so that one can compile a c18n-aware libunwind that doesn't try to protect anything using otypes. I'm happy to remove it if we're now completely fine with otype usage so that if you compile libunwind with c18n support, it will always make use of them? The name of LIBUNWIND_SANDBOX_OTYPES is quite misleading at this point, because its original meaning was: "support rtld-c18n that makes use of otypes". Perhaps it should be changed to LIBUNWIND_C18N_SUPPORT or something along those lines? Alternatively if we decide that we always want to actually make use of otypes, it could stay the way it is as it would imply that otypes are being used in addition to supporting c18n.

Renaming it is the right thing to do if it's about c18n not otypes. Whether it should be LIBUNWIND_C18N_SUPPORT or just LIBUNWIND_C18N depends on what happens if you build with the option enabled and run with a non-c18n rtld.

jrtc27 · 2024-06-03T22:07:06Z

libunwind/src/UnwindRegistersRestore.S

@@ -703,6 +703,25 @@ Lnovec:

 #elif defined(__aarch64__)

+#if defined(__CHERI_PURE_CAPABILITY__)
+DEFINE_LIBUNWIND_FUNCTION(__rtld_unw_setcontext)


Comment its prototype above like below?

Also, does this really need to be in assembly? rtld's overriding implementation isn't.

rtld's overriding implementation is in C because it calls into unwind_stack(), but the non-c18n implementation tail calls into the same assembly code as this (https://github.com/CTSRD-CHERI/cheribsd/blob/dev/libexec/rtld-elf/rtld_c18n.c#L1026-L1029). It's not great, but it was the simplest way to get the values in the right registers without trying to come up with some kind of ABI between libunwind and rtld. It's not great and should probably be reworked at some point in the future, perhaps with the CHERI-RISC-V implementation, but I probably don't have time to do that for the release.

jrtc27 · 2024-06-03T22:16:07Z

libunwind/src/UnwindRegistersRestore.S

 #else
-  ret                           // jump to pcc
+  b      _rtld_unw_setcontext_unsealed


If _LIBUNWIND_SANDBOX_OTYPES is off surely we can just act like before rather than needing to make a function call?

I think so. I was just avoiding adding code duplication since the thing we'd basically do is the same thing that's in the function call. Alternatively I could simply #ifdef _LIBUNWIND_SANDBOX_OTYPES the new code and then #else whatever we had there before to keep the functionality 1:1 when the define is not specified, but it would add some duplication.

jrtc27 · 2024-06-03T22:16:22Z

libunwind/src/UnwindRegistersRestore.S

@@ -703,6 +703,25 @@ Lnovec:

 #elif defined(__aarch64__)

+#if defined(__CHERI_PURE_CAPABILITY__)


Should this not be gated by _LIBUNWIND_SANDBOX_OTYPES?

It works fine as-is on a CheriBSD rtld that doesn't support c18n, but if the goal is to ifdef the whole thing out and keep libunwind behaving exactly the way it did before (in the sense that not defining _LIBUNWIND_SANDBOX_OTYPES causes all the changes to not be compiled in and we're left with the source of libunwind pre-c18n support 1:1) we could put it behind an #ifdef _LIBUNWIND_SANDBOX_OTYPES as it simply won't be used unless that is defined.

I mean, part of the point is that __rtld_foo isn't going to be the naming convention for other OS's dynamic linkers.

ACK. I'm leaning towards leaving it as-is for now because this will likely change as we redesign the APIs post-release anyway. This bit of the code needs to have a cleaner ABI that doesn't depend on the structure of the unwind context. It's very much tied to both libunwind and CheriBSD's rtld. We just need to make sure to document this in the c18n man page.

libunwind/src/CompartmentInfo.hpp

libunwind/src/libunwind.cpp

jrtc27 · 2024-06-03T22:24:00Z

libunwind/include/libunwind.h

@@ -678,7 +678,8 @@ enum {
  UNW_ARM64_C30 = 228,
  UNW_ARM64_CLR = 228,
  UNW_ARM64_C31 = 229,
-  UNW_ARM64_CSP = 229
+  UNW_ARM64_CSP = 229,
+  UNW_ARM64_ECSP = 230,


230 is PCC per https://github.com/ARM-software/abi-aa/blob/main/aadwarf64-morello/aadwarf64-morello.rst#dwarf-register-names

I see that it's reserved up to 233, so perhaps 234 or 240?

dstolfa

Thanks for the detailed feedback! I'd be interested to hear what you think on some of the bits around refactoring/ABIs/ifdefs brought up to avoid ping-ponging due to misunderstanding on what exactly it should look like. In the meantime I'll start making the obvious changes.

dstolfa · 2024-06-04T11:52:36Z

libunwind/include/libunwind.h

@@ -678,7 +678,8 @@ enum {
  UNW_ARM64_C30 = 228,
  UNW_ARM64_CLR = 228,
  UNW_ARM64_C31 = 229,
-  UNW_ARM64_CSP = 229
+  UNW_ARM64_CSP = 229,
+  UNW_ARM64_ECSP = 230,


I see that it's reserved up to 233, so perhaps 234 or 240?

dstolfa · 2024-06-04T11:57:35Z

libunwind/CMakeLists.txt

@@ -50,6 +50,8 @@ option(LIBUNWIND_IS_BAREMETAL "Build libunwind for baremetal targets." OFF)
 option(LIBUNWIND_USE_FRAME_HEADER_CACHE "Cache frame headers for unwinding. Requires locking dl_iterate_phdr." OFF)
 option(LIBUNWIND_REMEMBER_HEAP_ALLOC "Use heap instead of the stack for .cfi_remember_state." OFF)
 option(LIBUNWIND_INSTALL_HEADERS "Install the libunwind headers." OFF)
+option(LIBUNWIND_SANDBOX_OTYPES "Use a libunwind implementation that assumes a c18n RTLD using otypes." ON)
+option(LIBUNWIND_SANDBOX_HARDENED "Harden the current sandbox implementation." ON)


That was the plan, but through discussion with Robert there was a desire to be able to turn off any usage of otypes in libunwind itself. I gated it behind LIBUNWIND_SANDBOX_HARDENED as a result, so that one can compile a c18n-aware libunwind that doesn't try to protect anything using otypes. I'm happy to remove it if we're now completely fine with otype usage so that if you compile libunwind with c18n support, it will always make use of them? The name of LIBUNWIND_SANDBOX_OTYPES is quite misleading at this point, because its original meaning was: "support rtld-c18n that makes use of otypes". Perhaps it should be changed to LIBUNWIND_C18N_SUPPORT or something along those lines? Alternatively if we decide that we always want to actually make use of otypes, it could stay the way it is as it would imply that otypes are being used in addition to supporting c18n.

libunwind/src/CompartmentInfo.hpp

dstolfa · 2024-06-04T12:01:14Z

libunwind/src/Registers.hpp

@@ -4544,6 +4607,14 @@ class _LIBUNWIND_HIDDEN Registers_riscv {
  void        setSP(reg_t value) { _registers[2] = value; }
  reg_t       getIP() const { return _registers[0]; }
  void        setIP(reg_t value) { _registers[0] = value; }
+#if defined(__CHERI_PURE_CAPABILITY__) && defined(_LIBUNWIND_SANDBOX_OTYPES)
+  reg_t       getUnsealedECSP(uintptr_t sealer) { return getSP(); }


Yeah it was shim to make it build on CHERI-RISC-V. It's called ECSP because it was originally made for Morello, but would probably work similarly in CHERI-RISC-V (assuming otypes end up in there and we make use of them in libunwind). I'll rename usage of ECSP to TrustedStack or something along those lines to relay what it's supposed to mean. Alternatively, if we really don't want to make use of otypes I could just gate all calls behind a Morello ifdef.

dstolfa · 2024-06-04T12:02:09Z

libunwind/src/Registers.hpp

+#ifdef _LIBUNWIND_SANDBOX_HARDENED
+  void        unsealSP(uintptr_t sealer) {}
+  void        unsealFP(uintptr_t sealer) {}
+  void        unsealCalleeSavedRegisters(uintptr_t sealer) {}


I'll add a guard as I make the other changes.

dstolfa · 2024-06-04T12:07:40Z

libunwind/src/UnwindRegistersRestore.S

 #else
-  ret                           // jump to pcc
+  b      _rtld_unw_setcontext_unsealed


I think so. I was just avoiding adding code duplication since the thing we'd basically do is the same thing that's in the function call. Alternatively I could simply #ifdef _LIBUNWIND_SANDBOX_OTYPES the new code and then #else whatever we had there before to keep the functionality 1:1 when the define is not specified, but it would add some duplication.

libunwind/src/libunwind.cpp

dstolfa · 2024-06-04T12:08:31Z

libunwind/src/unwind_cheri.h

+#define __UNWIND_CHERI_H__
+
+#ifdef __CHERI_PURE_CAPABILITY__
+#define	_LIBUNWIND_CHERI_PERM_GLOBAL             (1 << 0) /* 0x00000001 */


I'm not a big fan of this header existing in the first place, but perhaps for now just #if defined(__CHERI_PURE_CAPABILITY__) && defined(_LIBUNWIND_TARGET_AARCH64) will do?

dstolfa · 2024-06-04T12:15:17Z

libunwind/src/UnwindRegistersRestore.S

@@ -703,6 +703,25 @@ Lnovec:

 #elif defined(__aarch64__)

+#if defined(__CHERI_PURE_CAPABILITY__)
+DEFINE_LIBUNWIND_FUNCTION(__rtld_unw_setcontext)


rtld's overriding implementation is in C because it calls into unwind_stack(), but the non-c18n implementation tail calls into the same assembly code as this (https://github.com/CTSRD-CHERI/cheribsd/blob/dev/libexec/rtld-elf/rtld_c18n.c#L1026-L1029). It's not great, but it was the simplest way to get the values in the right registers without trying to come up with some kind of ABI between libunwind and rtld. It's not great and should probably be reworked at some point in the future, perhaps with the CHERI-RISC-V implementation, but I probably don't have time to do that for the release.

dstolfa · 2024-06-04T12:19:24Z

libunwind/src/UnwindRegistersRestore.S

@@ -703,6 +703,25 @@ Lnovec:

 #elif defined(__aarch64__)

+#if defined(__CHERI_PURE_CAPABILITY__)


It works fine as-is on a CheriBSD rtld that doesn't support c18n, but if the goal is to ifdef the whole thing out and keep libunwind behaving exactly the way it did before (in the sense that not defining _LIBUNWIND_SANDBOX_OTYPES causes all the changes to not be compiled in and we're left with the source of libunwind pre-c18n support 1:1) we could put it behind an #ifdef _LIBUNWIND_SANDBOX_OTYPES as it simply won't be used unless that is defined.

jrtc27

I don't understand the hardening model and what specifically it's protecting against. What stops me from unwinding into a caller, copying some of the sealed registers, aborting that unwind, starting a new one that stays within my compartment, injecting that register and getting it back unsealed? I am skeptical that the interface can meaningfully be hardened, and that in trying to do so we're adding a whole bunch of complexity that doesn't really buy any security, just an illusion thereof.

The first commit in this series is also a bit screwed up. There's all kinds of mention of sealers despite not having hardening in it, and at least one _LIBUNWIND_SANDBOX_HARDENED check crept in.

jrtc27 · 2024-06-07T19:25:15Z

libunwind/CMakeLists.txt

@@ -50,6 +50,8 @@ option(LIBUNWIND_IS_BAREMETAL "Build libunwind for baremetal targets." OFF)
 option(LIBUNWIND_USE_FRAME_HEADER_CACHE "Cache frame headers for unwinding. Requires locking dl_iterate_phdr." OFF)
 option(LIBUNWIND_REMEMBER_HEAP_ALLOC "Use heap instead of the stack for .cfi_remember_state." OFF)
 option(LIBUNWIND_INSTALL_HEADERS "Install the libunwind headers." OFF)
+option(LIBUNWIND_SANDBOX_OTYPES "Use a libunwind implementation that assumes a c18n RTLD using otypes." OFF)
+option(LIBUNWIND_SANDBOX_HARDENED "Harden the current sandbox implementation." OFF)


Can we please only use one of compartmentalisation/c18n and sandbox?

jrtc27 · 2024-06-07T19:27:18Z

libunwind/CMakeLists.txt

@@ -293,6 +295,19 @@ if (NOT LIBUNWIND_ENABLE_THREADS)
  add_compile_flags(-D_LIBUNWIND_HAS_NO_THREADS)
 endif()

+# Sandboxing and c18n support
+if (LIBUNWIND_SANDBOX_OTYPES)
+  if (NOT CMAKE_SYSTEM_PROCESSOR MATCHES "^aarch64c" OR NOT CMAKE_SYSTEM_NAME MATCHES "FreeBSD")


cheribuild sets this to ARM64. I don't know what it's meant to be on FreeBSD for aarch64, probably aarch64. Given the triple is aarch64-unknown-freebsdX.Y even for purecap it's probably a bug that it reports as aarch64c.

Please fix this so cheribuild can enable c18n support when building libunwind standalone (you can test it with cheribuild libunwind-morello-purecap --libunwind-morello-purecap/cmake-options=-DLIBUNWIND_CHERI_C18N_SUPPORT)

jrtc27 · 2024-06-07T19:32:45Z

libunwind/src/AddressSpace.hpp

+/// Call into the RTLD to get a sealer capability. This sealer will be used to
+/// seal information in the unwinding context if _LIBUNWIND_SANDBOX_HARDENED is
+/// specified.
+LocalAddressSpace::pint_t _rtld_unw_getsealer(void);


Use types for the rtld interface that are what it actually is there

jrtc27 · 2024-06-07T20:06:17Z

libunwind/src/DwarfInstructions.hpp

+  static pint_t restoreRegistersFromSandbox(pint_t csp, A &addressSpace,
+                                            R &newRegisters,
+                                            CompartmentInfo &CI, pint_t sealer);
+  static bool isTrampoline(pint_t ecsp, A &addressSpace, CompartmentInfo &CI,


There are other types of trampolines in the world. Name should be specific in it being a compartment transition trampoline.

jrtc27 · 2024-06-07T20:14:42Z

libunwind/src/libunwind.cpp

+  LocalAddressSpace &addressSpace = LocalAddressSpace::sThisAddressSpace;
+  pint_t sealer = addressSpace.getUnwindSealer();
+  if (addressSpace.isValidSealer(sealer)) {
+#ifdef _LIBUNWIND_SANDBOX_HARDENED


Why not just add it to the existing #if? If this isn't defined we do a bunch of queries only to then do nothing with the result.

jrtc27 · 2024-06-07T20:15:27Z

libunwind/src/unwind_cheri.h

+#define __UNWIND_CHERI_H__
+
+#ifdef __CHERI_PURE_CAPABILITY__
+#define	_LIBUNWIND_CHERI_PERM_GLOBAL             (1 << 0) /* 0x00000001 */


Why does it even exist? I can't see any references to these defines.

jrtc27 · 2024-06-10T16:10:21Z

libunwind/src/DwarfInstructions.hpp

+  CHERI_DBG("SANDBOX: SETTING RESTRICTED CSP: %#p\n",
+            (void *)newRegisters.getSP());
+  size_t offset =
+      restoreCalleeSavedRegisters(csp, addressSpace, newRegisters, CI);


Why does this need to be in another method? It's not an operation that makes sense to perform on its own.

jrtc27 · 2024-06-10T16:13:06Z

libunwind/src/DwarfInstructions.hpp

@@ -274,7 +372,9 @@ int DwarfInstructions<A, R>::stepWithDwarf(A &addressSpace, pc_t pc,
      //
      // We set the SP here to the CFA, allowing for it to be overridden
      // by a CFI directive later on.
-      newRegisters.setSP(cfa);
+      pint_t newSP = cfa;


This local variable is pointless, just inline cfa in the two uses directly below

libunwind/src/DwarfInstructions.hpp

jrtc27 · 2024-06-10T16:30:44Z

libunwind/src/Registers.hpp

@@ -4544,6 +4565,9 @@ class _LIBUNWIND_HIDDEN Registers_riscv {
  void        setSP(reg_t value) { _registers[2] = value; }
  reg_t       getIP() const { return _registers[0]; }
  void        setIP(reg_t value) { _registers[0] = value; }
+#if defined(__CHERI_PURE_CAPABILITY__) && defined(_LIBUNWIND_CHERI_C18N_SUPPORT)
+  reg_t       getUnsealedTrustedStack(uintptr_t sealer) { return getSP(); }


Why? The option shouldn't have been enabled in the first place.

jrtc27 · 2024-06-10T16:33:42Z

libunwind/src/Registers.hpp

@@ -1863,17 +1863,23 @@ class _LIBUNWIND_HIDDEN Registers_arm64 {
 #ifdef __CHERI_PURE_CAPABILITY__
  bool        validCapabilityRegister(int num) const;
  uintcap_t   getCapabilityRegister(int num) const;
+#ifdef _LIBUNWIND_CHERI_C18N_SUPPORT
+  uintptr_t   getUnsealedTrustedStack(uintptr_t sealer) const;


Just have a getTrustedStack and make the caller unseal? Plus move down next to the setTrustedStack and declare the body inline.

libunwind/src/UnwindRegistersRestore.S

jrtc27 · 2024-06-10T17:25:48Z

libunwind/src/UnwindRegistersRestore.S

@@ -703,6 +703,25 @@ Lnovec:

 #elif defined(__aarch64__)

+#if defined(__CHERI_PURE_CAPABILITY__)


I mean, part of the point is that __rtld_foo isn't going to be the naming convention for other OS's dynamic linkers.

jrtc27 · 2024-06-10T17:26:06Z

libunwind/src/UnwindRegistersRestore.S

@@ -703,6 +703,24 @@ Lnovec:

 #elif defined(__aarch64__)

+//
+// extern "C" void __rtld_unw_setcontext(void *c0, void *c1, void *rcsp, void **buf);


What's buf and why is it pointing into the middle of the unwind context?

jrtc27 · 2024-06-10T17:31:01Z

libunwind/src/UnwindRegistersSave.S

  mov    c1,csp
-  str    c1,      [c0, #0x1f0]
+  stp    c30, c1, [c0, #0x1e0]


Leave it as it was to match the upstream non-Morello code

jrtc27 · 2024-06-10T17:32:18Z

libunwind/src/UnwindCursor.hpp

@@ -1294,7 +1294,7 @@ class UnwindCursor : public AbstractUnwindCursor{
  }
 #endif // defined(_LIBUNWIND_SUPPORT_TBTAB_UNWIND)

-  A               &_addressSpace;
+  A                &_addressSpace;


libunwind/src/CompartmentInfo.hpp

libunwind/src/DwarfInstructions.hpp

libunwind/src/Registers.hpp

jrtc27

Just some nits and a build system issue left

jrtc27 · 2024-06-11T18:51:14Z

libunwind/CMakeLists.txt

@@ -293,6 +295,19 @@ if (NOT LIBUNWIND_ENABLE_THREADS)
  add_compile_flags(-D_LIBUNWIND_HAS_NO_THREADS)
 endif()

+# Sandboxing and c18n support
+if (LIBUNWIND_SANDBOX_OTYPES)
+  if (NOT CMAKE_SYSTEM_PROCESSOR MATCHES "^aarch64c" OR NOT CMAKE_SYSTEM_NAME MATCHES "FreeBSD")


Please fix this so cheribuild can enable c18n support when building libunwind standalone (you can test it with cheribuild libunwind-morello-purecap --libunwind-morello-purecap/cmake-options=-DLIBUNWIND_CHERI_C18N_SUPPORT)

libunwind/include/libunwind.h

libunwind/src/DwarfInstructions.hpp

libunwind/src/UnwindRegistersRestore.S

jrtc27 · 2024-06-12T16:35:01Z

libunwind/src/UnwindRegistersSave.S

@@ -885,7 +898,7 @@ DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext)
  str    d30,     [c0, #0x0f0]
  str    d31,     [c0, #0x0f8]
  mov    x0, #0                   // return UNW_ESUCCESS
-  ret
+  b      _rtld_unw_getcontext


Thinking about this more, won't PLTs and lazy binding screw this up? c16 and c17 can be clobbered by PLTs, and lazy binding can clobber more.

I'm not sure I follow. _rtld_unw_getcontext doesn't use c16, and _rtld_unw_setcontext only uses it inside the function itself to temporarily store the value of c2. Why does it matter if the PLT makes use of c16 and c17?

In the unw_getcontext path we stp c16,c17, [c0, #0x100] first, then we branch to _rtld_unw_getcontext, going through the PLT (which clobbers c16 and c17, but we have the old values at c0+0x100), but gives us a sealed executive stack pointer.

In the _rtld_unw_setcontext path (resume), c16 and c17 don't get restored anyway since they were clobbered (there's already a comment stating that there), then we go through the PLT and clobber them again, and then in the assembly inside rtld we clobber c16 yet again to temporarily store the value of c2.

Perhaps I'm missing something here? If you mean that the call to unw_getcontext would go via the PLT, that is handled by marking unw_getcontext as a trusted function in the c18n policy, so it should never go through the PLT.

Oh I meant to comment on the setcontext one, just distracted by the concurrent meeting. In that case IP0/IP1 aren't a concern, but lazy binding still is. Specifically, temporary registers aren't saved by _rtld_bind_start, because they're not meant to be live across calls. To avoid that you'd either need to make an indirect call or backport and use variant PCS support (https://reviews.freebsd.org/D44869 has the stack).

(Also, re "marking unw_getcontext as a trusted function in the c18n policy, so it should never go through the PLT", it'll still go though a PLT, just not through an additional compartment switch trampoline)

Oh I meant to comment on the setcontext one, just distracted by the concurrent meeting. In that case IP0/IP1 aren't a concern, but lazy binding still is. Specifically, temporary registers aren't saved by _rtld_bind_start, because they're not meant to be live across calls. To avoid that you'd either need to make an indirect call or backport and use variant PCS support (https://reviews.freebsd.org/D44869 has the stack).

Ah. That's slightly annoying. How do we want to approach this for the release? Both of these sound a bit risky w.r.t. regressions. It seems unlikely that exception handling code will care about this in any meaningful way, but it does break libunwind API semantics...

jrtc27

One style nit, otherwise this is fine for "v0"

jrtc27 · 2024-06-13T17:37:11Z

libunwind/src/UnwindRegistersRestore.S

+  ldp    c0, c1,  [c0, #0x000]
+  // XXX: variant PCS is not yet supported by rtld, work around it
+  // using a function pointer.
+  adrp c16, :got:_rtld_unw_setcontext


Align the operands like above for these instructions

This commit adds support for backtrace and exception handling in libunwind when the process is running with the compartmentalization runtime linker. The unwinding process remains the same until a trampoline is encountered as the return address. This means that we are crossing compartment boundaries and we need to gather the unwind information from the runtime linker. We do this by reading information from the executive stack that the runtime linker populates for us in unw_getcontext. It also adds a new class, CompartmentInfo, which is responsible for abstracting away the details of c18n compartments. Currently, it is only used to define the constants relating to the trusted frame layout. The otype allocated to libunwind is given to libunwind by the runtime linker via the _rtld_unw_getsealer function, and as such this code is guarded by a LIBUNWIND_SANDBOX_OTYPES define. The sealer is used to internally access the executive stack pointer in order to unwind through compartment boundaries. This design may change in the future. This functionality only works on Morello right now.

jrtc27 · 2024-06-13T17:57:08Z

Feel free to merge once both Linux and FreeBSD Jenkins jobs are done (though something's gone very wrong if that change has broken something...)

dstolfa requested review from jrtc27, arichardson and dpgao March 4, 2024 23:25

dstolfa mentioned this pull request Mar 4, 2024

c18n, libgcc_s: Support a c18n-aware libunwind. CTSRD-CHERI/cheribsd#2032

Merged

dstolfa added libunwind ABI breaking change labels Mar 4, 2024

dstolfa force-pushed the cppexcept branch from cfdbc19 to fc157bc Compare March 18, 2024 17:52

dstolfa requested a review from bsdjhb March 18, 2024 19:06

dstolfa force-pushed the cppexcept branch from fc157bc to 73e475f Compare March 21, 2024 13:55

dpgao reviewed Mar 21, 2024

View reviewed changes

dstolfa force-pushed the cppexcept branch from 73e475f to c40d7d0 Compare March 26, 2024 13:09

dstolfa force-pushed the cppexcept branch from c40d7d0 to f907c4a Compare March 26, 2024 22:17

dstolfa force-pushed the cppexcept branch 3 times, most recently from 4459f80 to facade9 Compare March 28, 2024 12:41

dpgao reviewed Apr 18, 2024

View reviewed changes

libunwind/src/CompartmentInfo.hpp Outdated Show resolved Hide resolved

libunwind/src/DwarfInstructions.hpp Outdated Show resolved Hide resolved

bsdjhb reviewed Apr 24, 2024

View reviewed changes

libunwind/src/Registers.hpp Outdated Show resolved Hide resolved

libunwind/src/Registers.hpp Outdated Show resolved Hide resolved

libunwind/src/DwarfInstructions.hpp Outdated Show resolved Hide resolved

dstolfa force-pushed the cppexcept branch from facade9 to be0fe1a Compare April 29, 2024 18:37

jrtc27 reviewed Jun 3, 2024

View reviewed changes

dstolfa commented Jun 4, 2024

View reviewed changes

dstolfa force-pushed the cppexcept branch from be0fe1a to dbd8191 Compare June 5, 2024 17:02

dstolfa mentioned this pull request Jun 5, 2024

Guard -D_LIBUNWIND_SANDBOX_OTYPES behind an aarch64 check CTSRD-CHERI/cheribsd#2107

Merged

jrtc27 reviewed Jun 7, 2024

View reviewed changes

dstolfa force-pushed the cppexcept branch 3 times, most recently from cf0ac7b to 6643eaf Compare June 10, 2024 15:29

jrtc27 reviewed Jun 10, 2024

View reviewed changes

dstolfa force-pushed the cppexcept branch from 6643eaf to 1ce449d Compare June 11, 2024 11:40

dpgao reviewed Jun 11, 2024

View reviewed changes

libunwind/src/CompartmentInfo.hpp Outdated Show resolved Hide resolved

libunwind/src/DwarfInstructions.hpp Outdated Show resolved Hide resolved

libunwind/src/Registers.hpp Outdated Show resolved Hide resolved

dstolfa force-pushed the cppexcept branch from 1ce449d to c05c145 Compare June 11, 2024 14:34

jrtc27 reviewed Jun 11, 2024

View reviewed changes

dstolfa force-pushed the cppexcept branch 2 times, most recently from 4996e8c to d5780f4 Compare June 12, 2024 15:04

jrtc27 reviewed Jun 12, 2024

View reviewed changes

dstolfa force-pushed the cppexcept branch from d5780f4 to b5ea76f Compare June 13, 2024 12:21

jrtc27 approved these changes Jun 13, 2024

View reviewed changes

dstolfa force-pushed the cppexcept branch from b5ea76f to 5dbaf28 Compare June 13, 2024 17:47

dstolfa merged commit dd1a970 into dev Jun 13, 2024
5 of 6 checks passed

dstolfa deleted the cppexcept branch June 13, 2024 19:18

		@@ -703,6 +703,25 @@ Lnovec:

		#elif defined(__aarch64__)

		#if defined(__CHERI_PURE_CAPABILITY__)

[libunwind] Support rtld-c18n as the runtime linker. #731

[libunwind] Support rtld-c18n as the runtime linker. #731

Conversation

dstolfa commented Mar 4, 2024 • edited Loading

dstolfa commented Mar 18, 2024

jrtc27 commented Mar 18, 2024

dstolfa commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstolfa commented Mar 26, 2024

dstolfa commented Mar 26, 2024

bsdjhb left a comment

Choose a reason for hiding this comment

dstolfa commented Apr 29, 2024

dstolfa commented Jun 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstolfa Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstolfa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstolfa Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstolfa Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 commented Jun 13, 2024

dstolfa commented Mar 4, 2024 •

edited

Loading

dstolfa Jun 4, 2024 •

edited

Loading

dstolfa Jun 4, 2024 •

edited

Loading

dstolfa Jun 12, 2024 •

edited

Loading