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,26 @@ 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. This + * is also checked in sys_jail_attach and sys_jail_attach_jd. + */ + 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 +1547,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 +2008,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 +2435,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 +3080,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 +3122,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 +3139,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 +3180,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 +3202,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 +3291,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 @@ -1,0 +1,230 @@ +/*- + * SPDX-License-Identifier: BSD-2-Clause + * + * Copyright (c) 2026 James Gritton + */ + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define JAIL_NAME "jail_thread_attach_test" + +#define NJAILS 3 /* One master jail and two to race. */ +#define NROUNDS 10000 /* Number of attempts to make race happen. */ + +struct jailinfo { + int jfd; + ino_t ino; + char path[MAXPATHLEN]; +}; + +static pthread_barrier_t barrier; +static struct jailinfo jinfo[NJAILS]; + +/* 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(jinfo[(size_t)arg].jfd) == 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]; + + 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), "%d", jinfo[(size_t)arg].jfd); + 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); +} + +/* Attach a thread to a jail with chroot. */ +static void *thread_chroot(void *arg) +{ + int error; + + /* This is a race between jail_attach and chroot. */ + if ((size_t)arg > 1) + return thread_jail_attach_jd(arg); + error = pthread_barrier_wait(&barrier); + ATF_REQUIRE_MSG(error == 0 || error == PTHREAD_BARRIER_SERIAL_THREAD, + "pthread_barrier_wait: %s", strerror(errno)); + ATF_REQUIRE_MSG(chroot(jinfo[(size_t)arg].path) == 0 || errno == ENOENT, + "chroot: %s", strerror(errno)); + return (NULL); +} + +static void thread_attach_test(void *(*thread_handler)(void*), + const char *syscall_name, bool jail_reset) +{ + int ri, spn, mixed_jails; + size_t ji, ti, ji_hostname, ji_ino; + char *cwd; + struct stat st; + char jnamestr[32], jdescstr[16]; + pthread_t threads[NJAILS]; + + if (jinfo[0].jfd == 0) { + /* Start with a master jail, so we can return to real root. */ + jdescstr[0] = '\0'; + ATF_REQUIRE_MSG(jail_setv(JAIL_CREATE | JAIL_OWN_DESC, + "name", JAIL_NAME, + "path", "/", + "desc", jdescstr, + "persist", "true", + NULL) > 0, + "jail_setv jail 0: %s", + jail_errmsg[0] ? jail_errmsg : strerror(errno)); + jinfo[0].jfd = strtol(jdescstr, NULL, 10); + /* Make enough jails to cause contention. */ + cwd = getcwd(NULL, MAXPATHLEN); + ATF_REQUIRE_MSG(cwd != NULL, "getcwd: %s", strerror(errno)); + for (ji = 1; ji < NJAILS; ++ji) { + snprintf(jnamestr, sizeof(jnamestr), + JAIL_NAME "%zu", ji); + spn = snprintf(jinfo[ji].path, MAXPATHLEN, + "%s/jail%zu", cwd, ji); + ATF_REQUIRE_MSG((size_t)spn < MAXPATHLEN, + "snprintf exceeded MAXPATHLEN: %d", spn); + ATF_REQUIRE_MSG( + mkdir(jinfo[ji].path, 0755) == 0 || errno == EEXIST, + "mkdir %s: %s", jinfo[ji].path, strerror(errno)); + ATF_REQUIRE_MSG(stat(jinfo[ji].path, &st) == 0, + "stat %s: %s", jinfo[ji].path, 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", jinfo[ji].path, + "desc", jdescstr, + "persist", "true", + NULL) > 0, + "jail_setv: %s", + jail_errmsg[0] ? jail_errmsg : strerror(errno)); + jinfo[ji].jfd = strtol(jdescstr, NULL, 10); + } + } else + ATF_REQUIRE_MSG(jail_attach_jd(jinfo[0].jfd) == 0, + "jail_attach_jd: %s", strerror(errno)); + + /* Check the different system calls that can race. */ + mixed_jails = 0; + for (ri = 0; ri < NROUNDS; ++ri) { + ATF_REQUIRE_MSG( + pthread_barrier_init(&barrier, NULL, NJAILS - 1) == 0, + "pthread_barrier_init: %s", strerror(errno)); + for (ti = 1; ti < NJAILS; ++ti) + ATF_REQUIRE_MSG( + pthread_create(&threads[ti], NULL, thread_handler, + (void*)ti) == 0, + "pthread_create: %s", strerror(errno)); + 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 = 1; 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; + /* Reset to the master jail, required for chroot. */ + if (jail_reset) + ATF_REQUIRE_MSG(jail_attach_jd(jinfo[0].jfd) == 0, + "jail_attach_jd: %s", strerror(errno)); + } + /* It's an error if any of the rounds had a mismatch. */ + ATF_REQUIRE_MSG(mixed_jails == 0, + "%d of %d %s races with different root and " + "credentials", mixed_jails, NROUNDS, syscall_name); +} + +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) +{ + thread_attach_test(thread_jail_attach_jd, "jail_attach_jd", false); +} + +ATF_TC(jail_thread_attach_jail_set); + +ATF_TC_HEAD(jail_thread_attach_jail_set, tc) +{ + atf_tc_set_md_var(tc, "require.user", "root"); +} + +ATF_TC_BODY(jail_thread_attach_jail_set, tc) +{ + thread_attach_test(thread_jail_setv, "jail_setv", false); +} + +ATF_TC(jail_thread_attach_chroot); + +ATF_TC_HEAD(jail_thread_attach_chroot, tc) +{ + atf_tc_set_md_var(tc, "require.user", "root"); +} + +ATF_TC_BODY(jail_thread_attach_chroot, tc) +{ + thread_attach_test(thread_chroot, "chroot", true); +} + +ATF_TP_ADD_TCS(tp) +{ + ATF_TP_ADD_TC(tp, jail_thread_attach); + ATF_TP_ADD_TC(tp, jail_thread_attach_jail_set); + ATF_TP_ADD_TC(tp, jail_thread_attach_chroot); + return (atf_no_error()); +}