On Mon, Jul 10, 2017 at 10:07 AM, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote: > 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. 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 Pixel Security
