From debe558799a039749c66e1bfa41c6d004cd4f0f6 Mon Sep 17 00:00:00 2001 From: jamie Date: Thu, 9 Jun 2016 16:41:41 +0000 Subject: Make sure the OSD methods for jail set and remove can't run concurrently, by holding allprison_lock exclusively (even if only for a moment before downgrading) on all paths that call PR_METHOD_REMOVE. Since they may run on a downgraded lock, it's still possible for them to run concurrently with PR_METHOD_GET, which will need to use the prison lock. --- sys/kern/kern_jail.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) (limited to 'sys/kern/kern_jail.c') diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c index 35e270a..ea4b628 100644 --- a/sys/kern/kern_jail.c +++ b/sys/kern/kern_jail.c @@ -2383,7 +2383,14 @@ sys_jail_attach(struct thread *td, struct jail_attach_args *uap) if (error) return (error); - sx_slock(&allprison_lock); + /* + * Start with exclusive hold on allprison_lock to ensure that a possible + * PR_METHOD_REMOVE call isn't concurrent with jail_set or jail_remove. + * But then immediately downgrade it since we don't need to stop + * readers. + */ + sx_xlock(&allprison_lock); + sx_downgrade(&allprison_lock); pr = prison_find_child(td->td_ucred->cr_prison, uap->jid); if (pr == NULL) { sx_sunlock(&allprison_lock); @@ -2601,9 +2608,11 @@ prison_complete(void *context, int pending) { struct prison *pr = context; + sx_xlock(&allprison_lock); mtx_lock(&pr->pr_mtx); prison_deref(pr, pr->pr_uref - ? PD_DEREF | PD_DEUREF | PD_LOCKED : PD_LOCKED); + ? PD_DEREF | PD_DEUREF | PD_LOCKED | PD_LIST_XLOCKED + : PD_LOCKED | PD_LIST_XLOCKED); } /* @@ -2647,13 +2656,8 @@ prison_deref(struct prison *pr, int flags) */ if (lasturef) { if (!(flags & (PD_LIST_SLOCKED | PD_LIST_XLOCKED))) { - if (ref > 1) { - sx_slock(&allprison_lock); - flags |= PD_LIST_SLOCKED; - } else { - sx_xlock(&allprison_lock); - flags |= PD_LIST_XLOCKED; - } + sx_xlock(&allprison_lock); + flags |= PD_LIST_XLOCKED; } (void)osd_jail_call(pr, PR_METHOD_REMOVE, NULL); mtx_lock(&pr->pr_mtx); -- cgit v1.1 From 91dc125e2d62a46ec88abc017cb7d8f07c9c166e Mon Sep 17 00:00:00 2001 From: jamie Date: Thu, 9 Jun 2016 20:39:57 +0000 Subject: Clean up some logic in jail error messages, replacing a missing test and a redundant test with a single correct test. --- sys/kern/kern_jail.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) (limited to 'sys/kern/kern_jail.c') diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c index ea4b628..7bc496e 100644 --- a/sys/kern/kern_jail.c +++ b/sys/kern/kern_jail.c @@ -1929,19 +1929,17 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) vrele(root); done_errmsg: if (error) { - vfs_getopt(opts, "errmsg", (void **)&errmsg, &errmsg_len); - if (errmsg_len > 0) { + if (vfs_getopt(opts, "errmsg", (void **)&errmsg, + &errmsg_len) == 0 && errmsg_len > 0) { errmsg_pos = 2 * vfs_getopt_pos(opts, "errmsg") + 1; - if (errmsg_pos > 0) { - if (optuio->uio_segflg == UIO_SYSSPACE) - bcopy(errmsg, - optuio->uio_iov[errmsg_pos].iov_base, - errmsg_len); - else - copyout(errmsg, - optuio->uio_iov[errmsg_pos].iov_base, - errmsg_len); - } + if (optuio->uio_segflg == UIO_SYSSPACE) + bcopy(errmsg, + optuio->uio_iov[errmsg_pos].iov_base, + errmsg_len); + else + copyout(errmsg, + optuio->uio_iov[errmsg_pos].iov_base, + errmsg_len); } } done_free: -- cgit v1.1 From 786e18e2983525384b47757362dc5a8c151f650a Mon Sep 17 00:00:00 2001 From: jamie Date: Thu, 9 Jun 2016 20:43:14 +0000 Subject: Re-order some jail parameter reading to prevent a vnode leak. --- sys/kern/kern_jail.c | 80 ++++++++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 40 deletions(-) (limited to 'sys/kern/kern_jail.c') diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c index 7bc496e..74522d9 100644 --- a/sys/kern/kern_jail.c +++ b/sys/kern/kern_jail.c @@ -920,6 +920,46 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) } #endif + error = vfs_getopt(opts, "osrelease", (void **)&osrelstr, &len); + if (error == ENOENT) + osrelstr = NULL; + else if (error != 0) + goto done_free; + else { + if (flags & JAIL_UPDATE) { + error = EINVAL; + vfs_opterror(opts, + "osrelease cannot be changed after creation"); + goto done_errmsg; + } + if (len == 0 || len >= OSRELEASELEN) { + error = EINVAL; + vfs_opterror(opts, + "osrelease string must be 1-%d bytes long", + OSRELEASELEN - 1); + goto done_errmsg; + } + } + + error = vfs_copyopt(opts, "osreldate", &osreldt, sizeof(osreldt)); + if (error == ENOENT) + osreldt = 0; + else if (error != 0) + goto done_free; + else { + if (flags & JAIL_UPDATE) { + error = EINVAL; + vfs_opterror(opts, + "osreldate cannot be changed after creation"); + goto done_errmsg; + } + if (osreldt == 0) { + error = EINVAL; + vfs_opterror(opts, "osreldate cannot be 0"); + goto done_errmsg; + } + } + fullpath_disabled = 0; root = NULL; error = vfs_getopt(opts, "path", (void **)&path, &len); @@ -975,46 +1015,6 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) } } - error = vfs_getopt(opts, "osrelease", (void **)&osrelstr, &len); - if (error == ENOENT) - osrelstr = NULL; - else if (error != 0) - goto done_free; - else { - if (flags & JAIL_UPDATE) { - error = EINVAL; - vfs_opterror(opts, - "osrelease cannot be changed after creation"); - goto done_errmsg; - } - if (len == 0 || len >= OSRELEASELEN) { - error = EINVAL; - vfs_opterror(opts, - "osrelease string must be 1-%d bytes long", - OSRELEASELEN - 1); - goto done_errmsg; - } - } - - error = vfs_copyopt(opts, "osreldate", &osreldt, sizeof(osreldt)); - if (error == ENOENT) - osreldt = 0; - else if (error != 0) - goto done_free; - else { - if (flags & JAIL_UPDATE) { - error = EINVAL; - vfs_opterror(opts, - "osreldate cannot be changed after creation"); - goto done_errmsg; - } - if (osreldt == 0) { - error = EINVAL; - vfs_opterror(opts, "osreldate cannot be 0"); - goto done_errmsg; - } - } - /* * Find the specified jail, or at least its parent. * This abuses the file error codes ENOENT and EEXIST. -- cgit v1.1 From aad828cb49a839665c04e3b71d736991a15d378f Mon Sep 17 00:00:00 2001 From: jamie Date: Thu, 9 Jun 2016 21:59:11 +0000 Subject: Fix a vnode leak when giving a child jail a too-long path when debug.disablefullpath=1. --- sys/kern/kern_jail.c | 1 + 1 file changed, 1 insertion(+) (limited to 'sys/kern/kern_jail.c') diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c index 74522d9..ebd714f 100644 --- a/sys/kern/kern_jail.c +++ b/sys/kern/kern_jail.c @@ -1010,6 +1010,7 @@ kern_jail_set(struct thread *td, struct uio *optuio, int flags) if (len + (path[0] == '/' && strcmp(mypr->pr_path, "/") ? strlen(mypr->pr_path) : 0) > MAXPATHLEN) { error = ENAMETOOLONG; + vrele(root); goto done_free; } } -- cgit v1.1