c18n: [DRAFT] Rework implementation to be interrupt-safe #2079

dpgao · 2024-04-06T23:42:04Z

Edit: The c18n statistics part of this PR is a bit stalled, so I pulled out the interrupt-safe changes to #2090 which will hopefully get merged soon.

This PR builds upon #2012 and #2032 and the real content is in the very last commit entitled c18n: Rework implementation to be interrupt-safe. This is not meat to be merged but is a stable implementation needing feedback. I do hope it can be merged in the next release if time permits.

This commit completely refactors the trampoline and how stack switching works. The purecap and benchmark ABI implementations now both use a dedicated register to store the trusted stack (ddc and rddc respectively). This makes the trampolines look identical (modulo register names) on both ABIs. No metadata recording the current top of the stack is stored at the bottom of each compartment's stack. Instead, the stack lookup table now stores that information.

The signal handling mechanism has been rewritten to handle (rare) cases where c18n code, in particular trampolines, is interrupted. All c18n code paths that could be interrupted have been audited and it is believed that they can all be handled correctly, although testing for that is hard.

libexec/rtld-elf/rtld_c18n.c

brooksdavis · 2024-04-19T20:42:06Z

sys/cheri/cheri.h

+struct cheri_c18n_info {
+	uint8_t version;
+	size_t stats_size;
+	struct rtld_c18n_stats * __capability	stats;


Should this be __kerncap or does that complicate coredump support? I don't think only purecap rtld touches it in userspace?

I'm not familiar enough with the mechanisms involved here. Perhaps @rwatson and @bsdjhb can comment?

Non-purecap RTLD wouldn't even have c18n compiled in, so using __kerncap makes no practical difference, if I'm understanding the problem correctly.

Sounds like it should be __kerncap as the annotation will go away once we're purecap-only.

sys/kern/kern_proc.c

brooksdavis · 2024-04-19T20:48:09Z

sys/kern/kern_proc.c

+	struct proc *p;
+	struct cheri_c18n_info info;
+	int error;
+	void *buffer;


If you initialize this to NULL you don't need two labels for the exit path.

sys/sys/imgact.h

brooksdavis · 2024-04-19T20:56:22Z

sys/kern/kern_proc.c

+	}
+
+	buffer = malloc(info.stats_size, M_TEMP, M_WAITOK);
+	n = proc_readmem(curthread, p, (__cheri_addr vm_offset_t)info.stats,


We can't go blindly trusting the address here. At a minimum we need to check the capability or the process could leak secrets with a bad value.

How is such a check performed? Could you point me to an example?

You can use __CAP_CHECK to verify that the data is in range. That macro should likely be altered to require that the capability be unsealed as well as tagged. You should also check that it has load permission.

__CAP_CHECK does require the capability to be tagged, which in this causes it to always fail because info.stats is always untagged.

I don't understand why we need to do this check though. Wouldn't the userspace just leak it's own memory if it sets a bad value?

Why is info.stats untagged? That seems completely wrong.

Causing program secrets to be trivial available by sysctl seems like a bad idea.

It seems that proc_readmem uses the UIO_SYSSPACE flag which does a bcopynocap_c underneath, stripping all tags.

Sounds like we need an _c variant.

I wonder whether we want to extend uio_rw with UIO_READ_CAP UIO_WRITE_CAP variants, so that we can honor it in both uiomove_flags and uiomove_fromphys (and all possible variations) without having to carry around an extra flag. I think when we set the UIO_READ/WRITE we already know whether we expect capabilities to be there and the current scheme should work fine in most cases as we don't preserve capabilities by default.

I do think you want it orthogonal yes. Extending uio_rw is probably the cleanest way. My only worry is if there is any code doing if (uio->uio_rw == UIO_READ) else /* WRITE */ instead of using switch statements. A quick grep does show many, but also lots of KASSERT's that would catch this I think?

sys/sys/elf_common.h

sys/sys/sysctl.h

lib/libprocstat/libprocstat.c

Exposes LD_COMPARTMENT_STATS that exports a set of compartmentalisation-related statistics to a user-specified file.

The trampoline and other parts of RTLD are refactored to be interrupt-safe. The trusted frame is redesigned to allow trampolines to perform tail-calls that do not push a trusted frame. The new design also no longer relies on a region of metadata at the bottom of each compartment's stack.

dpgao · 2024-04-24T09:20:07Z

lib/libprocstat/libprocstat.c

+	/*
+	 * Error handling here is wrong.  If ENOEXEC, really want to print
+	 * output indicating no information, which this function signature
+	 * doesn't currently support.  This is because the process probably
+	 * simply doesn't have c18n in use
+	 */
+	name[0] = CTL_KERN;
+	name[1] = KERN_PROC;
+	name[2] = KERN_PROC_C18N;
+	name[3] = kp->ki_pid;
+	error = sysctl(name, nitems(name), *pp, lenp, NULL, 0);
+	if (error != 0 && errno != ESRCH && errno != EPERM &&
+	    errno != ENOEXEC) {
+		warn("sysctl(kern.proc.c18n)");
+		goto out_free;
+	}
+	if (error != 0)
+		goto out_free;
+	return (0);


@rwatson Do we need to fix the error handling here?

dpgao · 2024-04-24T14:31:38Z

sys/kern/kern_proc.c

+	}
+
+	buffer = malloc(info.stats_size, M_TEMP, M_WAITOK);
+	n = proc_readmem(curthread, p, (__cheri_addr vm_offset_t)info.stats,


__CAP_CHECK does require the capability to be tagged, which in this causes it to always fail because info.stats is always untagged.

I don't understand why we need to do this check though. Wouldn't the userspace just leak it's own memory if it sets a bad value?

dpgao force-pushed the c18n-nobot branch 6 times, most recently from 874a94c to 0ecef41 Compare April 10, 2024 17:01

dpgao changed the title ~~c18n: [WIP] Do not store stack metadata at its bottom~~ c18n: Rework implementation to be interrupt-safe Apr 10, 2024

dpgao force-pushed the c18n-nobot branch 2 times, most recently from 9680cbd to 773358e Compare April 11, 2024 17:14

rwatson mentioned this pull request Apr 17, 2024

c18n: Data corruption when trampolines are interrupted #2077

Open

dpgao mentioned this pull request Apr 17, 2024

[libunwind] Support new frame layout of rtld-c18n CTSRD-CHERI/llvm-project#732

Closed

dpgao force-pushed the c18n-nobot branch 2 times, most recently from e195871 to c0da4f1 Compare April 19, 2024 15:45

brooksdavis reviewed Apr 19, 2024

View reviewed changes

dpgao force-pushed the c18n-nobot branch 2 times, most recently from 66fd8d4 to c73231e Compare April 22, 2024 17:06

dpgao added 3 commits April 23, 2024 20:42

c18n: Do not get trusted stack when c18n is disabled

71bb4ca

c18n: Reallocate trampoline tables with malloc instead of mmap

3ce5b25

c18n: Block all signals when creating a new thread

187bfb5

dpgao force-pushed the c18n-nobot branch 3 times, most recently from 3dd5952 to b4b5479 Compare April 23, 2024 22:32

dpgao and others added 4 commits April 24, 2024 12:10

c18n: Export c18n statistics to file

365ac99

Exposes LD_COMPARTMENT_STATS that exports a set of compartmentalisation-related statistics to a user-specified file.

For demo purposes, procstat support for c18n. Not yet a merge candidate.

a63f476

c18n: Use superpages to store trampolines

8c7be11

dpgao force-pushed the c18n-nobot branch from b4b5479 to 8c7be11 Compare April 24, 2024 14:23

dpgao commented Apr 24, 2024

View reviewed changes

dpgao mentioned this pull request Apr 24, 2024

c18n: Rework implementation to be interrupt-safe #2090

Open

dpgao changed the title ~~c18n: Rework implementation to be interrupt-safe~~ c18n: [DRAFT] Rework implementation to be interrupt-safe Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c18n: [DRAFT] Rework implementation to be interrupt-safe #2079

c18n: [DRAFT] Rework implementation to be interrupt-safe #2079

dpgao commented Apr 6, 2024 •

edited

brooksdavis Apr 19, 2024

dpgao Apr 20, 2024

dpgao Apr 20, 2024

brooksdavis Apr 24, 2024

brooksdavis Apr 19, 2024

brooksdavis Apr 19, 2024

dpgao Apr 19, 2024

brooksdavis Apr 20, 2024

dpgao Apr 24, 2024

brooksdavis Apr 24, 2024

dpgao Apr 24, 2024

brooksdavis Apr 24, 2024

qwattash Apr 24, 2024

bsdjhb Apr 24, 2024

dpgao Apr 24, 2024

dpgao Apr 24, 2024

c18n: [DRAFT] Rework implementation to be interrupt-safe #2079

Are you sure you want to change the base?

c18n: [DRAFT] Rework implementation to be interrupt-safe #2079

Conversation

dpgao commented Apr 6, 2024 • edited

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

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

dpgao commented Apr 6, 2024 •

edited