linux-kernel - Re: [PATCH v2 1/8] exec: Correct comments about "point of no return"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGXu5j+_OJ88iJVDOyWoW8JgcUoGc2kWVCSh3hfxocMfpbsmgw@mail.gmail.com>
Date:   Tue, 18 Jul 2017 06:42:01 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        David Howells <dhowells@...hat.com>,
        Serge Hallyn <serge@...lyn.com>,
        John Johansen <john.johansen@...onical.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Michal Hocko <mhocko@...nel.org>,
        Ben Hutchings <ben@...adent.org.uk>,
        Hugh Dickins <hughd@...gle.com>,
        Oleg Nesterov <oleg@...hat.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Rik van Riel <riel@...hat.com>,
        James Morris <james.l.morris@...cle.com>,
        Greg Ungerer <gerg@...ux-m68k.org>,
        Ingo Molnar <mingo@...nel.org>,
        Nicolas Pitre <nicolas.pitre@...aro.org>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        Paul Moore <paul@...l-moore.com>,
        Vivek Goyal <vgoyal@...hat.com>,
        Mickaël Salaün <mic@...ikod.net>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "<linux-security-module@...r.kernel.org>" 
        <linux-security-module@...r.kernel.org>,
        SE Linux <selinux@...ho.nsa.gov>
Subject: Re: [PATCH v2 1/8] exec: Correct comments about "point of no return"

On Tue, Jul 18, 2017 at 6:12 AM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
> Kees Cook <keescook@...omium.org> writes:
>
>> On Mon, Jul 10, 2017 at 10:07 AM, Eric W. Biederman
>> <ebiederm@...ssion.com> wrote:
>>> Kees Cook <keescook@...omium.org> writes:
>>>
>>>> On Mon, Jul 10, 2017 at 1:46 AM, Eric W. Biederman
>>>> <ebiederm@...ssion.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

-- 
Kees Cook
Pixel Security