procstat: Add a 'compartments' command to list c18n compartments #2276

dpgao · 2024-12-22T04:00:00Z

This rebases #2272 to dev and adds more features (incl. a generation counter for the compartment array to deal with races).

rwatson · 2024-12-22T23:55:57Z

sys/cheri/c18n.h

+struct cheri_c18n_compart {
+	ssize_t	ccc_id;
+	char	ccc_name[CHERI_C18N_COMPART_MAXNAME];
+	char	_ccc_pad[64];		/* Shrink as new fields added above. */


I wonder if we should preemptively define a 'ccc_flags' field to capture concepts like "This is a non-default sub-library compartment", "this compartment can performance system calls", and similar?

I think we can always add them in the future when they become needed. Right now RTLD doesn't track such information about compartments, so it is risky to to add the flags prematurely.

rwatson · 2024-12-23T00:03:44Z

sys/kern/kern_proc.c

+	 */
+	if (len != sizeof(info) ||
+	    info.version != CHERI_C18N_INFO_VERSION ||
+	    info.comparts_gen % 2 != 0 ||


I somewhat wondered if we wanted some sort of memory-barrier arrangement to ensure that we got a clean(ish) snapshot -- i.e., that if we saw the current generation, we saw all the stores we read from the compartment / string tables came before the generation number we read was stored, and that at the end of the sysctl function we haven't seen any stores that post-dated that generation-number store?

Every iteration below we also re-read the generation number and check that it hasn’t changed. Presumably this achieves the desired effect?

rwatson · 2024-12-23T00:04:33Z

usr.bin/procstat/procstat_compartments.c

+		return;
+	}
+	if ((procstat_opts & PS_OPT_NOHEADER) == 0)
+		xo_emit("{T:/%5s %-19s %4s %-40s}\n", "PID", "COMM", "CID",


I always feel that 19 characters wide is quite a long slot for COMM, which most of the time uses list. Not sure if other procstat/ps modes might give less by default?

I think 19 is fine (in fact it is too short for the cheribsdtest variants). And the existing c18n and cheri commands both use 19.

I widened to 19 (MAXCOMLEN) for auxv in 64e9f6a. It can be shorter, but it should probably the right most column so it can safely spill if it's not going to be MAXCOMLEN.

The compartment name needs to be last and is likely to be the very long thing.

brooksdavis

Generally speaking it would be nice if this were split into kernel, libprocstat, and prostatic commits.

brooksdavis · 2025-01-02T23:20:19Z

lib/libprocstat/libprocstat.3

@@ -47,6 +53,8 @@
 .Nm procstat_getargv ,
 .Nm procstat_getauxv ,
 .Nm procstat_getenvv ,
+.Nm procstat_getc18n ,


Documenting this is good, but it should be a separate commit.

brooksdavis · 2025-01-03T22:05:07Z

usr.bin/procstat/procstat_compartments.c

+		return;
+	}
+	if ((procstat_opts & PS_OPT_NOHEADER) == 0)
+		xo_emit("{T:/%5s %-19s %4s %-40s}\n", "PID", "COMM", "CID",


I widened to 19 (MAXCOMLEN) for auxv in 64e9f6a. It can be shorter, but it should probably the right most column so it can safely spill if it's not going to be MAXCOMLEN.

bsdjhb · 2025-01-09T17:13:40Z

sys/cheri/c18n.h

+ * The interface provided by the kernel via sysctl for compartmentalization
+ * monitoring tools such as procstat.
+ */
+#define	CHERI_C18N_COMPART_MAXNAME	56


I worry this should be more like PATH_MAX + NAME_MAX + 2 (separator and terminator).

bsdjhb · 2025-01-09T17:16:05Z

sys/cheri/c18n.h

+#define	CHERI_C18N_COMPART_MAXNAME	56
+#define	CHERI_C18N_COMPART_LAST		-1
+
+struct cheri_c18n_compart {


Hmm, I would model this on other kinfo_* structures. I would embed a size field at the start and move the name field to the end. The name could be packed in the exported form similar to what we do for kinfo_file and kinfo_vmobject sysctls now. Userspace would unpack it back out into a simple array.

bsdjhb · 2025-01-09T17:16:44Z

sys/kern/kern_proc.c

@@ -2534,8 +2534,8 @@ sysctl_kern_proc_c18n(SYSCTL_HANDLER_ARGS)
 	    info.version != CHERI_C18N_INFO_VERSION ||
 	    info.stats_size == 0 ||
 	    info.stats_size > RTLD_C18N_STATS_MAX_SIZE ||
-	    !__CAP_CHECK(info.stats, info.stats_size) ||
-	    (cheri_getperm(info.stats) & CHERI_PERM_LOAD) == 0) {
+	    !cheri_can_access(info.stats, CHERI_PERM_LOAD,


These two hunks seem to be unrelated and some other bug fix?

This is just replacing the __CAP_CHECK pattern with cheri_can_access to resemble what sysctl_kern_proc_c18n_compartments is doing below.

bsdjhb · 2025-01-09T17:19:47Z

sys/kern/kern_proc.c

+		if (!cheri_can_access(sptr, CHERI_PERM_LOAD,
+		    (__cheri_addr ptraddr_t)&sptr[n], 1))
+			return (-1);
+		readlen = proc_readmem(td, p,


You can likely do a bit better than reading one byte at a time. That is, you can fetch the remaining bytes for the current page up to the limit of len or the remaining length of sptr. You could always read it into a temporary PAGE_SIZE'd buffer and only copy out to buf up to the first \0. That will be significantly more efficient.

bsdjhb · 2025-01-09T17:22:18Z

sys/sys/sysctl.h

@@ -1065,6 +1065,7 @@ TAILQ_HEAD(sysctl_ctx_list, sysctl_ctx_entry);
 #define	KERN_PROC_REVOKER_STATE	47	/* revoker state */
 #define	KERN_PROC_REVOKER_EPOCH	48	/* revoker epoch */
 #define	KERN_PROC_C18N		49	/* compartmentalisation statistics */


Seems like it may be worth renaming this to KERN_PROC_C18N_STATS

bsdjhb · 2025-01-09T17:25:39Z

sys/kern/kern_proc.c

+		error = ENOEXEC;
+		goto out;
+	}
+


You really want to optimize the first call to sysctl() with a NULL oldptr from userspace that is just trying to query the size to use. You can do a decent estimate by just reading in the top-level structure with the count of compartments and using that to compute the size needed and use that with SYSCTL_OUT with a NULL input buffer and then return early without reading the actual compartment list from userspace.

bsdjhb · 2025-01-09T17:31:57Z

lib/libprocstat/libprocstat.c

@@ -399,6 +405,53 @@ procstat_getc18n(struct procstat *procstat, struct kinfo_proc *kp,
 	return (-1);
 }

+int


This should not require the caller to allocate an array and hope for the best. :) It should return an allocated array of objects that the caller can free. You can then call the sysctl twice, once to get the size estimate and a second time to populate it. This requires fixing the sysctl to add the optimized path for querying the size I mentioned.

bsdjhb · 2025-01-09T17:32:20Z

usr.bin/procstat/procstat_compartments.c

+
+#include "procstat.h"
+
+#define	C18N_MAX_COMPARTS	1024	/* Horrible but functional, for now. */


Then this hack can go away as it should.

bsdjhb · 2025-01-09T17:32:53Z

usr.bin/procstat/procstat_compartments.c

+		return;
+	}
+	if ((procstat_opts & PS_OPT_NOHEADER) == 0)
+		xo_emit("{T:/%5s %-19s %4s %-40s}\n", "PID", "COMM", "CID",


The compartment name needs to be last and is likely to be the very long thing.

dpgao · 2025-01-09T17:46:08Z

@bsdjhb Would be curious to hear your thoughts about the generation counter as a fix for the race condition. Does this seem sound to you?

bsdjhb · 2025-01-09T18:50:14Z

@bsdjhb Would be curious to hear your thoughts about the generation counter as a fix for the race condition. Does this seem sound to you?

Yes, that pattern is used elsewhere for the same trick.

dpgao requested review from rwatson and jrtc27 December 22, 2024 04:00

rwatson approved these changes Dec 23, 2024

View reviewed changes

brooksdavis reviewed Jan 3, 2025

View reviewed changes

dpgao added 2 commits January 9, 2025 15:16

c18n: Make RTLD export the compartment array to the kernel

896ce61

proc: Teach kernel to extract list of compartment names from RTLD

0681b1d

dpgao force-pushed the c18n-procstat-comparts branch from 44db10c to acd2d25 Compare January 9, 2025 15:17

procstat: Allow querying the names of compartments of another process

dd8c97b

dpgao force-pushed the c18n-procstat-comparts branch from acd2d25 to dd8c97b Compare January 9, 2025 15:53

bsdjhb reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

procstat: Add a 'compartments' command to list c18n compartments #2276

procstat: Add a 'compartments' command to list c18n compartments #2276

dpgao commented Dec 22, 2024 •

edited

Loading

rwatson Dec 22, 2024

dpgao Dec 31, 2024

rwatson Dec 23, 2024

dpgao Dec 23, 2024

rwatson Dec 23, 2024

dpgao Dec 31, 2024

brooksdavis Jan 3, 2025

bsdjhb Jan 9, 2025

brooksdavis left a comment

brooksdavis Jan 2, 2025

brooksdavis Jan 3, 2025

bsdjhb Jan 9, 2025

bsdjhb Jan 9, 2025

bsdjhb Jan 9, 2025

dpgao Jan 9, 2025

bsdjhb Jan 9, 2025

bsdjhb Jan 9, 2025

bsdjhb Jan 9, 2025

bsdjhb Jan 9, 2025

bsdjhb Jan 9, 2025

bsdjhb Jan 9, 2025

dpgao commented Jan 9, 2025

bsdjhb commented Jan 9, 2025


		#include "procstat.h"

		#define C18N_MAX_COMPARTS 1024 /* Horrible but functional, for now. */

procstat: Add a 'compartments' command to list c18n compartments #2276

Are you sure you want to change the base?

procstat: Add a 'compartments' command to list c18n compartments #2276

Conversation

dpgao commented Dec 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksdavis 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

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 Jan 9, 2025

bsdjhb commented Jan 9, 2025

dpgao commented Dec 22, 2024 •

edited

Loading