linux-kernel - Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170222174102.GA19788@redhat.com>
Date:   Wed, 22 Feb 2017 18:41:03 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        Aleksa Sarai <asarai@...e.com>,
        Andy Lutomirski <luto@...capital.net>,
        Attila Fazekas <afazekas@...hat.com>,
        Jann Horn <jann@...jh.net>, Kees Cook <keescook@...omium.org>,
        Michal Hocko <mhocko@...nel.org>,
        Ulrich Obergfell <uobergfe@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 1/2] exec: don't wait for zombie threads with
        cred_guard_mutex held

On 02/22, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@...hat.com> writes:
>
> >> Reducing the
> >> scope of cred_guard_mutex concerns me.  There appear to be some fields
> >> like sighand that we currently expose in proc
> >
> > please see another email, collect_sigign_sigcatch() is called without this
> > mutex.
>
> I agree that it is called without the mutex.  It is not clear to me that
> is the correct behavior.

I fail to understand how/why this can be wrong.

> >> Do you know if we can make cred_guard_mutex a per-task lock again?
> >
> > I think we can, but this needs some (afaics simple) changes too.
> >
> > But for what? Note that the problem fixed by this series won't go away
> > if we do this.
>
> I believe it will if the other waiters use mutex_lock_killable.

No. They already use mutex_lock_killable/interruptible. And the test-case
can be killed, it is not the hard-lockup.

> I really don't like the first patch.

Just in case, I don't really like it too. Simply because it makes execve
more complex, we need to wait for sub-threads twice.

> It makes an information leak part
> a required detail of the implementation and as such possibly something
> we can never change.

Again, I simply can't understand how flush_signal_handlers() outside of
cred_guard_mutex can be treated as information leak. Even _if_
collect_sigign_sigcatch() was called with this mutex held.

Or do you mean something else?

> I suspect that a good fix that respects that proc and ptrace_attach need
> to exclude the setuid exec case for semantic reasons would have a similar
> complexity.

I am not sure I understand how we can do this. We need cred_guard_mutex
or something else even if exec is not setuid and does not change the
credentials, an LSM module can nack exec-under-ptrace by any reason.

> I think fixing the deadlock is important.

Yes. People actually hit this bug, it was reported several times.

> Right now it feels like your fix in patch 1 makes things a bit more
> brittle and I don't like that at all.

See above, I am not proud of this change too. I even mentioned on 0/2
that it would be nice to reconsider this change in the long term.

But I do not see another simple and _backportable_ solution for now.

What do you think we can do instead for stable trees?

Oleg.