diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c --- a/sys/kern/kern_jail.c +++ b/sys/kern/kern_jail.c @@ -1015,12 +1015,13 @@ #ifdef INET6 struct prison_ip *ip6; #endif + struct proc *p; struct vfsopt *opt; struct vfsoptlist *opts; struct prison *pr, *deadpr, *dinspr, *inspr, *mypr, *ppr, *tpr; struct ucred *jdcred; struct vnode *root; - char *domain, *errmsg, *host, *name, *namelc, *p, *path, *uuid; + char *domain, *ep, *errmsg, *host, *name, *namelc, *path, *uuid; char *g_path, *osrelstr; struct bool_flags *bf; struct jailsys_flags *jsf; @@ -1061,8 +1062,25 @@ if ((flags & (JAIL_USE_DESC | JAIL_AT_DESC)) == (JAIL_USE_DESC | JAIL_AT_DESC)) return (EINVAL); - prison_hold(mypr); + /* + * Make sure only one thread in the process is trying to + * attach to a jail, or they might end up with a process root + * from one prison, but attached to the jail of another. + */ + if (flags & JAIL_ATTACH) { + p = td->td_proc; + if ((p->p_flag & P_HADTHREADS) != 0) { + PROC_LOCK(p); + if (thread_single(p, SINGLE_BOUNDARY)) { + PROC_UNLOCK(p); + return (ERESTART); + } + PROC_UNLOCK(p); + } + } + + prison_hold(mypr); #ifdef INET ip4 = NULL; #endif @@ -1528,8 +1546,8 @@ maybe_changed = false; if (cuflags == JAIL_CREATE && jid == 0 && name != NULL) { namelc = strrchr(name, '.'); - jid = strtoul(namelc != NULL ? namelc + 1 : name, &p, 10); - if (*p != '\0') + jid = strtoul(namelc != NULL ? namelc + 1 : name, &ep, 10); + if (*ep != '\0') jid = 0; } sx_xlock(&allprison_lock); @@ -1989,8 +2007,8 @@ */ if (namelc[0] == '\0') snprintf(namelc = numbuf, sizeof(numbuf), "%d", jid); - else if ((strtoul(namelc, &p, 10) != jid || - namelc[0] < '1' || namelc[0] > '9') && *p == '\0') { + else if ((strtoul(namelc, &ep, 10) != jid || + namelc[0] < '1' || namelc[0] > '9') && *ep == '\0') { error = EINVAL; vfs_opterror(opts, "name cannot be numeric (unless it is the jid)"); @@ -2416,6 +2434,11 @@ if (opts != NULL) vfs_freeopts(opts); prison_free(mypr); + if ((flags & JAIL_ATTACH) && (p->p_flag & P_HADTHREADS)) { + PROC_LOCK(p); + thread_single_end(p, SINGLE_BOUNDARY); + PROC_UNLOCK(p); + } return (error); } @@ -3056,11 +3079,22 @@ sys_jail_attach(struct thread *td, struct jail_attach_args *uap) { struct prison *pr; + struct proc *p; int drflags, error; error = priv_check(td, PRIV_JAIL_ATTACH); if (error) return (error); + /* Only let a single thread in the process try to attach at a time. */ + p = td->td_proc; + if ((p->p_flag & P_HADTHREADS) != 0) { + PROC_LOCK(p); + if (thread_single(p, SINGLE_BOUNDARY)) { + PROC_UNLOCK(p); + return (ERESTART); + } + PROC_UNLOCK(p); + } sx_slock(&allprison_lock); drflags = PD_LIST_SLOCKED; @@ -3087,6 +3121,11 @@ done: prison_deref(pr, drflags); + if ((p->p_flag & P_HADTHREADS) != 0) { + PROC_LOCK(p); + thread_single_end(p, SINGLE_BOUNDARY); + PROC_UNLOCK(p); + } return (error); } @@ -3099,9 +3138,21 @@ sys_jail_attach_jd(struct thread *td, struct jail_attach_jd_args *uap) { struct prison *pr; + struct proc *p; struct ucred *jdcred; int drflags, error; + /* Only let a single thread in the process try to attach at a time. */ + p = td->td_proc; + if ((p->p_flag & P_HADTHREADS) != 0) { + PROC_LOCK(p); + if (thread_single(p, SINGLE_BOUNDARY)) { + PROC_UNLOCK(p); + return (ERESTART); + } + PROC_UNLOCK(p); + } + sx_slock(&allprison_lock); drflags = PD_LIST_SLOCKED; pr = NULL; @@ -3128,6 +3179,11 @@ done: prison_deref(pr, drflags); + if ((p->p_flag & P_HADTHREADS) != 0) { + PROC_LOCK(p); + thread_single_end(p, SINGLE_BOUNDARY); + PROC_UNLOCK(p); + } return (error); } @@ -3145,15 +3201,6 @@ sx_assert(&allprison_lock, SX_LOCKED); KASSERT(prison_isvalid(pr), ("Attaching to invalid prison %p", pr)); - /* - * XXX: Note that there is a slight race here if two threads - * in the same privileged process attempt to attach to two - * different jails at the same time. It is important for - * user processes not to do this, or they might end up with - * a process root from one prison, but attached to the jail - * of another. - */ - /* * Note the caller's locking state, but gain and track our own * references. The caller will see that locks have been @@ -3243,7 +3290,7 @@ e_unlock: VOP_UNLOCK(pr->pr_root); e_revert_osd: - /* Tell modules this thread is still in its old jail after all. */ + /* Tell modules this process is still in its old jail after all. */ if (!(drflags & (PD_LIST_SLOCKED | PD_LIST_XLOCKED))) { sx_slock(&allprison_lock); drflags |= PD_LIST_SLOCKED; diff --git a/tests/sys/kern/Makefile b/tests/sys/kern/Makefile --- a/tests/sys/kern/Makefile +++ b/tests/sys/kern/Makefile @@ -23,6 +23,7 @@ ATF_TESTS_C+= fdgrowtable_test ATF_TESTS_C+= getdirentries_test ATF_TESTS_C+= jail_lookup_root +ATF_TESTS_C+= jail_thread ATF_TESTS_C+= jaildesc ATF_TESTS_C+= inotify_test ATF_TESTS_C+= kill_zombie @@ -96,6 +97,7 @@ LIBADD.aslr+= util LIBADD.copy_file_range+= md LIBADD.jail_lookup_root+= jail util +LIBADD.jail_thread+= jail pthread LIBADD.jaildesc+= kvm pthread LIBADD.ssl_sendfile+= pthread crypto ssl CFLAGS.sys_getrandom+= -I${SRCTOP}/sys/contrib/zstd/lib diff --git a/tests/sys/kern/jail_thread.c b/tests/sys/kern/jail_thread.c new file mode 100644 --- /dev/null +++ b/tests/sys/kern/jail_thread.c @@ -0,0 +1,171 @@ +/*- + * SPDX-License-Identifier: BSD-2-Clause + * + * Copyright (c) 2026 James Gritton + */ + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define JAIL_NAME "jail_thread_attach_test" + +#define NJAILS 2 /* Jails/threads required to show race. */ +#define ATTACH_ROUNDS 10000 /* Number of attempts to make race happen. */ + +struct jailinfo { + ssize_t jfd; + ino_t ino; +}; + +static pthread_barrier_t barrier; + +/* Attach a thread to a jail with jail_attach_jd. */ +static void *thread_jail_attach_jd(void *arg) +{ + int error; + + /* + * Synchronize to get as close as possible to the same time, + * then attach to a jail. + */ + error = pthread_barrier_wait(&barrier); + ATF_REQUIRE_MSG(error == 0 || error == PTHREAD_BARRIER_SERIAL_THREAD, + "pthread_barrier_wait: %s", strerror(errno)); + ATF_REQUIRE_MSG(jail_attach_jd((int)(size_t)arg) == 0, + "jail_attach_jd: %s", strerror(errno)); + return (NULL); +} + +/* Attach a thread to a jail with jail_setv. */ +static void *thread_jail_setv(void *arg) +{ + int error; + char jdescstr[16]; + + /* + * Synchronize to get as close as possible to the same time, + * then attach to a jail. + */ + error = pthread_barrier_wait(&barrier); + ATF_REQUIRE_MSG(error == 0 || error == PTHREAD_BARRIER_SERIAL_THREAD, + "pthread_barrier_wait: %s", strerror(errno)); + snprintf(jdescstr, sizeof(jdescstr), "%zd", (ssize_t)arg); + ATF_REQUIRE_MSG(jail_setv(JAIL_UPDATE | JAIL_ATTACH | JAIL_USE_DESC, + "desc", jdescstr, NULL) > 0, + "jail_setv: %s", jail_errmsg[0] ? jail_errmsg : strerror(errno)); + return (NULL); +} + +ATF_TC(jail_thread_attach); + +ATF_TC_HEAD(jail_thread_attach, tc) +{ + atf_tc_set_md_var(tc, "require.user", "root"); +} + +ATF_TC_BODY(jail_thread_attach, tc) +{ + int ri, spn, mixed_jails; + size_t ji, ti, ji_hostname, ji_ino; + char *cwd; + void *(*thread_handler)(void*); + struct stat st; + char jnamestr[32], jpathstr[MAXPATHLEN], jdescstr[16]; + pthread_t threads[NJAILS]; + struct jailinfo jinfo[NJAILS]; + + /* Make enough jails to cause contention. */ + cwd = getcwd(NULL, MAXPATHLEN); + ATF_REQUIRE_MSG(cwd != NULL, "getcwd: %s", strerror(errno)); + for (ji = 0; ji < NJAILS; ++ji) { + snprintf(jnamestr, sizeof(jnamestr), JAIL_NAME "%zu", ji); + spn = snprintf(jpathstr, MAXPATHLEN, "%s/jail%zu", cwd, ji); + ATF_REQUIRE_MSG((size_t)spn < MAXPATHLEN, + "snprintf exceeded MAXPATHLEN: %d", spn); + ATF_REQUIRE_MSG(mkdir(jpathstr, 0755) == 0 || errno == EEXIST, + "mkdir %s: %s", jpathstr, strerror(errno)); + ATF_REQUIRE_MSG(stat(jpathstr, &st) == 0, + "stat %s: %s", jpathstr, strerror(errno)); + jinfo[ji].ino = st.st_ino; + jdescstr[0] = '\0'; + ATF_REQUIRE_MSG(jail_setv(JAIL_CREATE | JAIL_OWN_DESC, + "name", jnamestr, + "host.hostname", jnamestr, + "path", jpathstr, + "desc", jdescstr, + "persist", "true", + NULL) > 0, + "jail_setv: %s", + jail_errmsg[0] ? jail_errmsg : strerror(errno)); + jinfo[ji].jfd = strtol(jdescstr, NULL, 10); + } + + /* Two different system calls can attach threads; check both. */ + for (thread_handler = thread_jail_attach_jd; + thread_handler != NULL; + thread_handler = thread_handler == thread_jail_attach_jd ? + thread_jail_setv : NULL) { + mixed_jails = 0; + for (ri = 0; ri < ATTACH_ROUNDS; ++ri) { + ATF_REQUIRE_MSG( + pthread_barrier_init(&barrier, NULL, NJAILS) == 0, + "pthread_barrier_init: %s", strerror(errno)); + for (ti = 1; ti < NJAILS; ++ti) + ATF_REQUIRE_MSG( + pthread_create(&threads[ti], NULL, + thread_handler, (void*)jinfo[ti].jfd) == 0, + "pthread_create: %s", strerror(errno)); + thread_handler((void*)jinfo[0].jfd); + for (ti = 1; ti < NJAILS; ++ti) + ATF_REQUIRE_MSG( + pthread_join(threads[ti], NULL) == 0, + "pthread_join: %s", strerror(errno)); + ATF_REQUIRE_MSG(pthread_barrier_destroy(&barrier) == 0, + "pthread_barrier_destroy: %s", strerror(errno)); + /* + * Find the current jail from the hostname, and also + * by the root inode. They should be the same. + */ + ATF_REQUIRE_MSG( + gethostname(jnamestr, sizeof(jnamestr)) == 0, + "gethostname: %s", strerror(errno)); + ATF_REQUIRE_MSG(strncmp(jnamestr, JAIL_NAME, + sizeof(JAIL_NAME) - 1) == 0, + "unexpected jail hostname %s", jnamestr); + ji_hostname = strtol(jnamestr + sizeof(JAIL_NAME) - 1, + NULL, 10); + ATF_REQUIRE_MSG(stat("/", &st) == 0, + "stat /: %s", strerror(errno)); + for (ji_ino = 0; ji_ino < NJAILS; ++ji_ino) + if (jinfo[ji_ino].ino == st.st_ino) + break; + ATF_REQUIRE_MSG(ji_ino < NJAILS, + "unexpected jail root inode %lu", + (unsigned long)st.st_ino); + mixed_jails += ji_hostname != ji_ino; + } + /* It's an error if any of the rounds had a mismatch. */ + ATF_REQUIRE_MSG(mixed_jails == 0, + "%d of %d jail_attach races with different root and " + "credentials", mixed_jails, ATTACH_ROUNDS); + } +} + +ATF_TP_ADD_TCS(tp) +{ + ATF_TP_ADD_TC(tp, jail_thread_attach); + return (atf_no_error()); +}