Re: [PATCH 5/6] exec: Move handling of the point of no return to the top level

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>, Jann Horn <jannh@google.com>,
	Greg Ungerer <gerg@linux-m68k.org>, Rob Landley <rob@landley.net>,
	Bernd Edlinger <bernd.edlinger@hotmail.de>,
	linux-fsdevel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 5/6] exec: Move handling of the point of no return to the top level
Date: Sat, 09 May 2020 08:39:16 -0500	[thread overview]
Message-ID: <874ksp448r.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <202005082228.5C0E44CC6@keescook> (Kees Cook's message of "Fri, 8 May 2020 22:31:06 -0700")

Kees Cook <keescook@chromium.org> writes:

> On Fri, May 08, 2020 at 01:47:10PM -0500, Eric W. Biederman wrote:
>> 
>> Move the handing of the point of no return from search_binary_handler
>> into __do_execve_file so that it is easier to find, and to keep
>> things robust in the face of change.
>> 
>> Make it clear that an existing fatal signal will take precedence over
>> a forced SIGSEGV by not forcing SIGSEGV if a fatal signal is already
>> pending.  This does not change the behavior but it saves a reader
>> of the code the tedium of reading and understanding force_sig
>> and the signal delivery code.
>> 
>> Update the comment in begin_new_exec about where SIGSEGV is forced.
>> 
>> Keep point_of_no_return from being a mystery by documenting
>> what the code is doing where it forces SIGSEGV if the
>> code is past the point of no return.
>> 
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> I had to read the code around these changes a bit carefully, but yeah,
> this looks like a safe cleanup. It is a behavioral change, though (in
> that in unmasks non-SEGV fatal signals), so I do wonder if something
> somewhere might notice this, but I'd agree that it's the more robust
> behavior.

So the only behavioral change that I can see is that for non-SIGSEGV
fatal signals the signal handler for SIGSEGV will not be set to SIG_DFL
and SIGSEGV will not be removed from tasks local blocked signal set.

I think there is a good case that behavior change is a bug.

If you think that it was SIGSEGV that was being delivered and
it was masking an other existing fatal you would be incorrect.

If you look at:

fatal_signal_pending - you will see that it tests for SIGKILL on the
current's tasks queue.

complete_signal() - you will see that when a fatal (non-coredumpable)
signal is delvered it sets SIGKILL in every threads local queue.  As
well as setting SIGNAL_GROUP_EXIT

get_signal - It special cases SIGNAL_GROUP_EXIT and fast forwards
to the end.  So that a signal that has been delivered can not be
overriden by another signal.

__send_signal - It tests SIGNAL_GROUP_EXIT and if it is set
gets out early (which applies to force_sigsegv amoung others)

So unless it is de_thread or coredumping that sets the task
local SIGKILL there is no chance for a force SIGSEGV to do antyhing,
and the code has already tested for those to in de_thread and
exit_mmap before point_of_no_return is set.

So except for the SIGSEGV handler and blocked state there are no
behavior changes.

That does takes some reading through all of that code to see what is
going on, and just saying !fatal_signal_pending makes it all so much
clearer.

In the next patch when I move setting point_of_no_return earlier that
fatal_signal_pending check ensures that we don't stomp on de_thread or
coredump state with force_sigsegv(SIGSEGV).  But again that also won't
be a change in behavior, as we aren't performing the force_sigsegv test
when it could be either of those things until that patch.  So
force_sigsegv never gets a chance to stomp on those cases.

Eric