Re: [PATCH v2 1/8] exec: Correct comments about "point of no return"

[Kees Cook]

On Tue, Jul 18, 2017 at 6:12 AM, Eric W. Biederman
<ebiederm@xxxxxxxxxxxx> wrote:
> Kees Cook <keescook@xxxxxxxxxxxx> writes:
>
>> On Mon, Jul 10, 2017 at 10:07 AM, Eric W. Biederman
>> <ebiederm@xxxxxxxxxxxx> wrote:
>>> Kees Cook <keescook@xxxxxxxxxxxx> writes:
>>>
>>>> On Mon, Jul 10, 2017 at 1:46 AM, Eric W. Biederman
>>>> <ebiederm@xxxxxxxxxxxx> wrote:
>>>>>
>>>>> But you miss it.
>>>>>
>>>>> The "point of no return" is the call to de_thread.  Or aguably anything in
>>>>> flush_old_exec.  Once anything in the current task is modified you can't
>>>>> return an error.
>>>>>
>>>>> It very much does not have anything to do with brpm.    It has
>>>>> everything to do with current.
>>>>
>>>> Yes, but the thing that actually enforces this is the test of bprm->mm
>>>> and the SIGSEGV in search_binary_handlers().
>>>
>>> So what.  Calling that the point of no return is wrong.
>>>
>>> The point of no return is when we kill change anyting in signal_struct
>>> or task_struct.  AKA killing the first thread in de_thread.
>>
>> Well, okay, I think this is a semantic difference. Prior to bprm->mm
>> being NULL, there is still an error return path (yes?), though there
>> may have been side-effects (like de_thread(), as you say). But after
>> going NULL, the exec either succeeds or SEGVs. It is literally the
>> point of no "return".
>
> Nope.  The only exits out of de_thread without de_thread completing
> successfully are when we know the processes is already exiting
> (signal_group_exit) or when a fatal signal is pending
> (__fatal_signal_pending).  With a process exit already pending there is
> no need to send a separate signal.
>
> Quite seriously after exec starts having side effects on the process we may
> not return to userspace.
>
>>> It is more than just the SIGSEGV in search_binary_handlers that enforces
>>> this.  de_thread only returns (with a failure code) after having killed
>>> some threads if those threads are dead.
>>
>> This would still result in the exec-ing thread returning with that
>> error, yes?
>
> Nope.  The process dies before it gets to see the failure code.
>
>>> Similarly exec_mmap only returns with failure if we know that a core
>>> dump is pending, and as such the process will be killed before returning
>>> to userspace.
>>
>> Yeah, I had looked at this code and mostly decided it wasn't possible
>> for exec_mmap() to actually get its return value back to userspace.
>>
>>> I am a little worried that we may fail to dump some threads if a core
>>> dump races with exec, but that is a quality of implementation issue, and
>>> the window is very small so I don't know that it matters.
>>>
>>> The point of no return very much comes a while before clearing brpm->mm.
>>
>> I'm happy to re-write the comments, but I was just trying to document
>> the SEGV case, which is what that comment was originally trying to do
>> (and got lost in the various shuffles).
>
> My objection is you are misdocumenting what is going on.  If we are
> going to correct the comment let's correct the comment.
>
> The start of flush_old_exec is the point of no return.  Any errors after
> that point the process will never see.

Okay, I'll adjust it. Thanks!

-Kees

-- 
Kees Cook
Pixel Security