[REVIEW,02/26] signal/ptrace: Simplify and fix PTRACE_KILL

Submitted by Eric W. Biederman on May 23, 2019, 12:38 a.m.

Details

Message ID 20190523003916.20726-3-ebiederm@xmission.com
State New
Series "signal: Remove task argument from force_sig_info"
Headers show

Commit Message

Eric W. Biederman May 23, 2019, 12:38 a.m.
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(-)

Patch hide | download patch | download mbox

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);

Comments

Eric W. Biederman May 29, 2019, 2:35 p.m.
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);