Date: Sat, 26 Dec 2020 20:53:35 GMT From: Jamie Gritton <jamie@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 5d58f959d39b - main - jail: Fix lock-free access to dynamic pr.allow flags Message-ID: <202012262053.0BQKrZVp005511@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by jamie: URL: https://cgit.FreeBSD.org/src/commit/?id=5d58f959d39bc1d4cbe11634060c18455a46606b commit 5d58f959d39bc1d4cbe11634060c18455a46606b Author: Jamie Gritton <jamie@FreeBSD.org> AuthorDate: 2020-12-26 20:53:28 +0000 Commit: Jamie Gritton <jamie@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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202012262053.0BQKrZVp005511>