[Devel] Re: [PATCH 4/6] c/r: tighten logic to protect against bogus pids in input

[Serge E. Hallyn]

Quoting Oren Laadan (orenl at librato.com):
> During restart, each process that completes its restore will wake up
> the next process in line, using the pid provided in the image file.
> A malicious user could provide bogus pids and cause arbitrary tasks to
> be woken up.
> 
> This patch tightens the checks and requires that to wake-up a task by
> pid during restart, the target task must belong to (have) the same
> restart context (task->checkpoint_ctx) as the current process.
> 
> Also, update in-code comments about restart and zombie logic.
> 
> Signed-off-by: Oren Laadan <orenl at cs.columbia.edu>

Acked-by: Serge Hallyn <serue at us.ibm.com>

> ---
>  checkpoint/restart.c |   35 ++++++++++++++++++++---------------
>  1 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/checkpoint/restart.c b/checkpoint/restart.c
> index 2282ffc..840b5cb 100644
> --- a/checkpoint/restart.c
> +++ b/checkpoint/restart.c
> @@ -462,12 +462,12 @@ static int restore_read_tree(struct ckpt_ctx *ctx)
>  		return PTR_ERR(h);
> 
>  	ret = -EINVAL;
> -	if (h->nr_tasks < 0)
> +	if (h->nr_tasks <= 0)
>  		goto out;
> 
>  	ctx->nr_pids = h->nr_tasks;
>  	size = sizeof(*ctx->pids_arr) * ctx->nr_pids;
> -	if (size < 0)		/* overflow ? */
> +	if (size <= 0)		/* overflow ? */
>  		goto out;
> 
>  	ctx->pids_arr = kmalloc(size, GFP_KERNEL);
> @@ -520,8 +520,11 @@ int ckpt_activate_next(struct ckpt_ctx *ctx)
> 
>  	rcu_read_lock();
>  	task = find_task_by_pid_ns(pid, ctx->root_nsproxy->pid_ns);
> -	if (task)
> +	/* target task must have same restart context */
> +	if (task && task->checkpoint_ctx == ctx)
>  		wake_up_process(task);
> +	else
> +		task = NULL;
>  	rcu_read_unlock();
> 
>  	if (!task) {
> @@ -568,9 +571,8 @@ static int do_restore_task(void)
>  	int ret;
> 
>  	/*
> -	 * Wait for coordinator to make become visible, then grab a
> -	 * reference to its restart context. If we're the last task to
> -	 * do it, notify the coordinator.
> +	 * Wait for coordinator to become visible, then grab a
> +	 * reference to its restart context.
>  	 */
>  	ret = wait_event_interruptible(waitq, current->checkpoint_ctx);
>  	if (ret < 0)
> @@ -610,8 +612,9 @@ static int do_restore_task(void)
>  		goto out;
> 
>  	/*
> -	 * zombie: we're done here; Save @ctx on task_struct, to be
> -	 * used to ckpt_activate_next(), and released, from do_exit().
> +	 * zombie: we're done here; do_exit() will notice the @ctx on
> +	 * our current->checkpoint_ctx (and our PF_RESTARTING) - it
> +	 * will call ckpt_activate_next() and release the @ctx.
>  	 */
>  	if (ret)
>  		do_exit(current->exit_code);
> @@ -638,11 +641,11 @@ static int do_restore_task(void)
>  }
> 
>  /**
> - * prepare_descendants - set ->restart_tsk of all descendants
> + * prepare_descendants - set ->checkpoint_ctx of all descendants
>   * @ctx: checkpoint context
>   * @root: root process for restart
>   *
> - * Called by the coodinator to set the ->restart_tsk pointer of the
> + * Called by the coodinator to set the ->checkpoint_ctx pointer of the
>   * root task and all its descendants.
>   */
>  static int prepare_descendants(struct ckpt_ctx *ctx, struct task_struct *root)
> @@ -662,7 +665,7 @@ static int prepare_descendants(struct ckpt_ctx *ctx, struct task_struct *root)
>  			break;
>  		}
>  		/*
> -		 * Set task->restart_tsk of all non-zombie descendants.
> +		 * Set task->checkpoint_ctx of all non-zombie descendants.
>  		 * If a descendant already has a ->checkpoint_ctx, it
>  		 * must be a coordinator (for a different restart ?) so
>  		 * we fail.
> @@ -727,6 +730,7 @@ static int wait_all_tasks_finish(struct ckpt_ctx *ctx)
> 
>  	init_completion(&ctx->complete);
> 
> +	BUG_ON(ctx->active_pid != -1);
>  	ret = ckpt_activate_next(ctx);
>  	if (ret < 0)
>  		return ret;
> @@ -839,7 +843,7 @@ static int do_restore_coord(struct ckpt_ctx *ctx, pid_t pid)
>  		if (ret < 0)
>  			goto out;
>  	} else {
> -		/* prepare descendants' t->restart_tsk point to coord */
> +		/* prepare descendants' t->checkpoint_ctx point to coord */
>  		ret = prepare_descendants(ctx, ctx->root_task);
>  		if (ret < 0)
>  			goto out;
> @@ -970,11 +974,12 @@ void exit_checkpoint(struct task_struct *tsk)
>  {
>  	struct ckpt_ctx *ctx;
> 
> -	ctx = tsk->checkpoint_ctx;
> -	tsk->checkpoint_ctx = NULL;
> +	/* no one else will touch this, because @tsk is dead already */
> +	ctx = xchg(&tsk->checkpoint_ctx, NULL);
> 
> -	/* restarting zombies will acrivate next task in restart */
> +	/* restarting zombies will activate next task in restart */
>  	if (tsk->flags & PF_RESTARTING) {
> +		BUG_ON(ctx->active_pid == -1);
>  		if (ckpt_activate_next(ctx) < 0)
>  			pr_warning("c/r: [%d] failed zombie exit\n", tsk->pid);
>  	}
> -- 
> 1.6.0.4
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers