| Message ID | 1499673451-66160-2-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| 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
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
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(-)