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

sys/cheri/c18n.h

sys/kern/kern_proc.c

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.

Given that even copyinstr() doesn't do this on arm64, and this is used only for debugging/monitoring, I feel like there complexity from that change probably isn't worth it .. especially as we'll then probably get the capability bounds checks wrong :-).

But, for an amusing time, you can look at proc_read_string(), which basically does the optimisation but doesn't actually know about strings 😄 .

Unlike copyinstr this is having to go lock the VM map, wire the page, etc. for each byte. copyinstr is just setting pcb_onfault around each byte fetch, so it does seem like there is quite a bit more overhead in this case compared to copyinstr. proc_read_string() doesn't seem to do any bounds checking at all, probably in part because the freebsd64 caller of it doesn't generate proper USER_CAPs but instead uses cheri_fromint. But proc_read_string does seem to be a bit broken.

Given that proc_readmem() returns the bytes read and that we have cheri_bytes_remaining you could do:

ssize_t readlen; size_t n, valid; if (len < 1) return (EFAULT); if (!cheri_can_access(sptr, CHERI_PERM_LOAD, (ptraddr_t)sptr) return (EPROT); valid = MIN(len - 1, cheri_bytes_remaining(sptr)); readlen = proc_readmem(td, p, (ptraddr_t)sptr, buf, valid); if (readlen <= 0) return (EFAULT); n = strnlen(buf, valid); if (n == valid && valid != len - 1) return (EPROT); buf[len - 1] = '\0'; return (0);

(I would also return an error, you don't need the length and this distinguishes EFAULT from EPROT)

Should it be MIN(len, ...) instead of MIN(len - 1, ...)? Otherwise a non-null-terminated string of exactly len - 1 bytes would fail trigger the last EPROT error because valid == len - 1.

And the last EPROT check should be if (cheri_bytes_remaining(sptr) <= len && valid == n), I think? Otherwise the above counterexample with the a bad string of length len also escapes the check.

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

Yes, that would be good.

sys/kern/kern_proc.c

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 · 2025-01-22T16:36:11Z

@bsdjhb I've just pushed a fix for the build failure caused by a missing __cheri_addr.

sys/kern/kern_proc.c

lib/libprocstat/libprocstat.c

bsdjhb · 2025-01-25T15:53:21Z

lib/libprocstat/libprocstat.c

+	}
+
+	/* Unpack elements of the input buffer into the output buffer. */
+	outbuf = malloc(n * sizeof(*outbuf));


My one suggestion here would be to use calloc() so that the extra padding between structures is zeroed. However, that can be fixed up as a later commit after merging.

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 force-pushed the c18n-procstat-comparts branch 2 times, most recently from acd2d25 to dd8c97b Compare January 9, 2025 15:53

bsdjhb reviewed Jan 9, 2025

View reviewed changes

dpgao force-pushed the c18n-procstat-comparts branch from dd8c97b to 9a24af1 Compare January 13, 2025 00:09

dpgao mentioned this pull request Jan 22, 2025

First cut at a "procstat compartments” command: list c18n compartments. #2272

Closed

c18n: Make RTLD export the compartment array to the kernel

946a429

dpgao force-pushed the c18n-procstat-comparts branch from 9a24af1 to 335aa7e Compare January 22, 2025 16:35

bsdjhb reviewed Jan 22, 2025

View reviewed changes

sys/kern/kern_proc.c Outdated Show resolved Hide resolved

lib/libprocstat/libprocstat.c Outdated Show resolved Hide resolved

dpgao added 2 commits January 23, 2025 16:19

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

3e836b7

procstat: Allow querying the names of compartments of another process

5b0a59f

dpgao force-pushed the c18n-procstat-comparts branch from 335aa7e to 5b0a59f Compare January 23, 2025 16:20

dpgao requested a review from bsdjhb January 23, 2025 17:03

bsdjhb approved these changes Jan 25, 2025

View reviewed changes

bsdjhb merged commit cf628ae into dev Jan 25, 2025
29 checks passed

bsdjhb deleted the c18n-procstat-comparts branch January 25, 2025 15:53

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

rwatson Jan 11, 2025

rwatson Jan 11, 2025

bsdjhb Jan 11, 2025

dpgao Jan 12, 2025

dpgao Jan 12, 2025

bsdjhb Jan 9, 2025

rwatson Jan 11, 2025

bsdjhb Jan 9, 2025

bsdjhb Jan 9, 2025

bsdjhb Jan 9, 2025

dpgao commented Jan 9, 2025

bsdjhb commented Jan 9, 2025

dpgao commented Jan 22, 2025

bsdjhb Jan 25, 2025


		#include "procstat.h"

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

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

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

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

Choose a reason for hiding this comment

dpgao commented Dec 22, 2024 •

edited

Loading