Date: Sat, 21 Mar 2015 03:51:51 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-current@freebsd.org, jenkins-admin@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: Re: [PATCH 1/3] fork: assign refed credentials earlier Message-ID: <20150321015151.GF2379@kib.kiev.ua> In-Reply-To: <1426899640-6599-2-git-send-email-mjguzik@gmail.com> References: <20150320122125.GP2379@kib.kiev.ua> <1426899640-6599-1-git-send-email-mjguzik@gmail.com> <1426899640-6599-2-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Mar 21, 2015 at 02:00:38AM +0100, Mateusz Guzik wrote:
> From: Mateusz Guzik <mjg@freebsd.org>
>
> Prior to this change the kernel would take p1's credentials and assign
> them tempororarily to p2. But p1 could change credentials at that time
> and in effect give us a use-after-free.
In which way could it change the credentials ? The assigned credentials
are taken from td_ucred, which, I thought, are guaranteed to be stable
for the duration of a syscall.
Other two patches look fine.
> ---
> sys/kern/kern_fork.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
> index ae86fe1..15833fd 100644
> --- a/sys/kern/kern_fork.c
> +++ b/sys/kern/kern_fork.c
> @@ -410,9 +410,6 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2,
> bzero(&p2->p_startzero,
> __rangeof(struct proc, p_startzero, p_endzero));
>
> - crhold(td->td_ucred);
> - proc_set_cred(p2, td->td_ucred);
> -
> /* Tell the prison that we exist. */
> prison_proc_hold(p2->p_ucred->cr_prison);
>
> @@ -832,7 +829,7 @@ fork1(struct thread *td, int flags, int pages, struct proc **procp,
> td2 = thread_alloc(pages);
> if (td2 == NULL) {
> error = ENOMEM;
> - goto fail1;
> + goto fail2;
> }
> proc_linkup(newproc, td2);
> } else {
> @@ -841,7 +838,7 @@ fork1(struct thread *td, int flags, int pages, struct proc **procp,
> vm_thread_dispose(td2);
> if (!thread_alloc_stack(td2, pages)) {
> error = ENOMEM;
> - goto fail1;
> + goto fail2;
> }
> }
> }
> @@ -850,7 +847,7 @@ fork1(struct thread *td, int flags, int pages, struct proc **procp,
> vm2 = vmspace_fork(p1->p_vmspace, &mem_charged);
> if (vm2 == NULL) {
> error = ENOMEM;
> - goto fail1;
> + goto fail2;
> }
> if (!swap_reserve(mem_charged)) {
> /*
> @@ -861,7 +858,7 @@ fork1(struct thread *td, int flags, int pages, struct proc **procp,
> */
> swap_reserve_force(mem_charged);
> error = ENOMEM;
> - goto fail1;
> + goto fail2;
> }
> } else
> vm2 = NULL;
> @@ -870,7 +867,7 @@ fork1(struct thread *td, int flags, int pages, struct proc **procp,
> * XXX: This is ugly; when we copy resource usage, we need to bump
> * per-cred resource counters.
> */
> - proc_set_cred(newproc, p1->p_ucred);
> + proc_set_cred(newproc, crhold(td->td_ucred));
>
> /*
> * Initialize resource accounting for the child process.
> @@ -946,6 +943,8 @@ fail:
> #endif
> racct_proc_exit(newproc);
> fail1:
> + crfree(proc_set_cred(newproc, NULL));
> +fail2:
> if (vm2 != NULL)
> vmspace_free(vm2);
> uma_zfree(proc_zone, newproc);
> --
> 2.3.2
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150321015151.GF2379>