From aaf78c16f5087b0fd998f97696ab492ee30e30f4 Mon Sep 17 00:00:00 2001 From: Konstantin Belousov Date: Wed, 23 Sep 2020 18:03:07 +0000 Subject: [PATCH] Do not leak oldvmspace if image activation failed and current address space is already destroyed, so kern_execve() terminates the process. While there, clean up some internals of post_execve() inlined in init_main. Reported by: Peter Reviewed by: markj Sponsored by: The FreeBSD Foundation MFC after: 1 week Differential revision: https://reviews.freebsd.org/D26525 --- sys/compat/cloudabi/cloudabi_proc.c | 2 +- sys/compat/freebsd32/freebsd32_misc.c | 4 +-- sys/compat/linux/linux_emul.c | 2 +- sys/kern/init_main.c | 9 ++---- sys/kern/kern_exec.c | 40 +++++++++++++++++---------- sys/sys/imgact.h | 1 + sys/sys/syscallsubr.h | 3 +- 7 files changed, 35 insertions(+), 26 deletions(-) diff --git a/sys/compat/cloudabi/cloudabi_proc.c b/sys/compat/cloudabi/cloudabi_proc.c index 24333311d38..378409edaef 100644 --- a/sys/compat/cloudabi/cloudabi_proc.c +++ b/sys/compat/cloudabi/cloudabi_proc.c @@ -57,7 +57,7 @@ cloudabi_sys_proc_exec(struct thread *td, uap->fds, uap->fds_len); if (error == 0) { args.fd = uap->fd; - error = kern_execve(td, &args, NULL); + error = kern_execve(td, &args, NULL, oldvmspace); } post_execve(td, error, oldvmspace); return (error); diff --git a/sys/compat/freebsd32/freebsd32_misc.c b/sys/compat/freebsd32/freebsd32_misc.c index bafe3e55211..0677fae89ff 100644 --- a/sys/compat/freebsd32/freebsd32_misc.c +++ b/sys/compat/freebsd32/freebsd32_misc.c @@ -440,7 +440,7 @@ freebsd32_execve(struct thread *td, struct freebsd32_execve_args *uap) error = freebsd32_exec_copyin_args(&eargs, uap->fname, UIO_USERSPACE, uap->argv, uap->envv); if (error == 0) - error = kern_execve(td, &eargs, NULL); + error = kern_execve(td, &eargs, NULL, oldvmspace); post_execve(td, error, oldvmspace); return (error); } @@ -459,7 +459,7 @@ freebsd32_fexecve(struct thread *td, struct freebsd32_fexecve_args *uap) uap->argv, uap->envv); if (error == 0) { eargs.fd = uap->fd; - error = kern_execve(td, &eargs, NULL); + error = kern_execve(td, &eargs, NULL, oldvmspace); } post_execve(td, error, oldvmspace); return (error); diff --git a/sys/compat/linux/linux_emul.c b/sys/compat/linux/linux_emul.c index 73678d963af..4326ef5b16c 100644 --- a/sys/compat/linux/linux_emul.c +++ b/sys/compat/linux/linux_emul.c @@ -256,7 +256,7 @@ linux_common_execve(struct thread *td, struct image_args *eargs) if (error != 0) return (error); - error = kern_execve(td, eargs, NULL); + error = kern_execve(td, eargs, NULL, oldvmspace); post_execve(td, error, oldvmspace); if (error != EJUSTRETURN) return (error); diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c index f3531c061e8..045e2797a2f 100644 --- a/sys/kern/init_main.c +++ b/sys/kern/init_main.c @@ -752,16 +752,11 @@ start_init(void *dummy) KASSERT((td->td_pflags & TDP_EXECVMSPC) == 0, ("nested execve")); oldvmspace = td->td_proc->p_vmspace; - error = kern_execve(td, &args, NULL); + error = kern_execve(td, &args, NULL, oldvmspace); KASSERT(error != 0, ("kern_execve returned success, not EJUSTRETURN")); if (error == EJUSTRETURN) { - if ((td->td_pflags & TDP_EXECVMSPC) != 0) { - KASSERT(p->p_vmspace != oldvmspace, - ("oldvmspace still used")); - vmspace_free(oldvmspace); - td->td_pflags &= ~TDP_EXECVMSPC; - } + exec_cleanup(td, oldvmspace); free(free_init_path, M_TEMP); TSEXIT(); return; diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c index 926633e4e55..314d63aaa28 100644 --- a/sys/kern/kern_exec.c +++ b/sys/kern/kern_exec.c @@ -118,7 +118,7 @@ static int sysctl_kern_ps_strings(SYSCTL_HANDLER_ARGS); static int sysctl_kern_usrstack(SYSCTL_HANDLER_ARGS); static int sysctl_kern_stackprot(SYSCTL_HANDLER_ARGS); static int do_execve(struct thread *td, struct image_args *args, - struct mac *mac_p); + struct mac *mac_p, struct vmspace *oldvmspace); /* XXX This should be vm_size_t. */ SYSCTL_PROC(_kern, KERN_PS_STRINGS, ps_strings, CTLTYPE_ULONG|CTLFLAG_RD| @@ -223,7 +223,7 @@ sys_execve(struct thread *td, struct execve_args *uap) error = exec_copyin_args(&args, uap->fname, UIO_USERSPACE, uap->argv, uap->envv); if (error == 0) - error = kern_execve(td, &args, NULL); + error = kern_execve(td, &args, NULL, oldvmspace); post_execve(td, error, oldvmspace); return (error); } @@ -249,7 +249,7 @@ sys_fexecve(struct thread *td, struct fexecve_args *uap) uap->argv, uap->envv); if (error == 0) { args.fd = uap->fd; - error = kern_execve(td, &args, NULL); + error = kern_execve(td, &args, NULL, oldvmspace); } post_execve(td, error, oldvmspace); return (error); @@ -278,7 +278,7 @@ sys___mac_execve(struct thread *td, struct __mac_execve_args *uap) error = exec_copyin_args(&args, uap->fname, UIO_USERSPACE, uap->argv, uap->envv); if (error == 0) - error = kern_execve(td, &args, uap->mac_p); + error = kern_execve(td, &args, uap->mac_p, oldvmspace); post_execve(td, error, oldvmspace); return (error); #else @@ -326,30 +326,26 @@ post_execve(struct thread *td, int error, struct vmspace *oldvmspace) thread_single_end(p, SINGLE_BOUNDARY); PROC_UNLOCK(p); } - if ((td->td_pflags & TDP_EXECVMSPC) != 0) { - KASSERT(p->p_vmspace != oldvmspace, - ("oldvmspace still used")); - vmspace_free(oldvmspace); - td->td_pflags &= ~TDP_EXECVMSPC; - } + exec_cleanup(td, oldvmspace); } /* - * XXX: kern_execve has the astonishing property of not always returning to + * kern_execve() has the astonishing property of not always returning to * the caller. If sufficiently bad things happen during the call to * do_execve(), it can end up calling exit1(); as a result, callers must * avoid doing anything which they might need to undo (e.g., allocating * memory). */ int -kern_execve(struct thread *td, struct image_args *args, struct mac *mac_p) +kern_execve(struct thread *td, struct image_args *args, struct mac *mac_p, + struct vmspace *oldvmspace) { AUDIT_ARG_ARGV(args->begin_argv, args->argc, exec_args_get_begin_envv(args) - args->begin_argv); AUDIT_ARG_ENVV(exec_args_get_begin_envv(args), args->envc, args->endp - exec_args_get_begin_envv(args)); - return (do_execve(td, args, mac_p)); + return (do_execve(td, args, mac_p, oldvmspace)); } /* @@ -357,7 +353,8 @@ kern_execve(struct thread *td, struct image_args *args, struct mac *mac_p) * userspace pointers from the passed thread. */ static int -do_execve(struct thread *td, struct image_args *args, struct mac *mac_p) +do_execve(struct thread *td, struct image_args *args, struct mac *mac_p, + struct vmspace *oldvmspace) { struct proc *p = td->td_proc; struct nameidata nd; @@ -949,6 +946,7 @@ do_execve(struct thread *td, struct image_args *args, struct mac *mac_p) if (error && imgp->vmspace_destroyed) { /* sorry, no more process anymore. exit gracefully */ + exec_cleanup(td, oldvmspace); exit1(td, 0, SIGABRT); /* NOT REACHED */ } @@ -967,6 +965,20 @@ do_execve(struct thread *td, struct image_args *args, struct mac *mac_p) return (error == 0 ? EJUSTRETURN : error); } +void +exec_cleanup(struct thread *td, struct vmspace *oldvmspace) +{ + struct proc *p; + + p = td->td_proc; + if ((td->td_pflags & TDP_EXECVMSPC) != 0) { + KASSERT(p->p_vmspace != oldvmspace, + ("oldvmspace still used")); + vmspace_free(oldvmspace); + td->td_pflags &= ~TDP_EXECVMSPC; + } +} + int exec_map_first_page(struct image_params *imgp) { diff --git a/sys/sys/imgact.h b/sys/sys/imgact.h index 6222725c9ca..5463b6f80d1 100644 --- a/sys/sys/imgact.h +++ b/sys/sys/imgact.h @@ -112,6 +112,7 @@ int exec_args_adjust_args(struct image_args *args, size_t consume, ssize_t extend); char *exec_args_get_begin_envv(struct image_args *args); int exec_check_permissions(struct image_params *); +void exec_cleanup(struct thread *td, struct vmspace *); int exec_copyout_strings(struct image_params *, uintptr_t *); void exec_free_args(struct image_args *); int exec_new_vmspace(struct image_params *, struct sysentvec *); diff --git a/sys/sys/syscallsubr.h b/sys/sys/syscallsubr.h index 7e5329b50ec..61afa0e61be 100644 --- a/sys/sys/syscallsubr.h +++ b/sys/sys/syscallsubr.h @@ -63,6 +63,7 @@ struct stat; struct thr_param; struct uio; struct vm_map; +struct vmspace; typedef int (*mmap_check_fp_fn)(struct file *, int, int, int); @@ -129,7 +130,7 @@ int kern_cpuset_setid(struct thread *td, cpuwhich_t which, id_t id, cpusetid_t setid); int kern_dup(struct thread *td, u_int mode, int flags, int old, int new); int kern_execve(struct thread *td, struct image_args *args, - struct mac *mac_p); + struct mac *mac_p, struct vmspace *oldvmspace); int kern_fchmodat(struct thread *td, int fd, const char *path, enum uio_seg pathseg, mode_t mode, int flag); int kern_fchownat(struct thread *td, int fd, const char *path, -- 2.45.0