[RFC] c18n: New {get,set}context APIs for libunwind #2122

dpgao · 2024-06-18T15:50:28Z

This PR is not intended to be merged but merely to collect feedback.

I changed _unw_setcontext to call _rtld_unw_setcontext before restoring the registers. Also, _unw_setcontext is now marked as a trusted function so no trampoline is involved when entering and exiting it. This ensures that no register is clobbered when setting the context.

Assembly stubs for _rtld_unw_{get,set}context in libunwind have been removed. Instead, we make calls to these functions behave like no-ops if they are not defined by RTLD.

libexec/rtld-elf/rtld_c18n.c

dpgao · 2024-06-18T18:32:08Z

Not for libunwind, but for longjmp, which takes two arguments. Edit: Ignore this comment. c1 does not need to be preserved here.

…

On 18 Jun 2024, at 18:53, Domagoj Stolfa ***@***.***> wrote: @dstolfa commented on this pull request. In libexec/rtld-elf/rtld_c18n.c: > @@ -1009,9 +999,11 @@ _rtld_longjmp(struct jmp_args ret, void *rcsp, void **buf) } struct jmp_args -_rtld_unw_setcontext_impl(struct jmp_args ret, void *rcsp, void **buf) +_rtld_unw_setcontext(struct jmp_args ret, void *rcsp, void **buf) While I don't think it does any harm, do we need to preserve c1 here? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

contrib/subrepo-cheri-libunwind/src/UnwindRegistersRestore.S

libexec/rtld-elf/rtld_c18n_policy.txt

dstolfa · 2024-06-20T09:11:54Z

This looks good to me now, but you should open a PR on https://github.com/CTSRD-CHERI/llvm-project/ and a separate one on this repo for the rtld changes.

dpgao · 2024-06-20T10:44:49Z

This looks good to me now, but you should open a PR on https://github.com/CTSRD-CHERI/llvm-project/ and a separate one on this repo for the rtld changes.

I've created #2123 and CTSRD-CHERI/llvm-project#740. Note that I no longer define the stub no-op assembly functions in-line because that might be difficult to read.

dpgao · 2024-06-21T16:03:48Z

@jrtc27 @dstolfa I've implemented what we discussed in CTSRD-CHERI/llvm-project#740. CompartmentInfo has been repurposed as a delegate that calls RTLD APIs to read the trusted stack and update the unwind cursor.

dstolfa · 2024-06-24T13:17:01Z

@jrtc27 @dstolfa I've implemented what we discussed in CTSRD-CHERI/llvm-project#740. CompartmentInfo has been repurposed as a delegate that calls RTLD APIs to read the trusted stack and update the unwind cursor.

I pulled the changes down to give them a test, but they don't build. Building CheriBSD on a Morello box fails for lib32:

>>> stage 4.3.1: building lib32 shim libraries
--------------------------------------------------------------
make[2]: "/home/ds815/cheribsd-dev/Makefile.inc1" line 970: META_MODE: Rebuilding host tools due to ABI breakage in __FreeBSD_version 1200031.
** edit: HAVE_TCGETATTR 1
** edit: HAVE_TERMIOS_H 1
** edit: HAVE_TERMIO_H 0
** edit: BROKEN_LINKER 0
In file included from /home/ds815/cheribsd-dev/contrib/subrepo-cheri-libunwind/src/libunwind.cpp:31:
/home/ds815/cheribsd-dev/contrib/subrepo-cheri-libunwind/src/CompartmentInfo.hpp:31:3: error: "LIBUNWIND_CHERI_C18N_SUPPORT is not supported on this target"
# error "LIBUNWIND_CHERI_C18N_SUPPORT is not supported on this target"
  ^
/home/ds815/cheribsd-dev/contrib/subrepo-cheri-libunwind/src/CompartmentInfo.hpp:59:26: error: variable has incomplete type 'struct compart_state'
    struct compart_state state;
                         ^
/home/ds815/cheribsd-dev/contrib/subrepo-cheri-libunwind/src/CompartmentInfo.hpp:42:29: note: forward declaration of 'libunwind::compart_state'
c18n_pop_trusted_stk(struct compart_state *, struct trusted_frame *);
                            ^
2 errors generated.
*** [libunwind.o] Error code 1

While building for CHERI-RISC-V fails almost immediately:

In file included from /local/scratch/ds815/cheri/cheribsd/contrib/subrepo-cheri-libunwind/src/libunwind.cpp:31:
/local/scratch/ds815/cheri/cheribsd/contrib/subrepo-cheri-libunwind/src/CompartmentInfo.hpp:31:3: error: "LIBUNWIND_CHERI_C18N_SUPPORT is not supported on this target"
# error "LIBUNWIND_CHERI_C18N_SUPPORT is not supported on this target"
  ^
/local/scratch/ds815/cheri/cheribsd/contrib/subrepo-cheri-libunwind/src/CompartmentInfo.hpp:59:26: error: variable has incomplete type 'struct compart_state'
    struct compart_state state;
                         ^
