[CRIU] [PATCH 08/10] seccomp: Dont forget to suspend filtering on threads

[Andrey Vagin]

On Mon, May 07, 2018 at 11:42:48AM +0300, Cyrill Gorcunov wrote:
> When considering if we to call PTRACE_O_SUSPEND_SECCOMP
> on the tid we should take into account if there at least
> one thread which has seccomp mode enabled, otherwise
> we might miss filter suspension and restore procedure
> might break due to own criu syscall get filtered out.
> 
> Same time we should move seccomp restore for threads
> to take place after CR_STATE_RESTORE_SIGCHLD state
> so that main criu code will attach to threads and
> setup seccomp suspension flag before we start
> restoring the filters.
> 
> Reported-by: Andrei Vagin <avagin at virtuozzo.com>
> Reviewed-by: Dmitry Safonov <0x7f454c46 at gmail.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov at gmail.com>
> ---
>  criu/cr-restore.c   | 29 +++++++++++++++++++++++++----
>  criu/pie/restorer.c | 21 ++++++++++-----------
>  2 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/criu/cr-restore.c b/criu/cr-restore.c
> index a7a232b2e028..89d73e191e97 100644
> --- a/criu/cr-restore.c
> +++ b/criu/cr-restore.c
> @@ -914,6 +914,23 @@ static int open_cores(int pid, CoreEntry *leader_core)
>  
>  	current->core = cores;
>  
> +	/*
> +	 * Walk over all threads and if one them is having
> +	 * active seccomp mode we will suspend filtering
> +	 * on the whole group until restore complete.
> +	 *
> +	 * Otherwise any criu code which might use same syscall
> +	 * if present inside a filter chain would take filter
> +	 * action and might break restore procedure.
> +	 */
> +	for (i = 0; i < current->nr_threads; i++) {
> +		ThreadCoreEntry *thread_core = cores[i]->thread_core;
> +		if (thread_core->seccomp_mode != SECCOMP_MODE_DISABLED) {
> +			rsti(current)->has_seccomp = true;
> +			break;
> +		}
> +	}
> +
>  	return 0;
>  err:
>  	xfree(cores);
> @@ -1531,10 +1548,14 @@ static inline int fork_with_pid(struct pstree_item *item)
>  			return -1;
>  		}
>  
> -		if (item->pid->state != TASK_DEAD)
> -			rsti(item)->has_seccomp = ca.core->thread_core->seccomp_mode != SECCOMP_MODE_DISABLED;
> -		else
> -			rsti(item)->has_seccomp = false;
> +		/*
> +		 * By default we assume that seccomp is not
> +		 * used at all (especially on dead task). Later
> +		 * we will walk over all threads and check in
> +		 * details if filter is present setting up
> +		 * this flag as appropriate.
> +		 */
> +		rsti(item)->has_seccomp = false;
>  
>  		if (unlikely(item == root_item))
>  			maybe_clone_parent(item, &ca);
> diff --git a/criu/pie/restorer.c b/criu/pie/restorer.c
> index 2ba0bcf2f72f..44f8648319e1 100644
> --- a/criu/pie/restorer.c
> +++ b/criu/pie/restorer.c
> @@ -574,17 +574,6 @@ long __export_restore_thread(struct thread_restore_args *args)
>  		sys_close(fd);
>  	}
>  
> -	/*
> -	 * Make sure it's before creds, since it's privileged
> -	 * operation bound to uid 0 in current user ns.
> -	 */
> -	if (restore_seccomp(args))
> -		goto core_restore_end;
> -
> -	ret = restore_creds(args->creds_args, args->ta->proc_fd);
> -	if (ret)
> -		goto core_restore_end;
> -
>  	ret = restore_dumpable_flag(&args->ta->mm);
>  	if (ret)
>  		goto core_restore_end;
> @@ -599,6 +588,16 @@ long __export_restore_thread(struct thread_restore_args *args)
>  	restore_finish_stage(task_entries_local, CR_STATE_RESTORE_SIGCHLD);
>  	restore_pdeath_sig(args);
>  
> +	/*
> +	 * Make sure it's before creds, since it's privileged
> +	 * operation bound to uid 0 in current user ns.
> +	 */
> +	if (restore_seccomp(args))
> +		goto core_restore_end;

I far as I remember, we can't call exit after CR_STATE_RESTORE_SIGCHLD,
I mean we can't use goto core_restore_end here

> +
> +	ret = restore_creds(args->creds_args, args->ta->proc_fd);
> +	if (ret)
> +		goto core_restore_end;
>  	restore_finish_stage(task_entries_local, CR_STATE_RESTORE_CREDS);
>  
>  	futex_dec_and_wake(&thread_inprogress);
> -- 
> 2.14.3
>