git: 5d58f959d39b - main - jail: Fix lock-free access to dynamic pr.allow flags

Jamie Gritton jamie at FreeBSD.org
Sat Dec 26 20:53:35 UTC 2020


The branch main has been updated by jamie:

URL: https://cgit.FreeBSD.org/src/commit/?id=5d58f959d39bc1d4cbe11634060c18455a46606b

commit 5d58f959d39bc1d4cbe11634060c18455a46606b
Author:     Jamie Gritton <jamie at FreeBSD.org>
AuthorDate: 2020-12-26 20:53:28 +0000
Commit:     Jamie Gritton <jamie at FreeBSD.org>
CommitDate: 2020-12-26 20:53:28 +0000

    jail: Fix lock-free access to dynamic pr.allow flags
    
    Use atomic access and a memory barrier to ensure that the flag parameter
    in pr_flag_allow is indeed set after the rest of the structure is valid.
    
    Simplify adding flag bits with pr_allow_all, a dynamic version of
    PR_ALLOW_ALL_STATIC.
---
 sys/kern/kern_jail.c | 56 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c
index c29d966283f8..bdcebcdfaf6c 100644
--- a/sys/kern/kern_jail.c
+++ b/sys/kern/kern_jail.c
@@ -121,7 +121,7 @@ MTX_SYSINIT(prison0, &prison0.pr_mtx, "jail mutex", MTX_DEF);
 struct bool_flags {
 	const char	*name;
 	const char	*noname;
-	unsigned	 flag;
+	volatile u_int	 flag;
 };
 struct jailsys_flags {
 	const char	*name;
@@ -185,7 +185,11 @@ static struct jailsys_flags pr_flag_jailsys[] = {
 };
 const size_t pr_flag_jailsys_size = sizeof(pr_flag_jailsys);
 
-/* Make this array full-size so dynamic parameters can be added. */
+/*
+ * Make this array full-size so dynamic parameters can be added.
+ * It is protected by prison0.mtx, but lockless reading is allowed
+ * with an atomic check of the flag values.
+ */
 static struct bool_flags pr_flag_allow[NBBY * NBPW] = {
 	{"allow.set_hostname", "allow.noset_hostname", PR_ALLOW_SET_HOSTNAME},
 	{"allow.sysvipc", "allow.nosysvipc", PR_ALLOW_SYSVIPC},
@@ -202,6 +206,7 @@ static struct bool_flags pr_flag_allow[NBBY * NBPW] = {
 	 PR_ALLOW_UNPRIV_DEBUG},
 	{"allow.suser", "allow.nosuser", PR_ALLOW_SUSER},
 };
+static unsigned pr_allow_all = PR_ALLOW_ALL_STATIC;
 const size_t pr_flag_allow_size = sizeof(pr_flag_allow);
 
 #define	JAIL_DEFAULT_ALLOW		(PR_ALLOW_SET_HOSTNAME | \
@@ -349,7 +354,7 @@ kern_jail(struct thread *td, struct jail *j)
 	if (!jailed(td->td_ucred)) {
 		for (bf = pr_flag_allow;
 		     bf < pr_flag_allow + nitems(pr_flag_allow) &&
-			bf->flag != 0;
+			atomic_load_int(&bf->flag) != 0;
 		     bf++) {
 			optiov[opt.uio_iovcnt].iov_base = __DECONST(char *,
 			    (jail_default_allow & bf->flag)
@@ -684,7 +689,8 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags)
 
 	pr_allow = ch_allow = 0;
 	for (bf = pr_flag_allow;
-	     bf < pr_flag_allow + nitems(pr_flag_allow) && bf->flag != 0;
+	     bf < pr_flag_allow + nitems(pr_flag_allow) &&
+		atomic_load_int(&bf->flag) != 0;
 	     bf++) {
 		vfs_flagopt(opts, bf->name, &pr_allow, bf->flag);
 		vfs_flagopt(opts, bf->noname, &ch_allow, bf->flag);
@@ -2190,7 +2196,8 @@ kern_jail_get(struct thread *td, struct uio *optuio, int flags)
 			goto done_deref;
 	}
 	for (bf = pr_flag_allow;
-	     bf < pr_flag_allow + nitems(pr_flag_allow) && bf->flag != 0;
+	     bf < pr_flag_allow + nitems(pr_flag_allow) &&
+		atomic_load_int(&bf->flag) != 0;
 	     bf++) {
 		i = (pr->pr_allow & bf->flag) ? 1 : 0;
 		error = vfs_setopt(opts, bf->name, &i, sizeof(i));
@@ -3904,7 +3911,7 @@ prison_add_allow(const char *prefix, const char *name, const char *prefix_descr,
 #ifndef NO_SYSCTL_DESCR
 	char *descr_deprecated;
 #endif
-	unsigned allow_flag;
+	u_int allow_flag;
 
 	if (prefix
 	    ? asprintf(&allow_name, M_PRISON, "allow.%s.%s", prefix, name)
@@ -3923,7 +3930,8 @@ prison_add_allow(const char *prefix, const char *name, const char *prefix_descr,
 	 */
 	mtx_lock(&prison0.pr_mtx);
 	for (bf = pr_flag_allow;
-	     bf < pr_flag_allow + nitems(pr_flag_allow) && bf->flag != 0;
+	     bf < pr_flag_allow + nitems(pr_flag_allow) &&
+		atomic_load_int(&bf->flag) != 0;
 	     bf++) {
 		if (strcmp(bf->name, allow_name) == 0) {
 			allow_flag = bf->flag;
@@ -3932,38 +3940,37 @@ prison_add_allow(const char *prefix, const char *name, const char *prefix_descr,
 	}
 
 	/*
-	 * Find a free bit in prison0's pr_allow, failing if there are none
+	 * Find a free bit in pr_allow_all, failing if there are none
 	 * (which shouldn't happen as long as we keep track of how many
 	 * potential dynamic flags exist).
-	 *
-	 * Due to per-jail unprivileged process debugging support
-	 * using pr_allow, also verify against PR_ALLOW_ALL_STATIC.
-	 * prison0 may have unprivileged process debugging unset.
 	 */
 	for (allow_flag = 1;; allow_flag <<= 1) {
 		if (allow_flag == 0)
 			goto no_add;
-		if (allow_flag & PR_ALLOW_ALL_STATIC)
-			continue;
-		if ((prison0.pr_allow & allow_flag) == 0)
+		if ((pr_allow_all & allow_flag) == 0)
 			break;
 	}
 
-	/*
-	 * Note the parameter in the next open slot in pr_flag_allow.
-	 * Set the flag last so code that checks pr_flag_allow can do so
-	 * without locking.
-	 */
-	for (bf = pr_flag_allow; bf->flag != 0; bf++)
+	/* Note the parameter in the next open slot in pr_flag_allow. */
+	for (bf = pr_flag_allow; ; bf++) {
 		if (bf == pr_flag_allow + nitems(pr_flag_allow)) {
 			/* This should never happen, but is not fatal. */
 			allow_flag = 0;
 			goto no_add;
 		}
-	prison0.pr_allow |= allow_flag;
+		if (atomic_load_int(&bf->flag) == 0)
+			break;
+	}
 	bf->name = allow_name;
 	bf->noname = allow_noname;
-	bf->flag = allow_flag;
+	pr_allow_all |= allow_flag;
+	/*
+	 * prison0 always has permission for the new parameter.
+	 * Other jails must have it granted to them.
+	 */
+	prison0.pr_allow |= allow_flag;
+	/* The flag indicates a valid entry, so make sure it is set last. */
+	atomic_store_rel_int(&bf->flag, allow_flag);
 	mtx_unlock(&prison0.pr_mtx);
 
 	/*
@@ -4260,7 +4267,8 @@ db_show_prison(struct prison *pr)
 	}
 	db_printf(" allow           = 0x%x", pr->pr_allow);
 	for (bf = pr_flag_allow;
-	     bf < pr_flag_allow + nitems(pr_flag_allow) && bf->flag != 0;
+	     bf < pr_flag_allow + nitems(pr_flag_allow) &&
+		atomic_load_int(&bf->flag) != 0;
 	     bf++)
 		if (pr->pr_allow & bf->flag)
 			db_printf(" %s", bf->name);


More information about the dev-commits-src-all mailing list