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

[ebiederm@xmission.com (Eric W. Biederman)]

Kees Cook <keescook@chromium.org> writes:

> On Mon, Jul 10, 2017 at 1:46 AM, Eric W. Biederman
> <ebiederm@xmission.com> 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.

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.

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.

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.

>>> diff --git a/fs/exec.c b/fs/exec.c
>>> index 904199086490..7842ae661e34 100644
>>> --- a/fs/exec.c
>>> +++ b/fs/exec.c
>>> @@ -1285,7 +1285,14 @@ int flush_old_exec(struct linux_binprm * bprm)
>>>       if (retval)
>>>               goto out;
>>>
>>> -     bprm->mm = NULL;                /* We're using it now */
>>> +     /*
>>> +      * After clearing bprm->mm (to mark that current is using the
>>> +      * prepared mm now), we are at the point of no return. If
>>> +      * anything from here on returns an error, the check in
>>> +      * search_binary_handler() will kill current (since the mm has
>>> +      * been replaced).
>>> +      */
>>> +     bprm->mm = NULL;
>>>
>>>       set_fs(USER_DS);
>>>       current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |

Eric