| Message ID | 87y2q25knl.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec: Trivial cleanups for exec | expand |
On Fri, May 08, 2020 at 01:47:10PM -0500, Eric W. Biederman wrote: > > Move the handing of the point of no return from search_binary_handler > into __do_execve_file so that it is easier to find, and to keep > things robust in the face of change. > > Make it clear that an existing fatal signal will take precedence over > a forced SIGSEGV by not forcing SIGSEGV if a fatal signal is already > pending. This does not change the behavior but it saves a reader > of the code the tedium of reading and understanding force_sig > and the signal delivery code. > > Update the comment in begin_new_exec about where SIGSEGV is forced. > > Keep point_of_no_return from being a mystery by documenting > what the code is doing where it forces SIGSEGV if the > code is past the point of no return. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> I had to read the code around these changes a bit carefully, but yeah, this looks like a safe cleanup. It is a behavioral change, though (in that in unmasks non-SEGV fatal signals), so I do wonder if something somewhere might notice this, but I'd agree that it's the more robust behavior. Reviewed-by: Kees Cook <keescook@chromium.org>
Kees Cook <keescook@chromium.org> writes: > On Fri, May 08, 2020 at 01:47:10PM -0500, Eric W. Biederman wrote: >> >> Move the handing of the point of no return from search_binary_handler >> into __do_execve_file so that it is easier to find, and to keep >> things robust in the face of change. >> >> Make it clear that an existing fatal signal will take precedence over >> a forced SIGSEGV by not forcing SIGSEGV if a fatal signal is already >> pending. This does not change the behavior but it saves a reader >> of the code the tedium of reading and understanding force_sig >> and the signal delivery code. >> >> Update the comment in begin_new_exec about where SIGSEGV is forced. >> >> Keep point_of_no_return from being a mystery by documenting >> what the code is doing where it forces SIGSEGV if the >> code is past the point of no return. >> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > > I had to read the code around these changes a bit carefully, but yeah, > this looks like a safe cleanup. It is a behavioral change, though (in > that in unmasks non-SEGV fatal signals), so I do wonder if something > somewhere might notice this, but I'd agree that it's the more robust > behavior. So the only behavioral change that I can see is that for non-SIGSEGV fatal signals the signal handler for SIGSEGV will not be set to SIG_DFL and SIGSEGV will not be removed from tasks local blocked signal set. I think there is a good case that behavior change is a bug. If you think that it was SIGSEGV that was being delivered and it was masking an other existing fatal you would be incorrect. If you look at: fatal_signal_pending - you will see that it tests for SIGKILL on the current's tasks queue. complete_signal() - you will see that when a fatal (non-coredumpable) signal is delvered it sets SIGKILL in every threads local queue. As well as setting SIGNAL_GROUP_EXIT get_signal - It special cases SIGNAL_GROUP_EXIT and fast forwards to the end. So that a signal that has been delivered can not be overriden by another signal. __send_signal - It tests SIGNAL_GROUP_EXIT and if it is set gets out early (which applies to force_sigsegv amoung others) So unless it is de_thread or coredumping that sets the task local SIGKILL there is no chance for a force SIGSEGV to do antyhing, and the code has already tested for those to in de_thread and exit_mmap before point_of_no_return is set. So except for the SIGSEGV handler and blocked state there are no behavior changes. That does takes some reading through all of that code to see what is going on, and just saying !fatal_signal_pending makes it all so much clearer. In the next patch when I move setting point_of_no_return earlier that fatal_signal_pending check ensures that we don't stomp on de_thread or coredump state with force_sigsegv(SIGSEGV). But again that also won't be a change in behavior, as we aren't performing the force_sigsegv test when it could be either of those things until that patch. So force_sigsegv never gets a chance to stomp on those cases. Eric
diff --git a/fs/exec.c b/fs/exec.c index 15682a1dfee9..443eb960f9a0 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1329,8 +1329,8 @@ int begin_new_exec(struct linux_binprm * bprm) /* * With the new mm installed it is completely impossible to * fail and return to the original process. If anything from - * here on returns an error, the check in - * search_binary_handler() will SEGV current. + * here on returns an error, the check in __do_execve_file() + * will SEGV current. */ bprm->point_of_no_return = true; bprm->mm = NULL; @@ -1722,13 +1722,8 @@ int search_binary_handler(struct linux_binprm *bprm) read_lock(&binfmt_lock); put_binfmt(fmt); - if (retval < 0 && bprm->point_of_no_return) { - /* we got to flush_old_exec() and failed after it */ - read_unlock(&binfmt_lock); - force_sigsegv(SIGSEGV); - return retval; - } - if (retval != -ENOEXEC || !bprm->file) { + if (bprm->point_of_no_return || !bprm->file || + (retval != -ENOEXEC)) { read_unlock(&binfmt_lock); return retval; } @@ -1899,6 +1894,14 @@ static int __do_execve_file(int fd, struct filename *filename, return retval; out: + /* + * If past the point of no return ensure the the code never + * returns to the userspace process. Use an existing fatal + * signal if present otherwise terminate the process with + * SIGSEGV. + */ + if (bprm->point_of_no_return && !fatal_signal_pending(current)) + force_sigsegv(SIGSEGV); if (bprm->mm) { acct_arg_size(bprm, 0); mmput(bprm->mm);
Move the handing of the point of no return from search_binary_handler into __do_execve_file so that it is easier to find, and to keep things robust in the face of change. Make it clear that an existing fatal signal will take precedence over a forced SIGSEGV by not forcing SIGSEGV if a fatal signal is already pending. This does not change the behavior but it saves a reader of the code the tedium of reading and understanding force_sig and the signal delivery code. Update the comment in begin_new_exec about where SIGSEGV is forced. Keep point_of_no_return from being a mystery by documenting what the code is doing where it forces SIGSEGV if the code is past the point of no return. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)