LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: linux-kernel@vger.kernel.org
Cc: Linux Containers <containers@lists.linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>,
	linux-arch@vger.kernel.org
Subject: Re: [REVIEW][PATCH 02/26] signal/ptrace: Simplify and fix PTRACE_KILL
Date: Wed, 29 May 2019 09:35:13 -0500	[thread overview]
Message-ID: <87muj52use.fsf@xmission.com> (raw)
In-Reply-To: <20190523003916.20726-3-ebiederm@xmission.com> (Eric W. Biederman's message of "Wed, 22 May 2019 19:38:52 -0500")


I am dropping this one for now, as there are no dependencies with
the other patches, and this probably deserves some discussion on it's
own.

Eric

"Eric W. Biederman" <ebiederm@xmission.com> writes:

> Since PTRACE_KILL was introduced in 1.1.78 it has only worked if the
> process is stopped in do_signal.  On a ptraced but non-stopped process
> PTRACE_KILL has always returned success and done nothing.
>
> Separate the noop case of PTRACE_KILL from the case where it does
> nothing.  This fixes the fact that taking sighand lock in
> ptrace_resume is not safe if the process could be in the middle of
> exec or do_exit.  The current test for child->state is insufficient to
> prevent that race.
>
> With the code explicitly implementing the noop people maintaining
> ptrace no longer need to worry what happens in PTRACE_KILL if the
> process is not stopped.
>
> The alternative fix is to change the implementation of PTRACE_KILL
> to just be send_sig(SIGKILL, child, 1);  But I don't know if anything
> depends on the current documented behavior.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: b72c186999e6 ("ptrace: fix race between ptrace_resume() and wait_task_stopped()")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  kernel/ptrace.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 6f357f4fc859..5d6ff7040863 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -212,15 +212,18 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
>   *
>   * Check whether @child is being ptraced by %current and ready for further
>   * ptrace operations.  If @ignore_state is %false, @child also should be in
> - * %TASK_TRACED state and on return the child is guaranteed to be traced
> - * and not executing.  If @ignore_state is %true, @child can be in any
> - * state.
> + * %TASK_TRACED state and on succesful return the child is guaranteed to be
> + * traced and not executing.  If @ignore_state is %true, @child can be in
> + * any state on succesful return.
>   *
>   * CONTEXT:
>   * Grabs and releases tasklist_lock and @child->sighand->siglock.
>   *
>   * RETURNS:
> - * 0 on success, -ESRCH if %child is not ready.
> + * 0 on success,
> + * -ESRCH if %child is not traced
> + * -EAGAIN if %child can not be frozen
> + * -EBUSY if the wait for %child fails
>   */
>  static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
>  {
> @@ -240,6 +243,7 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
>  		 * child->sighand can't be NULL, release_task()
>  		 * does ptrace_unlink() before __exit_signal().
>  		 */
> +		ret = -EAGAIN;
>  		if (ignore_state || ptrace_freeze_traced(child))
>  			ret = 0;
>  	}
> @@ -253,7 +257,7 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
>  			 * so we should not worry about leaking __TASK_TRACED.
>  			 */
>  			WARN_ON(child->state == __TASK_TRACED);
> -			ret = -ESRCH;
> +			ret = -EBUSY;
>  		}
>  	}
>  
> @@ -1074,8 +1078,6 @@ int ptrace_request(struct task_struct *child, long request,
>  		return ptrace_resume(child, request, data);
>  
>  	case PTRACE_KILL:
> -		if (child->exit_state)	/* already dead */
> -			return 0;
>  		return ptrace_resume(child, request, SIGKILL);
>  
>  #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
> @@ -1147,14 +1149,17 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
>  		goto out_put_task_struct;
>  	}
>  
> -	ret = ptrace_check_attach(child, request == PTRACE_KILL ||
> -				  request == PTRACE_INTERRUPT);
> -	if (ret < 0)
> -		goto out_put_task_struct;
> -
> -	ret = arch_ptrace(child, request, addr, data);
> -	if (ret || request != PTRACE_DETACH)
> -		ptrace_unfreeze_traced(child);
> +	ret = ptrace_check_attach(child, request == PTRACE_INTERRUPT);
> +	if (!ret) {
> +		ret = arch_ptrace(child, request, addr, data);
> +		if (ret || request != PTRACE_DETACH)
> +			ptrace_unfreeze_traced(child);
> +	}
> +	/* PTRACE_KILL is a noop when not attached */
> +	else if ((request == PTRACE_KILL) && (ret != -ESRCH))
> +		ret = 0;
> +	else
> +		ret = -ESRCH;
>  
>   out_put_task_struct:
>  	put_task_struct(child);
> @@ -1292,13 +1297,17 @@ COMPAT_SYSCALL_DEFINE4(ptrace, compat_long_t, request, compat_long_t, pid,
>  		goto out_put_task_struct;
>  	}
>  
> -	ret = ptrace_check_attach(child, request == PTRACE_KILL ||
> -				  request == PTRACE_INTERRUPT);
> +	ret = ptrace_check_attach(child, request == PTRACE_INTERRUPT);
>  	if (!ret) {
>  		ret = compat_arch_ptrace(child, request, addr, data);
>  		if (ret || request != PTRACE_DETACH)
>  			ptrace_unfreeze_traced(child);
>  	}
> +	/* PTRACE_KILL is a noop when not attached */
> +	else if ((request == PTRACE_KILL) && (ret != -ESRCH))
> +		ret = 0;
> +	else
> +		ret = -ESRCH;
>  
>   out_put_task_struct:
>  	put_task_struct(child);

  reply	other threads:[~2019-05-29 14:35 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-23  0:38 [REVIEW][PATCH 00/26] signal: Remove task argument from force_sig_info Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 01/26] signal: Correct namespace fixups of si_pid and si_uid Eric W. Biederman
     [not found]   ` <20190529131503.F2AC221871@mail.kernel.org>
2019-05-29 15:18     ` Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 02/26] signal/ptrace: Simplify and fix PTRACE_KILL Eric W. Biederman
2019-05-29 14:35   ` Eric W. Biederman [this message]
2019-05-23  0:38 ` [REVIEW][PATCH 03/26] signal/arm64: Use force_sig not force_sig_fault for SIGKILL Eric W. Biederman
2019-05-23 10:17   ` Will Deacon
2019-05-23 14:59     ` Eric W. Biederman
2019-05-23 16:11     ` [REVIEW][PATCHv2 " Eric W. Biederman
2019-05-23 16:15       ` Will Deacon
2019-05-23 20:59         ` Eric W. Biederman
2019-05-24 10:00           ` Will Deacon
2019-05-24 22:36             ` Eric W. Biederman
2019-05-29 15:12               ` Will Deacon
2019-05-29 15:34                 ` Eric W. Biederman
2019-05-23 10:21   ` [REVIEW][PATCH " Dave Martin
2019-05-23 14:53     ` Eric W. Biederman
2019-05-23 16:12       ` Dave P Martin
2019-05-23 21:00         ` Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 04/26] signal/drbd: Use send_sig not force_sig Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 05/26] signal/bpfilter: Fix bpfilter_kernl to use " Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 06/26] signal/pid_namespace: Fix reboot_pid_ns " Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 07/26] signal/cifs: Fix cifs_put_tcp_session to call send_sig instead of force_sig Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 08/26] signal: Remove task parameter from force_sigsegv Eric W. Biederman
2019-05-23  0:38 ` [REVIEW][PATCH 09/26] signal: Remove task parameter from force_sig Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 10/26] signal: Remove task parameter from force_sig_mceerr Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 11/26] signal/x86: Remove task parameter from send_sigtrap Eric W. Biederman
2019-05-28 18:18   ` Thomas Gleixner
2019-05-23  0:39 ` [REVIEW][PATCH 12/26] signal/um: " Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 13/26] signal/sh: Remove tsk parameter from force_sig_info_fault Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 14/26] signal/riscv: Remove tsk parameter from do_trap Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 15/26] signal/nds32: Remove tsk parameter from send_sigtrap Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 16/26] signal/arm: Remove tsk parameter from ptrace_break Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 17/26] signal/arm: Remove tsk parameter from __do_user_fault Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 18/26] signal/unicore32: " Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 19/26] signal: Explicitly call force_sig_fault on current Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 20/26] signal: Use force_sig_fault_to_task for the two calls that don't deliver to current Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 21/26] signal: Remove the task parameter from force_sig_fault Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 22/26] signal: Properly set TRACE_SIGNAL_LOSE_INFO in __send_signal Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 23/26] signal: Move the computation of force into send_signal and correct it Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 24/26] signal: Generate the siginfo in force_sig Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 25/26] signal: Factor force_sig_info_to_task out of force_sig_info Eric W. Biederman
2019-05-23  0:39 ` [REVIEW][PATCH 26/26] signal: Remove the signal number and task parameters from force_sig_info Eric W. Biederman
2019-05-24 23:35 ` [REVIEW][PATCH 00/26] signal: Remove task argument " Eric W. Biederman
2019-05-29 15:37 ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87muj52use.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=containers@lists.linux-foundation.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --subject='Re: [REVIEW][PATCH 02/26] signal/ptrace: Simplify and fix PTRACE_KILL' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).