From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) To: Kees Cook Cc: Linus Torvalds , Andy Lutomirski , David Howells , Serge Hallyn , John Johansen , Casey Schaufler , Alexander Viro , Michal Hocko , Ben Hutchings , Hugh Dickins , Oleg Nesterov , "Jason A. Donenfeld" , Rik van Riel , James Morris , Greg Ungerer , Ingo Molnar , Nicolas Pitre , Stephen Smalley , Paul Moore , Vivek Goyal , =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= , Tetsuo Handa , "linux-fsdevel\@vger.kernel.org" , LKML , "" , SE Linux References: <1499673451-66160-1-git-send-email-keescook@chromium.org> <1499673451-66160-2-git-send-email-keescook@chromium.org> <87van0r86d.fsf@xmission.com> Date: Mon, 10 Jul 2017 12:07:06 -0500 In-Reply-To: (Kees Cook's message of "Mon, 10 Jul 2017 09:04:15 -0700") Message-ID: <87pod8mdad.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [PATCH v2 1/8] exec: Correct comments about "point of no return" List-Id: "Security-Enhanced Linux \(SELinux\) mailing list" List-Post: List-Help: Kees Cook writes: > On Mon, Jul 10, 2017 at 1:46 AM, Eric W. Biederman > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) Subject: Re: [PATCH v2 1/8] exec: Correct comments about "point of no return" Date: Mon, 10 Jul 2017 12:07:06 -0500 Message-ID: <87pod8mdad.fsf@xmission.com> References: <1499673451-66160-1-git-send-email-keescook@chromium.org> <1499673451-66160-2-git-send-email-keescook@chromium.org> <87van0r86d.fsf@xmission.com> Mime-Version: 1.0 Content-Type: text/plain Cc: Nicolas Pitre , "Jason A. Donenfeld" , Andy Lutomirski , Tetsuo Handa , Michal Hocko , David Howells , SE Linux , Ingo Molnar , Hugh Dickins , Greg Ungerer , Stephen Smalley , Vivek Goyal , Rik van Riel , "linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Alexander Viro , James Morris , =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= , John Johansen , Ben Hutchings , Oleg Nesterov Return-path: In-Reply-To: (Kees Cook's message of "Mon, 10 Jul 2017 09:04:15 -0700") List-Post: List-Help: Errors-To: selinux-bounces-+05T5uksL2qpZYMLLGbcSA@public.gmane.org Sender: "Selinux" List-Id: linux-fsdevel.vger.kernel.org Kees Cook writes: > On Mon, Jul 10, 2017 at 1:46 AM, Eric W. Biederman > 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