libunwind: Simplify _unw_{get,set}context under c18n

[dpgao]

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.

[dstolfa]

This works fine on my end, but wait for @jrtc27 to review as well.

[jrtc27]

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.

[jrtc27]

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".

[dpgao]

How about __unw_c18n_setcontext and __unw_c18n_getcontext?

[jrtc27]

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.

[dpgao]

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)

_rtld_longjmp(uintptr_t ret, void *rcsp, void *csp)
{
	return (unwind_stack(ret, rcsp, cheri_unseal(csp, sealer_jmpbuf)));
}

uintptr_t
_rtld_unw_setcontext(uintptr_t ret, void *rcsp, void *csp)
{
	if (C18N_ENABLED)
		ret = unwind_stack(ret, rcsp, cheri_unseal(csp, sealer_unwbuf));
	return (ret);
}

[dstolfa]

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 if (C18N_ENABLED) check. I'm not sure how setjmp/longjmp work now, but I assume it wouldn't do any harm to add that check to them as well?

[dpgao]

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 if (C18N_ENABLED) check.

Teaching RTLD about the internal layout of jmpbufs and libunwind structures seems too much of a cost to pay to unify the API.

I'm not sure how setjmp/longjmp work now, but I assume it wouldn't do any harm to add that check to them as well?

That's true.

[jrtc27]

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).

[dstolfa]

Teaching RTLD about the internal layout of jmpbufs and libunwind structures seems too much of a cost to pay to unify the API.

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 setCapabilityRegister & co.

[dpgao]

Teaching RTLD about the internal layout of jmpbufs and libunwind structures seems too much of a cost to pay to unify the API.

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 setCapabilityRegister & co.

Oh, I see what you mean now. Yes, delegating the task of restoreRegistersFromSandbox to RTLD is a good idea. And we no longer need to expose the sealer to libunwind, but instead need an API for libunwind to query whether c18n is enabled.

I'll implement that and push a patch to CTSRD-CHERI/cheribsd#2122.

[dstolfa]

I ran the tests and it works for me. A few nits in the code, but overall I'm fine with this change.

[jrtc27]

Happy to see the cumulative diff significantly reducing and becoming much more additive instead of invasive. Some comments to polish it up.

[dpgao]

@jrtc27 I removed the LIBUNWIND_CHERI_C18N_SUPPORT option so that c18n support code is always compiled on purecap Morello. I also moved all the unwinding logic into one function in struct CompartmentInfo which is now called in UnwindCursor.

[jrtc27]

What's this for?

[dpgao]

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.

[jrtc27]

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.

[jrtc27]

Also, why isn't the struct declared by system headers then?..

[dpgao]

Hmmm maybe it's easier to just hard-code everything here in libunwind then.

[jrtc27]

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:

1. Follow the ABI of a normal function call

2. 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.

[dpgao]

The latest commit now does 1.