Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c18n: Support for C++ exceptions #688

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dpgao
Copy link

@dpgao dpgao commented Mar 18, 2023

Modifies libunwind to correctly unwind through the trusted stack when library-based compartmentalisation is enabled.

Modifies libunwind to correctly unwind through the trusted stack
when library-based compartmentalisation is enabled.
@dpgao dpgao requested review from jrtc27, brooksdavis and bsdjhb March 18, 2023 23:50
Copy link
Member

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using RTLD_SANDBOX feels wrong, especially when libunwind.h is a public interface. I also have ABI concerns about changing the libunwind context; I think that's public ABI...

@@ -1969,6 +1987,8 @@ inline bool Registers_arm64::validCapabilityRegister(int regNum) const {
return true;
if ((regNum >= UNW_ARM64_C0) && (regNum <= UNW_ARM64_C31))
return true;
if (regNum == UNW_ARM64_CCSP)
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true for !RTLD_SANDBOX

@@ -21,6 +21,9 @@
#include "DwarfParser.hpp"
#include "config.h"

#ifdef __CHERI_PURE_CAPABILITY__
#include <cheri/cheric.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just use the __builtin_cheri_foo directly; so far libunwind doesn't use any CHERI headers. Definitely not cheri/cheric.h though, that's CheriBSD-specific.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should I include to use the CHERI_PERM_EXECUTIVE below?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be an equivalent macro pre-defined by the compiler itself, something like __ARM_CAP_PERM_EXECUTIVE__

@@ -824,6 +824,13 @@ DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext)

#elif defined(__aarch64__)

#if defined(__CHERI_PURE_CAPABILITY__) && defined(RTLD_SANDBOX)
DEFINE_LIBUNWIND_FUNCTION(__rtld_unw_getcontext)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly hooking into rtld from libunwind feels pretty gross...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the same for libc and libthr. Is libunwind any different?

Hooking into rtld seems to be the only way to access Executive mode registers from libunwind...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because they’re developed by and for FreeBSD, rather than being a third-party library, and they already have such hooks.

@dpgao
Copy link
Author

dpgao commented Mar 20, 2023

Using RTLD_SANDBOX feels wrong, especially when libunwind.h is a public interface. I also have ABI concerns about changing the libunwind context; I think that's public ABI...

@jrtc27 The libunwind context is indeed public ABI, as it is used by the personality function in libcxxrt. But are there any other libraries that use it?

More generally, do you have any suggestions on how best to integrate c18n-compatible unwinding into libunwind?

@jrtc27
Copy link
Member

jrtc27 commented Mar 20, 2023

Using RTLD_SANDBOX feels wrong, especially when libunwind.h is a public interface. I also have ABI concerns about changing the libunwind context; I think that's public ABI...

@jrtc27 The libunwind context is indeed public ABI, as it is used by the personality function in libcxxrt. But are there any other libraries that use it?

More generally, do you have any suggestions on how best to integrate c18n-compatible unwinding into libunwind?

I don't, I just remember that GDB got rather unhappy when we broke the libunwind context ABI for RISC-V by adding DDC for hybrid (which we later reverted for various reasons) and therefore am pretty apprehensive about this.

@jrtc27
Copy link
Member

jrtc27 commented Mar 20, 2023

Exposing CCSP is pretty dodgy anyway, doesn't this effectively give restricted mode code the ability to bypass the protection and get at the executive mode stack capability, which in turn gets them access to other compartments?

@dpgao
Copy link
Author

dpgao commented Mar 20, 2023

Using RTLD_SANDBOX feels wrong, especially when libunwind.h is a public interface. I also have ABI concerns about changing the libunwind context; I think that's public ABI...

@jrtc27 The libunwind context is indeed public ABI, as it is used by the personality function in libcxxrt. But are there any other libraries that use it?
More generally, do you have any suggestions on how best to integrate c18n-compatible unwinding into libunwind?

I don't, I just remember that GDB got rather unhappy when we broke the libunwind context ABI for RISC-V by adding DDC for hybrid (which we later reverted for various reasons) and therefore am pretty apprehensive about this.

I see. The RTLD_SANDBOX enabled libunwind will be a seperate binary from the non-sandboxed original, which means that non-compartmentalised programs will not be affected. Plus, GDB doesn't work with c18n already, so work needs to be done to make it compatible anyway.

Does this lessen the problem in anyway?

@jrtc27
Copy link
Member

jrtc27 commented Mar 20, 2023

Using RTLD_SANDBOX feels wrong, especially when libunwind.h is a public interface. I also have ABI concerns about changing the libunwind context; I think that's public ABI...

@jrtc27 The libunwind context is indeed public ABI, as it is used by the personality function in libcxxrt. But are there any other libraries that use it?
More generally, do you have any suggestions on how best to integrate c18n-compatible unwinding into libunwind?

I don't, I just remember that GDB got rather unhappy when we broke the libunwind context ABI for RISC-V by adding DDC for hybrid (which we later reverted for various reasons) and therefore am pretty apprehensive about this.

I see. The RTLD_SANDBOX enabled libunwind will be a seperate binary from the non-sandboxed original, which means that non-compartmentalised programs will not be affected. Plus, GDB doesn't work with c18n already, so work needs to be done to make it compatible anyway.

Does this lessen the problem in anyway?

GDB's just an example, it's surely not the only thing. And just because nothing you're running cares about the ABI doesn't mean that it's ok to just go and deliberately break the ABI.

@dpgao
Copy link
Author

dpgao commented Mar 20, 2023

Exposing CCSP is pretty dodgy anyway, doesn't this effectively give restricted mode code the ability to bypass the protection and get at the executive mode stack capability, which in turn gets them access to other compartments?

This is indeed a risk, but I think it is possible to make this scheme secure by only allowing libunwind and the personality function to have access to the context struct. For example, we can forbid other libraries from linking against functions like unw_getcontext that can lead to leakage.

@dpgao
Copy link
Author

dpgao commented Mar 20, 2023

@jrtc27 The libunwind context is indeed public ABI, as it is used by the personality function in libcxxrt. But are there any other libraries that use it?

GDB's just an example, it's surely not the only thing. And just because nothing you're running cares about the ABI doesn't mean that it's ok to just go and deliberately break the ABI.

That's why I'm wondering if you are aware of any other libraries that rely on this ABI. If there are very few (if any at all other than libcxxrt) than we should be safe.

@jrtc27
Copy link
Member

jrtc27 commented Mar 20, 2023

Exposing CCSP is pretty dodgy anyway, doesn't this effectively give restricted mode code the ability to bypass the protection and get at the executive mode stack capability, which in turn gets them access to other compartments?

This is indeed a risk, but I think it is possible to make this scheme secure by only allowing libunwind and the personality function to have access to the context struct. For example, we can forbid other libraries from linking against functions like unw_getcontext that can lead to leakage.

https://codesearch.debian.net/search?q=unw_getcontext&literal=1

Good luck with that

@jrtc27
Copy link
Member

jrtc27 commented Mar 20, 2023

I think in reality what you need is a very different design; you need libunwind to do the unwinding within a compartment, but at the boundary you need to request that rtld effectively catch and re-raise (maybe that's even how it should be implemented) the exception, or some other privileged executive thing. You cannot use an unw_context_t supplied by an unprivileged entity for things outside the compartment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants