| Message ID | 1499673451-66160-2-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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
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(). -Kees > > >> 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
On Mon, Jul 10, 2017 at 10:07 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > 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. 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". > 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? > 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). -Kees
Kees Cook <keescook@chromium.org> writes: > On Mon, Jul 10, 2017 at 10:07 AM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> 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. > > 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. Eric
On Tue, Jul 18, 2017 at 6:12 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Kees Cook <keescook@chromium.org> writes: > >> On Mon, Jul 10, 2017 at 10:07 AM, Eric W. Biederman >> <ebiederm@xmission.com> wrote: >>> 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. >> >> 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
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 | @@ -1332,7 +1339,6 @@ void setup_new_exec(struct linux_binprm * bprm) { arch_pick_mmap_layout(current->mm); - /* This is the point of no return */ current->sas_ss_sp = current->sas_ss_size = 0; if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid())) @@ -1350,7 +1356,6 @@ void setup_new_exec(struct linux_binprm * bprm) */ current->mm->task_size = TASK_SIZE; - /* install the new credentials */ if (!uid_eq(bprm->cred->uid, current_euid()) || !gid_eq(bprm->cred->gid, current_egid())) { current->pdeath_signal = 0;
In commit 221af7f87b97 ("Split 'flush_old_exec' into two functions"), the comment about the point of no return should have stayed in flush_old_exec() since it refers to "bprm->mm = NULL;" line, but prior changes in commits c89681ed7d0e ("remove steal_locks()"), and fd8328be874f ("sanitize handling of shared descriptor tables in failing execve()") made it look like it meant the current->sas_ss_sp line instead. The comment is referring to the fact that once bprm->mm is NULL, all failures from a binfmt load_binary hook (e.g. load_elf_binary), will get SEGV raised against current. Move this comment and expand the explanation a bit, putting it above the assignment this time. This also removes an erroneous commet about when credentials are being installed. That has its own dedicated function, install_exec_creds(), which carries a similar (and correct) comment, so remove the bogus comment where installation is not actually happening. Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/exec.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)