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

dpgao · 2024-06-20T10:41:32Z

See also #2122 for a draft of this PR.

_rtld_unw_getcontext_epilogue is removed because when c18n is not enabled, the trusted stack field of the context is unused anyway, so there's no need to fill it.

The assembly stub in _rtld_unw_setcontext is removed because _rtld_unw_setcontext is now expected to be called before all registers are restored, so it does not need to restore any register anymore.

dstolfa · 2024-06-20T14:26:10Z

This makes changes in setjmp/longjmp as well and the commit message should probably reflect that.

bsdjhb

Certainly some text in the commit log explaining why setjmp/longjmp are changing would be good.

bsdjhb · 2024-07-01T18:49:22Z

lib/libc/aarch64/gen/_setjmp.S

-#if defined(__CHERI_PURE_CAPABILITY__) && defined(RTLD_SANDBOX)
-	mov	c2, c8
-#else
+#if !defined(__CHERI_PURE_CAPABILITY__) || !defined(RTLD_SANDBOX)


I find it easier to read as !(purecap && sandbox) rather than !purecap || !sandbox.

Also, is RTLD_SANDBOX ever true for !purecap? That is, can we simplify some of the #ifdef's to #ifdef RTLD_SANDBOX or #ifndef RTLD_SANDBOX?

Also, is RTLD_SANDBOX ever true for !purecap? That is, can we simplify some of the #ifdef's to #ifdef RTLD_SANDBOX or #ifndef RTLD_SANDBOX?

The purecap check is indeed redundant. I'll do a find-and-replace once the libsys is merged. (libsys renames RTLD_SANDBOX to CHERI_LIB_C18N.)

@bsdjhb I've removed the checks for __CHERI_PURE_CAPABILITY__ in #2200.

Security fixes: CTSRD-CHERI#2135 rar: Fix OOB in rar e8 filter (CVE-2024-26256) CTSRD-CHERI#2145 zip: Fix out of boundary access Important bugfixes: CTSRD-CHERI#2131 7zip: Limit amount of properties CTSRD-CHERI#2110 bsdtar: Fix error handling around strtol() usages CTSRD-CHERI#2116 passphrase: Never allow empty passwords CTSRD-CHERI#2124 rar: Fix "File CRC Error" when extracting specific rar4 archives CTSRD-CHERI#2123 xar: Avoid infinite link loop CTSRD-CHERI#2108 zip: Update AppleDouble support for directories CTSRD-CHERI#2071 zstd: Implement core detection Obained from: libarchive Libarchive commit: 313aa1fa10b657de791e3202c168a6c833bc3543

Libarchive 3.7.4 + three fixes from master Security fixes: CTSRD-CHERI#2135 rar: Fix OOB in rar e8 filter (CVE-2024-26256) CTSRD-CHERI#2145 zip: Fix out of boundary access CTSRD-CHERI#2148 rar: Fix OOB in rar delta filter CTSRD-CHERI#2149 rar: Fix OOB in rar audio filter Important bugfixes: CTSRD-CHERI#2131 7zip: Limit amount of properties CTSRD-CHERI#2110 bsdtar: Fix error handling around strtol() usages CTSRD-CHERI#2116 passphrase: Never allow empty passwords CTSRD-CHERI#2124 rar: Fix "File CRC Error" when extracting specific rar4 archives CTSRD-CHERI#2123 xar: Avoid infinite link loop CTSRD-CHERI#2150 xar: Fix another infinite loop and expat error handling CTSRD-CHERI#2108 zip: Update AppleDouble support for directories CTSRD-CHERI#2071 zstd: Implement core detectiongit PR: 278588 (exp-run) MFC after: 1 day

dstolfa

Builds & runs fine, similar to CTSRD-CHERI/llvm-project#740 just a few nits.

libexec/rtld-elf/rtld_c18n.h

jrtc27 · 2024-08-27T20:52:19Z

libexec/rtld-elf/rtld_c18n.c


-static void *
-unwind_cursor(void)
+bool c18n_is_enabled(void);


These should probably be in sys/sys/link_elf.h with dl_iterate_phdr and friends.

Yes. Although I guess it would require renaming struct trusted_frame etc. to have a dl_ prefix?

Hmmm, since a struct trusted_frame * only ever leaves RTLD as a sealed capability, a void * should be sufficient.

libexec/rtld-elf/rtld_c18n.c

jrtc27 · 2024-08-27T21:19:42Z

lib/libc/aarch64/gen/_setjmp.S

-#ifdef CHERI_LIB_C18N
-	mov	c2, c8
-#else
+#ifndef CHERI_LIB_C18N
 	mov	REGN(sp), REG(8)


What's wrong with doing this for c18n? I'd like to see as much of libc etc de-ifdef'ed as eventually I'd like to see lib/c18n killed off, just as libunwind doesn't need two copies.

jrtc27 · 2024-08-27T21:36:21Z

libexec/rtld-elf/rtld_c18n.h