/local/scratch/ds815/cheri/cheribsd/contrib/subrepo-cheri-libunwind/src/CompartmentInfo.hpp:42:29: note: forward declaration of 'libunwind::compart_state'
c18n_pop_trusted_stk(struct compart_state *, struct trusted_frame *);
                            ^

dpgao · 2024-06-25T09:43:15Z

I pulled the changes down to give them a test, but they don't build.

@dstolfa The build issue has been resolved now and CI is passing. Could you give it another try?

dstolfa

I've run it on Morello and it seems to work fine. CHERI-RISC-V also builds fine, but I haven't yet tested isolated libunwind builds. Will report back once I do.

contrib/subrepo-cheri-libunwind/src/UnwindRegistersSave.S

libexec/rtld-elf/rtld_c18n.h

libexec/rtld-elf/rtld_c18n.c

dstolfa · 2024-06-25T17:02:54Z

libexec/rtld-elf/rtld_c18n.h

+	/*
+	 * Zeros
+	 */
+	uint16_t zeros;


Not specifically related to this diff, but what is "zeros" and why is it here?

This is just padding space that must be filled with zeros.

@jrtc27 So that when you do a wide load (e.g. a capability load) from the caller field, you can extract the caller for free by using a narrower register (i.e. the w register on Morello).

I will document it.

libexec/rtld-elf/rtld_c18n.c

dstolfa · 2024-06-25T17:09:37Z

contrib/subrepo-cheri-libunwind/src/CompartmentInfo.hpp

