-
Notifications
You must be signed in to change notification settings - Fork 39
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
libunwind: Simplify _unw_{get,set}context under c18n #740
base: dev
Are you sure you want to change the base?
Conversation
This works fine on my end, but wait for @jrtc27 to review as well. |
By using the same name for these unversioned functions, you have a compatibility problem when upgrading from 23.05 to a newer version, as there will be a window within which rtld has been upgraded but not libunwind. This isn't limited to when c18n is enabled (which would make it a non-issue), because part of the restoration process is done by the callee currently even for non-c18n, and so with a new rtld you will no longer have those registers restored which, importantly, includes CSP. C2 and C3 are probably unused enough in practice that you wouldn't notice those during that window. |
Now, having said all that, I had commented before that these functions probably shouldn't be called _rtld_foo, because that's a very FreeBSD-oriented naming scheme. So I think the right approach is to use new, less FreeBSD-y names for them, and the problem will then be avoided. If you enable c18n then things will break unless rtld provides the old names for compatibility, but I think it's fine to just say "don't turn it on when upgrading". |
How about |
I would rather see it be a generic API that both {set,long}jmp and unw{get, set}context use, because at the end of the day they want exactly the same thing. |
RTLD's unwinding code is indeed shared by both *jmp and *context. Separate APIs are provided just because different sealers are used. (See rtld_c18n.c and excerpt below)
|
If we do the register restoring part of libunwind inside rtld, we don't need the sealer to be passed to libunwind and thus could probably use the same sealer. This would allow us to use the same APIs sans the |
Teaching RTLD about the internal layout of jmpbufs and libunwind structures seems too much of a cost to pay to unify the API.
That's true. |
Restoring of registers from the trusted stack (well, really, extraction into some generic “c18n state” struct for the caller to use, libunwind doesn’t want them actually restored whilst unwinding, just recorded in its context). |
Why would we have to teach it about libunwind internals? The idea would be to pass in a buffer into rtld from libunwind (guarded by a policy that only exposes it to libunwind), have rtld fill out the data from the trusted stack that we passed in (which would be sealed using the jmpbuf) and then populate the buffer. rtld would remain unaware about libunwind internals, and libunwind could then use that buffer to |
Oh, I see what you mean now. Yes, delegating the task of I'll implement that and push a patch to CTSRD-CHERI/cheribsd#2122. |
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.
I ran the tests and it works for me. A few nits in the code, but overall I'm fine with this change.
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.
Happy to see the cumulative diff significantly reducing and becoming much more additive instead of invasive. Some comments to polish it up.
@jrtc27 I removed the |
libunwind/src/CompartmentInfo.hpp
Outdated
@@ -13,21 +13,68 @@ | |||
#ifndef __COMPARTMENT_INFO_HPP__ | |||
#define __COMPARTMENT_INFO_HPP__ | |||
|
|||
extern "C" { | |||
|
|||
#include <link.h> |
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.
What's this for?
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.
Since the declarations for dl_c18n_is_tramp
and dl_c18n_pop_trusted_stk
are now in link_elf.h
, I include link.h
to avoid having to re-declare both functions.
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.
Ok, but then you need to detect them in CMake, because otherwise you'll break the build on Linux or current/old versions of CheriBSD.
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.
Also, why isn't the struct declared by system headers then?..
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.
Hmmm maybe it's easier to just hard-code everything here in libunwind then.
@@ -731,8 +712,12 @@ WEAK_ALIAS(__rtld_unw_setcontext, _rtld_unw_setcontext) | |||
.p2align 2 | |||
DEFINE_LIBUNWIND_FUNCTION(__libunwind_Registers_arm64_jumpto) | |||
#ifdef __CHERI_PURE_CAPABILITY__ | |||
ldr c2, [c0, #0x1f0] // Pass the target untrusted stack pointer | |||
ldr c3, [c0, #0x210] // Pass the target trusted stack pointer | |||
bl dl_c18n_unwind_trusted_stk |
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.
I would much rather not have all this magic "pass through an arbitrary set of arguments as the return values so those registers are preserved, according to whatever the caller this is designed for happens to need" and instead do one of:
-
Follow the ABI of a normal function call
-
Use a highly-customised calling convention like TLSDESC does
I don't think any of the callers of these kinds of functions is performance-critical enough to warrant 2.
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.
The latest commit now does 1.
Assembly stubs for _rtld_unw_{get,set}context are no longer needed. Due to the significantly simplified implementation, the LIBUNWIND_CHERI_C18N_SUPPORT option has been removed and c18n support is now included by default for supported architectures (currently Morello only).
Assembly stubs for
_rtld_unw_{get,set}context
are removed. Instead, turn calls to these functions into no-ops when they are not defined by RTLD.