@@ -217,8 +259,8 @@ func_sig_legal(struct func_sig sig)
 /*
 * This macro can only be used in a function directly invoked by a trampoline.
 */
-#define	c18n_return_address()						\
-	(C18N_ENABLED ? get_trusted_stk()->pc : __builtin_return_address(0))
+#define	c18n_return_address()	(C18N_ENABLED ?				\


There should probably be a non-c18n fallback implementation of this so you don't need to ifdef every use.

Will do in another PR

jrtc27 · 2024-08-27T21:47:04Z

libexec/rtld-elf/rtld_c18n.c

@@ -936,13 +913,17 @@ unwind_stack(struct jmp_args ret, void *rcsp, struct trusted_frame *target)
 	struct trusted_frame *cur, *tf;
 	sigset_t nset, oset;

+	if (!C18N_ENABLED)
+		return (ret);
+
 	/*
 	 * Make the function re-entrant by blocking all signals.
 	 */
 	SIGFILLSET(nset);
 	sigprocmask(SIG_SETMASK, &nset, &oset);


Could expose functions from rtld_lock.c that use sigfastblock when available, saves having to do syscalls here for single-threaded applications. In theory one could also register a per-thread mask just for c18n to use, given rtld unregisters its locking one when a thread is created. But not for this PR.

Strictly speaking, using >= to compare pointers to different trusted frames is undefined behaviour.

dpgao · 2024-08-28T17:08:17Z

@jrtc27 I've changed how setjmp/longjmp invokes the RTLD's unwinding APIs such that it is very simple to remove the ifdefs in a future PR (essentially we just need a noop ASM stub like in libunwind). I also added dl_ prefixes to the APIs and exported them in link_elf.h.

dpgao · 2024-08-29T20:33:46Z

The dl_* functions now use the normal ABI and CTSRD-CHERI/llvm-project#740 has been updated accordingly.

jrtc27 · 2024-08-29T20:38:30Z

Can we keep the old interfaces implemented as compatibility shims so upgrades work when c18n is enabled?

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. The unified unwinding APIs are declared as dl_c18n_* functions in link.h. setjmp/longjmp have been updated to use them. The previous API has been retained for compatibility with old libunwind.

dpgao · 2024-08-30T14:17:24Z

Can we keep the old interfaces implemented as compatibility shims so upgrades work when c18n is enabled?

It seems that preserving the old interfaces implies retaining much of the old implementation as well (including the assembly stubs), but that's fine.

dpgao mentioned this pull request Jun 20, 2024

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

Closed

bsdjhb reviewed Jul 1, 2024

View reviewed changes

dpgao force-pushed the c18n-unw-real branch from 45c9be3 to ff2600d Compare July 18, 2024 14:41

dpgao force-pushed the c18n-unw-real branch from ff2600d to 3b76a6e Compare August 22, 2024 19:27

dpgao requested review from jrtc27 and dstolfa August 22, 2024 19:46

dstolfa reviewed Aug 24, 2024

View reviewed changes

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

libexec/rtld-elf/rtld_c18n.h Show resolved Hide resolved

jrtc27 reviewed Aug 27, 2024

View reviewed changes

c18n: Compare pointers by converting them to integers first

53f014c

Strictly speaking, using >= to compare pointers to different trusted frames is undefined behaviour.

dpgao force-pushed the c18n-unw-real branch from 3b76a6e to 530305d Compare August 28, 2024 16:58

dpgao requested review from jrtc27 and dstolfa August 28, 2024 17:10

dpgao force-pushed the c18n-unw-real branch from 530305d to 7a0f720 Compare August 29, 2024 20:28

dpgao force-pushed the c18n-unw-real branch from 7a0f720 to f0c2c5d Compare August 30, 2024 10:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dpgao commented Jun 20, 2024

dstolfa commented Jun 20, 2024

bsdjhb left a comment

bsdjhb Jul 1, 2024

bsdjhb Jul 1, 2024 •

edited by jrtc27

Loading

dpgao Jul 19, 2024

dpgao Aug 22, 2024

dstolfa left a comment

jrtc27 Aug 27, 2024

dpgao Aug 28, 2024

dpgao Aug 28, 2024 •

edited

Loading

jrtc27 Aug 27, 2024

jrtc27 Aug 27, 2024

dpgao Aug 28, 2024

jrtc27 Aug 27, 2024

dpgao commented Aug 28, 2024

dpgao commented Aug 29, 2024

jrtc27 commented Aug 29, 2024

dpgao commented Aug 30, 2024

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

Are you sure you want to change the base?

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

Conversation

dpgao commented Jun 20, 2024

dstolfa commented Jun 20, 2024

bsdjhb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdjhb Jul 1, 2024 • edited by jrtc27 Loading

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

dpgao Aug 28, 2024 • edited Loading

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

dpgao commented Aug 28, 2024

dpgao commented Aug 29, 2024

jrtc27 commented Aug 29, 2024

dpgao commented Aug 30, 2024

bsdjhb Jul 1, 2024 •

edited by jrtc27

Loading

dpgao Aug 28, 2024 •

edited

Loading