Re: [PATCH 1/3] fork: assign refed credentials earlier

On Sat, Mar 21, 2015 at 02:00:38AM +0100, Mateusz Guzik wrote:
> From: Mateusz Guzik <mjg_at_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
> _at__at_ -410,9 +410,6 _at__at_ 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);
>  
> _at__at_ -832,7 +829,7 _at__at_ 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 {
> _at__at_ -841,7 +838,7 _at__at_ 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;
>  			}
>  		}
>  	}
> _at__at_ -850,7 +847,7 _at__at_ 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)) {
>  			/*
> _at__at_ -861,7 +858,7 _at__at_ 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;
> _at__at_ -870,7 +867,7 _at__at_ 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.
> _at__at_ -946,6 +943,8 _at__at_ 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