-
Notifications
You must be signed in to change notification settings - Fork 60
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
Support C++ exceptions for rtld-c18n using otypes. #2003
base: dev
Are you sure you want to change the base?
Conversation
f2f29dd
to
5f265bf
Compare
return false; | ||
} | ||
|
||
#ifdef _LIBUNWIND_SANDBOX_OTYPES | ||
inline uintcap_t | ||
Registers_arm64::getSealedExecutiveStack(uintcap_t sealer) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not seem to be used anywhere.
@@ -1970,7 +2015,14 @@ inline void Registers_arm64::setRegister(int regNum, uintptr_t value) { | |||
#ifdef __CHERI_PURE_CAPABILITY__ | |||
else if ((regNum >= UNW_ARM64_C0) && (regNum <= UNW_ARM64_C31)) | |||
_registers.__x[regNum - UNW_ARM64_C0] = value; | |||
#endif | |||
#ifdef _LIBUNWIND_SANDBOX_OTYPES | |||
else if (regNum == UNW_ARM64_ECSP) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should getRegister
also support this new regNum?
ldr c2, sealer_unwbuf | ||
ldr x10, [csp] | ||
scvalue c1, csp, x10 | ||
cseal c1, c1, c2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this is the only place where sealer_unwbuf
is used in RTLD. Would it be a good idea to move sealer_unwbuf
to libunwind
and let it perform this cseal
?
We can use linkage policy to ensure that only libunwind
can call _rtld_unw_getcontext
.
capability_t getCapability(pint_t addr) { return get<capability_t>(addr); } | ||
#if defined(__CHERI_PURE_CAPABILITY__) && defined(_LIBUNWIND_SANDBOX_OTYPES) | ||
static uintcap_t getUnwindSealer(); | ||
capability_t getSealedCapability(pint_t addr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not seem to be used anywhere.
After some discussion with @dpgao, a few issues were identified:
|
5f265bf
to
52d9d7f
Compare
Updated the review:
Presently, there are some questions about what the name of the defines should really be, but this seems okay for now. There are also failing libunwind tests on Morello, but perhaps that is a separate PR as they were failing to begin with. This is still not properly tested outside of Morello and the update is mainly to get some high-level discussion going. |
52d9d7f
to
2005c18
Compare
2005c18
to
353318d
Compare
Is there a design doc for this? A few things are not clear to me:
Presumably the exception object and its type information are trusted. This causes some problems because the throwing compartment is the one that initialises the object and provides the pointer to the destructor. If the object type is provided by another library then it's possible for the throwing library to construct an arbitrary object that the destructor will run on. This is basically a COOP gadget that will run in another compartment's context with the program in a state that cannot be reached by normal control flow. The security implications of this are not obvious to me. It may be no worse than calling destructors on local objects, it may be a compartment escape. |
Thanks for the feedback!
Not yet, we are still going through the design and trying to figure out what the right way to approach the problem is. I took this design as a starting point, effectively opting to treat libunwind as a sort of a TCB, but it is by no means something I am convinced is "the right way" to do things at the moment. I'm waiting on @dpgao to land a change to make unwinding easier at which point the whole hash table bit should go away since we will no longer need to maintain any state there. After that, I'll write down the design document and go through the design in more scrutiny.
My information leakage concerns with this design have to do with the register context, notably callee-saved registers, restricted stack pointers from other compartments and even the executive stack pointer could easily leak out of the libunwind boundary and be accessible on the caller stack. The "hardening" happens by sealing anything that is a pointer and is not sealed already (and thus is unable to seal sentries with an otype), but I am fairly certain there are other security concerns here that we'll have to address.
Which one in particular? If you mean the simple hash map allocated as a part of #ifdef _LIBUNWIND_SANDBOX_HARDENED
capability_t sealer = addressSpace.getUnwindSealer();
if (sealer != addressSpace.to_capability_t(-1))
stackTable = __builtin_cheri_seal(stackTable, sealer);
#endif However when #2061 lands, this entire hash table should no longer be necessary as we won't need to read and write to the bottom of the restricted stack anymore. Perhaps you are talking about a different heap allocation and I'm misunderstanding which one?
This is very likely to be a problem and needs to be thought about further than I have at this stage. The attacker should not be able to corrupt any of the sealed registers themselves, but I'm not quite sure how to protect the context itself seeing as it lives on the caller stack.
Agreed. For some background, we thought that libunwind support was necessary to get the desktop stack running properly for a demo that is due next week (and we still aren't sure that it isn't) so the goal was simply to get it working as fast as possible in an experimental state. I took the view of isolating the changes to libunwind as much as possible, but this design is probably a far cry from the thing we want to eventually have.
I'm not sure which destructor you mean here, but if it's the exception destructor I think (@dpgao can confirm this) that it runs in the same compartment that libcxxrt is. I believe that Dapeng was looking at handling function pointers in the near-ish future, at which point it should jump through the c18n rtld and be called in a different one (probably with supporting code on the libcxxrt end...?).
Agreed, and I would love to have a more detailed discussion about pretty much all of the above if you are available some time (ideally with at least @dpgao also being present as he knows the rtld bits in detail). Even though this "works" it's just a starting point and likely requires more thought to claim any kind of security. If this does land for the demo, it would be have to be documented as experimental with a disclaimer on security. FWIW, one design point I'm interested in is handling exceptions by having each compartment unwind itself up to the boundary and then re-raising the exception in the next compartment. It's unclear to me at this point how it would integrate with libunwind's public APIs, which is part of the reason why I went for this sort of design as a starting point. |
353318d
to
cbd89dd
Compare
Update the code to remove the hash table which is no longer necessary. |
43a348e
to
5e60df9
Compare
This commit pulls out the functionality necessary to implement stack unwinding in rtld into macros and implements longjmp in terms of them. Using these macros, this commit implements the functionality necessary to support exception handling for libunwind. Additionally, it adds a new otype which is reserved for the unwinding library to use.
5e60df9
to
0124316
Compare
Instead, use the get_trusted_frame macro to obtain the trusted frame in C.
0124316
to
61813fe
Compare
This commit also fixes the missing unw_getcontext_unsealed in trusted symbols and moves libunwind symbols closer to the setjmp/longjmp ones.
61813fe
to
cba61f2
Compare
Opening this to mainly look for feedback, this should not be merged, especially not into CheriBSD directly. The reason the PR is here is so that the code can easily be built for testing. This approach is similar to the approach taken by @dpgao and the differences are summarized below.
This PR implements an initial version of DWARF unwinding for Morello with the c18n runtime linker. It implementation uses otypes, and therefore might not be compatible with RISC-V.
This implementation is under
#ifdef _LIBUNWIND_SANDBOX_OTYPES
, as we will likely want to explore different designs for this.Since libunwind's
unw_step()
is a part of the public API, adding a new return code when a compartment boundary is encountered is not feasible as it would break third party consumers. Furthermore, I have tried to be careful about introducing any compile-time ABI changes outside of ones under__CHERI_PURE_CAPABILITY__
so that we don't need a secondarylibgcc_s
for c18n in CheriBSD. This all seems to work and doesn't seem to break any third party software in my testing, but there are probably still some edge cases to catch.Because libunwind has to be thread-safe and its main approach to doing that is using a context, the executive stack pointer was placed into the context. However, the pointer never leaves libunwind without being sealed. This is also true for all the frame and callee-saved registers that are not sealed using otypes or sentries. Unfortunately this still means that we might leak sentries in the context, but I do not have a good way of addressing that right now.
We still call into the rtld to fetch and restore the executive stack pointer, but I don't see a way around this using this kind of design. Furthermore, the CHERI-specific defines are mostly there as a hack and should probably not live in libunwind.