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