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 @@ -158,6 +158,8 @@ static int get_next_prid(struct prison **insprp); static int get_next_deadid(struct prison **insprp); static int do_jail_attach(struct thread *td, struct prison *pr, int *drflagsp); +static int prison_attach_thread_single(struct thread *td); +static void prison_attach_thread_single_end(struct thread *td); static void prison_complete(void *context, int pending); static void prison_deref(struct prison *pr, int flags); static void prison_deref_kill(struct prison *pr, struct prisonlist *freeprison); @@ -1061,7 +1063,13 @@ if ((flags & (JAIL_USE_DESC | JAIL_AT_DESC)) == (JAIL_USE_DESC | JAIL_AT_DESC)) return (EINVAL); - prison_hold(mypr); + + /* Only let a single thread in the process try to attach at a time. */ + if (flags & JAIL_ATTACH) { + error = prison_attach_thread_single(td); + if (error != 0) + return (error); + } #ifdef INET ip4 = NULL; @@ -1072,6 +1080,7 @@ g_path = NULL; jfp_out = NULL; jfd_out = -1; + prison_hold(mypr); /* * Check all the parameters before committing to anything. Not all * errors can be caught early, but we may as well try. Also, this @@ -2416,6 +2425,8 @@ if (opts != NULL) vfs_freeopts(opts); prison_free(mypr); + if (flags & JAIL_ATTACH) + prison_attach_thread_single_end(td); return (error); } @@ -3061,6 +3072,10 @@ 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. */ + error = prison_attach_thread_single(td); + if (error != 0) + return (error); sx_slock(&allprison_lock); drflags = PD_LIST_SLOCKED; @@ -3087,6 +3102,7 @@ done: prison_deref(pr, drflags); + prison_attach_thread_single_end(td); return (error); } @@ -3102,6 +3118,11 @@ struct ucred *jdcred; int drflags, error; + /* Only let a single thread in the process try to attach at a time. */ + error = prison_attach_thread_single(td); + if (error != 0) + return (error); + sx_slock(&allprison_lock); drflags = PD_LIST_SLOCKED; pr = NULL; @@ -3128,6 +3149,7 @@ done: prison_deref(pr, drflags); + prison_attach_thread_single_end(td); return (error); } @@ -3145,15 +3167,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 +3256,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; @@ -3253,6 +3266,43 @@ return (error); } +/* + * Only one thread in a process should try to attach to a jail, or + * they might end up with a process root from one prison, but attached + * to the jail of another. Enforce this by making the process run in + * single-threaded mode for the duration of the system call, which + * also prevents other related calls such as chroot. + */ +static int +prison_attach_thread_single(struct thread *td) +{ + struct proc *p; + int error; + + error = 0; + p = td->td_proc; + if ((atomic_load_int(&p->p_flag) & P_HADTHREADS) != 0) { + PROC_LOCK(p); + if (thread_single(p, SINGLE_BOUNDARY)) + error = ERESTART; + PROC_UNLOCK(p); + } + return (error); +} + +static void +prison_attach_thread_single_end(struct thread *td) +{ + struct proc *p; + + p = td->td_proc; + if ((atomic_load_int(&p->p_flag) & P_HADTHREADS) != 0) { + PROC_LOCK(p); + thread_single_end(p, SINGLE_BOUNDARY); + PROC_UNLOCK(p); + } +} + /* * Returns a locked prison instance, or NULL on failure. */ 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 @@ -97,6 +98,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,231 @@ +/*- + * SPDX-License-Identifier: BSD-2-Clause + * + * Copyright (c) 2026 James Gritton + */ + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#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 *jail_name, + const char *syscall_name, bool jail_reset) +{ + int ri, spn, mixed_jails; + size_t ji, ti, ji_hostname, ji_ino, jail_namelen; + char *cwd; + struct stat st; + char jnamestr[64], 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), + "%s%zu", jail_name, 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. */ + jail_namelen = strlen(jail_name); + 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, jail_namelen) == 0, + "unexpected jail hostname %s", jnamestr); + ji_hostname = strtol(jnamestr + jail_namelen, 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); +} + +#define JAIL_NAME "jail_thread_attach_test" + + +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_thread_attach_test", "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_thread_attach_test_jail_set", "jail_set", 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, + "jail_thread_attach_test_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()); +}