Re: [PATCH 1/6] signal: Remove the bogus sigkill_pending in ptrace_stop

[Kees Cook]

On Thu, Sep 23, 2021 at 07:09:34PM -0500, Eric W. Biederman wrote:
> 
> The existence of sigkill_pending is a little silly as it is
> functionally a duplicate of fatal_signal_pending that is used in
> exactly one place.

sigkill_pending() checks for &tsk->signal->shared_pending.signal but
fatal_signal_pending() doesn't.

> Checking for pending fatal signals and returning early in ptrace_stop
> is actively harmful.  It casues the ptrace_stop called by
> ptrace_signal to return early before setting current->exit_code.
> Later when ptrace_signal reads the signal number from
> current->exit_code is undefined, making it unpredictable what will
> happen.
> 
> Instead rely on the fact that schedule will not sleep if there is a
> pending signal that can awaken a task.

This reasoning sound fine, but I can't see where it's happening.
It looks like recalc_sigpending() is supposed to happen at the start
of scheduling? I see it at the end of ptrace_stop(), though, so it looks
like it's reasonable to skip checking shared_pending.

(Does the scheduler deal with shared_pending directly?)

> Removing the explict sigkill_pending test fixes fixes ptrace_signal
> when ptrace_stop does not stop because current->exit_code is always
> set to to signr.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 3d749b9e676b ("ptrace: simplify ptrace_stop()->sigkill_pending() path")
> Fixes: 1a669c2f16d4 ("Add arch_ptrace_stop")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> ---
>  kernel/signal.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 952741f6d0f9..9f2dc9cf3208 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2182,15 +2182,6 @@ static inline bool may_ptrace_stop(void)
>  	return true;
>  }
>  
> -/*
> - * Return non-zero if there is a SIGKILL that should be waking us up.
> - * Called with the siglock held.
> - */
> -static bool sigkill_pending(struct task_struct *tsk)
> -{
> -	return sigismember(&tsk->pending.signal, SIGKILL) ||
> -	       sigismember(&tsk->signal->shared_pending.signal, SIGKILL);
> -}
>  
>  /*
>   * This must be called with current->sighand->siglock held.
> @@ -2217,17 +2208,16 @@ static void ptrace_stop(int exit_code, int why, int clear_code, kernel_siginfo_t
>  		 * calling arch_ptrace_stop, so we must release it now.
>  		 * To preserve proper semantics, we must do this before
>  		 * any signal bookkeeping like checking group_stop_count.
> -		 * Meanwhile, a SIGKILL could come in before we retake the
> -		 * siglock.  That must prevent us from sleeping in TASK_TRACED.
> -		 * So after regaining the lock, we must check for SIGKILL.

Where is the sleep this comment is talking about?

i.e. will recalc_sigpending() have been called before the above sleep
would happen? I assume it's after ptrace_stop() returns... But I want to
make sure the sleep isn't in ptrace_stop() itself somewhere I can't see.
I *do* see freezable_schedule() called, and that dumps us into
__schedule(), and I don't see a recalc before it checks
signal_pending_state().

Does a recalc need to happen in plce of the old sigkill_pending()
call?

-- 
Kees Cook