linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	linux-api@vger.kernel.org
Subject: Re: [PATCH 1/6] signal: Remove the bogus sigkill_pending in ptrace_stop
Date: Fri, 24 Sep 2021 10:48:18 -0500	[thread overview]
Message-ID: <87tuiaotz1.fsf@disp2133> (raw)
In-Reply-To: <202109240804.BC44773A@keescook> (Kees Cook's message of "Fri, 24 Sep 2021 08:22:20 -0700")

Kees Cook <keescook@chromium.org> writes:

> 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.

The extra test is unnecessary as all SIGKILL's visit complete_signal
immediately run the loop:

			/*
			 * Start a group exit and wake everybody up.
			 * This way we don't have other threads
			 * running and doing things after a slower
			 * thread has the fatal signal pending.
			 */
			signal->flags = SIGNAL_GROUP_EXIT;
			signal->group_exit_code = sig;
			signal->group_stop_count = 0;
			t = p;
			do {
				task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
				sigaddset(&t->pending.signal, SIGKILL);
				signal_wake_up(t, 1);
			} while_each_thread(p, t);
			return;

Which sets SIGKILL in the task specific queue.  Which means only the
non-shared queue needs to be tested.  Further fatal_signal_pending would
be buggy if this was not the case.

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

In the call of signal_pending_state from kernel/core/.c:__schedule().

ptrace_stop would actually be badly broken today if that was not the
case as several places enter into ptrace_event without testing signals
first.

>> 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@vger.kernel.org
>> Fixes: 3d749b9e676b ("ptrace: simplify ptrace_stop()->sigkill_pending() path")
>> Fixes: 1a669c2f16d4 ("Add arch_ptrace_stop")
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  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?

You read that correctly freezable_schedule is where ptrace_stop sleeps.

The call chain you are looking for looks something like:
send_signal
  complete_signal
     signal_wake_up
       signal_wake_up_state
         set_tsk_thread_flag(t, TIF_SIGPENDING)

That is to say complete_signal sets TIF_SIGPENDING and
the per task siqueue SIGKILL entry.

Calling recalc_sigpending is only needed when a signal is removed from
the queues, not when a signal is added.

Eric

  reply	other threads:[~2021-09-24 15:48 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-24  0:08 [PATCH 0/6] per signal_struct coredumps Eric W. Biederman
2021-09-24  0:09 ` [PATCH 1/6] signal: Remove the bogus sigkill_pending in ptrace_stop Eric W. Biederman
2021-09-24 15:22   ` Kees Cook
2021-09-24 15:48     ` Eric W. Biederman [this message]
2021-09-24 19:06       ` Kees Cook
2021-09-24  0:10 ` [PATCH 2/6] ptrace: Remove the unnecessary arguments from arch_ptrace_stop Eric W. Biederman
2021-09-24 15:26   ` Kees Cook
2021-09-24  0:10 ` [PATCH 3/6] exec: Check for a pending fatal signal instead of core_state Eric W. Biederman
2021-09-24 15:38   ` Kees Cook
2021-09-24  0:11 ` [PATCH 4/6] exit: Factor coredump_exit_mm out of exit_mm Eric W. Biederman
2021-09-24 18:28   ` Kees Cook
2021-09-24  0:11 ` [PATCH 5/6] coredump: Don't perform any cleanups before dumping core Eric W. Biederman
2021-09-24 18:51   ` Kees Cook
2021-09-24 21:28     ` Eric W. Biederman
2021-09-24 21:41       ` Kees Cook
2021-09-24  0:12 ` [PATCH 6/6] coredump: Limit coredumps to a single thread group Eric W. Biederman
2021-09-24 18:56   ` Kees Cook
2021-10-06 17:03     ` Eric W. Biederman
2021-11-19 16:03   ` Kyle Huey
2021-11-19 17:38     ` Eric W. Biederman
2021-09-24  5:59 ` [PATCH 0/6] per signal_struct coredumps Kees Cook
2021-09-24 14:00   ` Eric W. Biederman
2021-09-24 15:22 ` [PATCH 2/6] ptrace: Remove the unnecessary arguments from arch_ptrace_stop 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=87tuiaotz1.fsf@disp2133 \
    --to=ebiederm@xmission.com \
    --cc=keescook@chromium.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).