git: f3851b235b23 - main - ktrace: Fix a race with fork()

[Dmitry Chagin]

On Thu, May 27, 2021 at 07:53:23PM +0000, Mark Johnston wrote:
> The branch main has been updated by markj:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=f3851b235b23d9220ace31bbc89b1fe0a78fc75c
> 
> commit f3851b235b23d9220ace31bbc89b1fe0a78fc75c
> Author:     Mark Johnston <markj at FreeBSD.org>
> AuthorDate: 2021-05-27 19:49:59 +0000
> Commit:     Mark Johnston <markj at FreeBSD.org>
> CommitDate: 2021-05-27 19:52:20 +0000
> 
>     ktrace: Fix a race with fork()
>     
>     ktrace(2) may toggle trace points in any of
>     1. a single process
>     2. all members of a process group
>     3. all descendents of the processes in 1 or 2
>     
>     In the first two cases, we do not permit the operation if the process is
>     being forked or not visible. However, in case 3 we did not enforce this
>     restriction for descendents. As a result, the assertions about the child
>     in ktrprocfork() may be violated.
>     
>     Move these checks into ktrops() so that they are applied consistently.
>     
>     Allow KTROP_CLEAR for nascent processes. Otherwise, there is a window
>     where we cannot clear trace points for a nascent child if they are
>     inherited from the parent.
>     
>     Reported by:    syzbot+d96676592978f137e05c at syzkaller.appspotmail.com
>     Reported by:    syzbot+7c98fcf84a4439f2817f at syzkaller.appspotmail.com
>     Reviewed by:    kib
>     MFC after:      1 week
>     Sponsored by:   The FreeBSD Foundation
>     Differential Revision:  https://reviews.freebsd.org/D30481
> ---
>  sys/kern/kern_ktrace.c | 43 ++++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/sys/kern/kern_ktrace.c b/sys/kern/kern_ktrace.c
> index dc064d9ebd67..4a2dad20b035 100644
> --- a/sys/kern/kern_ktrace.c
> +++ b/sys/kern/kern_ktrace.c
> @@ -1006,7 +1006,7 @@ sys_ktrace(struct thread *td, struct ktrace_args *uap)
>  	int facs = uap->facs & ~KTRFAC_ROOT;
>  	int ops = KTROP(uap->ops);
>  	int descend = uap->ops & KTRFLAG_DESCEND;
> -	int nfound, ret = 0;
> +	int ret = 0;
>  	int flags, error = 0;
>  	struct nameidata nd;
>  	struct ktr_io_params *kiop, *old_kiop;
> @@ -1080,42 +1080,31 @@ restart:
>  			error = ESRCH;
>  			goto done;
>  		}
> +
>  		/*
>  		 * ktrops() may call vrele(). Lock pg_members
>  		 * by the proctree_lock rather than pg_mtx.
>  		 */
>  		PGRP_UNLOCK(pg);
> -		nfound = 0;
> +		if (LIST_EMPTY(&pg->pg_members)) {
> +			sx_sunlock(&proctree_lock);
> +			error = ESRCH;
> +			goto done;
> +		}
>  		LIST_FOREACH(p, &pg->pg_members, p_pglist) {
>  			PROC_LOCK(p);
> -			if (p->p_state == PRS_NEW ||
> -			    p_cansee(td, p) != 0) {
> -				PROC_UNLOCK(p); 
> -				continue;
> -			}
> -			nfound++;
>  			if (descend)
>  				ret |= ktrsetchildren(td, p, ops, facs, kiop);
>  			else
>  				ret |= ktrops(td, p, ops, facs, kiop);
>  		}
> -		if (nfound == 0) {
> -			sx_sunlock(&proctree_lock);
> -			error = ESRCH;
> -			goto done;
> -		}
>  	} else {
>  		/*
>  		 * by pid
>  		 */
>  		p = pfind(uap->pid);
> -		if (p == NULL)
> +		if (p == NULL) {
>  			error = ESRCH;
> -		else
> -			error = p_cansee(td, p);
> -		if (error) {
> -			if (p != NULL)
> -				PROC_UNLOCK(p);
>  			sx_sunlock(&proctree_lock);
>  			goto done;
>  		}
> @@ -1187,8 +1176,20 @@ ktrops(struct thread *td, struct proc *p, int ops, int facs,
>  		PROC_UNLOCK(p);
>  		return (0);
>  	}
> -	if (p->p_flag & P_WEXIT) {
> -		/* If the process is exiting, just ignore it. */
> +	if ((ops == KTROP_SET && p->p_state == PRS_NEW) || !p_cansee(td, p)) {

			^^^^^^^^^^^^^^ seems that it broke ktrace:
			root at mordor:~ # ktrace ls
			ktrace: ktrace.out: Operation not permitted


> +		/*
> +		 * Disallow setting trace points if the process is being born.
> +		 * This avoids races with trace point inheritance in
> +		 * ktrprocfork().
> +		 */
> +		PROC_UNLOCK(p);
> +		return (0);
> +	}
> +	if ((p->p_flag & P_WEXIT) != 0) {
> +		/*
> +		 * There's nothing to do if the process is exiting, but avoid
> +		 * signaling an error.
> +		 */
>  		PROC_UNLOCK(p);
>  		return (1);
>  	}