+    return c18n_is_tramp(pc, (struct trusted_frame *)tf);
+  }
+
+  static pint_t getC18NState(R &newRegisters, pint_t tf) {


The use of "get" here feels a little bit weird since it also populates newRegisters. More of a comment than anything else, I'm not sure what name would make the most sense here.

How about fillC18NState?

That sounds fine to me

libexec/rtld-elf/rtld_c18n.c

dstolfa

I've tested on both Morello and CHERI-RISC-V and both seem to work fine. It might make sense to update the individual reviews once you've addressed the dummy stack comments from the other review unless @jrtc27 has other comments about the specific functionality here.

dstolfa · 2024-07-03T11:39:33Z

libexec/rtld-elf/rtld_c18n.c

+	 * Push a dummy trusted frame indicating that the current compartment is
+	 * RTLD.
+	 */
+	*--tf = (struct trusted_frame) {


This seems to be mixing in stuff from #2134 if I'm not mistaken? If so, updating it to reflect @jrtc27's comments from there might make sense? This is more of a general comment for the rtld changes rather than this specific spot.

Yeah I rebased on #2134 to see if everything works together as a whole. This PR remains focused on the unwind-related bits.

The check for benchmark ABI was erroneously added and conflicts with benchmark ABI-only code below.

This is a common pattern and warrants shared code.

The dummy frame indicates that the current compartment is RTLD. Because _rtld_bind may cause domain switches (e.g., via a ifunc resolver which calls a lazily-bound symbol), a frame that correctly identifies the current compartment as RTLD is necessary. The signal handler has also been updated to handle the temporary inconsistency between the untrusted stack and the callee field of the topmost truted frame that result from the changes above.

The current method of popping the topmost trusted frame and restoring it at the end of the function does not correctly identify the current compartment as RTLD. Instead, push a dummy frame that does so.

This prevents a compartment from being able to set its stack to NULL and then trigger a signal that would cause it to be mis-identified as RTLD.

The dummy frame indicates that the current compartment is RTLD. Because _rtld_tlsdesc_dynamic may cause domain transitions (e.g., locking), a frame that correctly identifies the current compartment as RTLD is necessary.

_rtld_unw_getcontext_epilogue is removed because when c18n is not enabled, the trusted stack field of the unwind cursor is unused anyway, so there's no need to fill it. _rtld_unw_setcontext is no longer in assembly because it is now expected to be called before all registers are restored, so it does not need to restore any register. setjmp and longjmp are updated to use the slightly changed API signatures.

Previously, libunwind hard-codes knowledge about the layout of the trusted frame and has read access to the trusted stack. This is fragile and insecure. Now, the task of extracting the relevant registers from the trusted stack is delegated to RTLD, and libunwind no longer has access to the trusted stack. In addition, unify the APIs exposed to setjmp/longjmp and libunwind.

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.

brooksdavis · 2024-07-17T22:19:17Z

Is this superseded by #2123?

dpgao · 2024-07-17T22:32:18Z

Is this superseded by #2123?

Yes indeed. Closing now.

dstolfa · 2024-07-17T23:31:55Z

Could you update the two PRs relating to this with the new version that's in this PR? That way we don't have out of date PRs which supersede this one, but don't actually have the same code.

dpgao · 2024-07-18T14:45:55Z

Could you update the two PRs relating to this with the new version that's in this PR? That way we don't have out of date PRs which supersede this one, but don't actually have the same code.

Done!

dpgao requested review from jrtc27 and dstolfa June 18, 2024 15:50

dstolfa reviewed Jun 18, 2024

View reviewed changes

libexec/rtld-elf/rtld_c18n.c Outdated Show resolved Hide resolved

dstolfa reviewed Jun 19, 2024

View reviewed changes

contrib/subrepo-cheri-libunwind/src/UnwindRegistersRestore.S Outdated Show resolved Hide resolved

contrib/subrepo-cheri-libunwind/src/UnwindRegistersRestore.S Outdated Show resolved Hide resolved

libexec/rtld-elf/rtld_c18n_policy.txt Outdated Show resolved Hide resolved

dpgao force-pushed the c18n-unw branch 2 times, most recently from 88e3fbd to 3af16f4 Compare June 19, 2024 11:55

dpgao requested a review from dstolfa June 19, 2024 20:53

dpgao mentioned this pull request Jun 20, 2024

c18n: New {get,set}context APIs for libunwind #2123

Open

dpgao mentioned this pull request Jun 21, 2024

libunwind: Use new unwinding APIs under c18n CTSRD-CHERI/llvm-project#740

Merged

dpgao force-pushed the c18n-unw branch 3 times, most recently from 8e36b3f to a359b71 Compare June 21, 2024 15:57

dstolfa reviewed Jun 25, 2024

View reviewed changes

dpgao force-pushed the c18n-unw branch 2 times, most recently from d3a0850 to 34d6023 Compare July 1, 2024 16:03

dpgao requested a review from dstolfa July 1, 2024 16:04

dstolfa reviewed Jul 3, 2024

View reviewed changes

dpgao and others added 7 commits July 4, 2024 15:34

cheribsdtest: Add test for cross-object IFUNC invocation

5deb157

c18n: Fix typo in ifdef expression

337b055

The check for benchmark ABI was erroneously added and conflicts with benchmark ABI-only code below.

c18n: Factor out code that push/pop dummy trusted frames

4b13173

This is a common pattern and warrants shared code.

c18n: Reduce platform-specific code in signal handling

510f645

c18n: Improve stack consistency during stack resolution

8cc83d7

The current method of popping the topmost trusted frame and restoring it at the end of the function does not correctly identify the current compartment as RTLD. Instead, push a dummy frame that does so.

c18n: Use a tagged dummy stack for Morello purecap ABI

b3b8d81

This prevents a compartment from being able to set its stack to NULL and then trigger a signal that would cause it to be mis-identified as RTLD.

dpgao added 5 commits July 4, 2024 16:52

c18n: Push dummy frame during _rtld_tlsdesc_dynamic

905deea

The dummy frame indicates that the current compartment is RTLD. Because _rtld_tlsdesc_dynamic may cause domain transitions (e.g., locking), a frame that correctly identifies the current compartment as RTLD is necessary.

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

c9e0215

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.

libunwind: Use APIs exposed by RTLD to unwind the trusted stack

67752c9

dpgao force-pushed the c18n-unw branch from 34d6023 to 67752c9 Compare July 4, 2024 15:53

dpgao closed this Jul 17, 2024

dpgao deleted the c18n-unw branch July 18, 2024 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] c18n: New {get,set}context APIs for libunwind #2122

[RFC] c18n: New {get,set}context APIs for libunwind #2122

dpgao commented Jun 18, 2024

dpgao commented Jun 18, 2024 via email •

edited

Loading

dstolfa commented Jun 20, 2024

dpgao commented Jun 20, 2024

dpgao commented Jun 21, 2024

dstolfa commented Jun 24, 2024

dpgao commented Jun 25, 2024

dstolfa left a comment

dstolfa Jun 25, 2024

dpgao Jul 1, 2024

jrtc27 Jul 1, 2024

dpgao Jul 1, 2024

dpgao Jul 1, 2024

dstolfa Jun 25, 2024

dpgao Jul 1, 2024

dstolfa Jul 1, 2024

dstolfa left a comment

dstolfa Jul 3, 2024

dpgao Jul 3, 2024

brooksdavis commented Jul 17, 2024

dpgao commented Jul 17, 2024

dstolfa commented Jul 17, 2024

dpgao commented Jul 18, 2024

[RFC] c18n: New {get,set}context APIs for libunwind #2122

[RFC] c18n: New {get,set}context APIs for libunwind #2122

Conversation

dpgao commented Jun 18, 2024

dpgao commented Jun 18, 2024 via email • edited Loading

dstolfa commented Jun 20, 2024

dpgao commented Jun 20, 2024

dpgao commented Jun 21, 2024

dstolfa commented Jun 24, 2024

dpgao commented Jun 25, 2024

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

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

brooksdavis commented Jul 17, 2024

dpgao commented Jul 17, 2024

dstolfa commented Jul 17, 2024

dpgao commented Jul 18, 2024

dpgao commented Jun 18, 2024 via email •

edited

Loading