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@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: b72c186999e6 ("ptrace: fix race between ptrace_resume() and wait_task_stopped()")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> ---
> 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);
