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: <20170220155005.GA15036@redhat.com>
Date:   Mon, 20 Feb 2017 16:50:06 +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, David Howells <dhowells@...hat.com>
Subject: Re: [PATCH V2 1/2] exec: don't wait for zombie threads with
        cred_guard_mutex held

On 02/17, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@...hat.com> writes:
>
> > de_thread() waits for other threads with ->cred_guard_mutex held and this
> > is really bad because the time is not bounded, debugger can delay the exit
> > and this lock has a lot of users (mostly abusers imo) in fs/proc and more.
> > And this leads to deadlock if debugger tries to take the same mutex:
>
> Oleg.  I looked at the history in proc of users of cred_guard_mutex
> and the proc users are grabbing cred_guard_mutex for the proper
> semantic reasons.  To avoid races with setuid exec that could result in
> an information disclosure.

This is clear. However I really think it is over-used. For example, I do
think that lock_trace() should die.

OK, of course I can be wrong. And in any case this is almost off-topic
right now, lets discuss this separately.

> On that score I believe we can incrementally approach that point and
> only grab the cred_guard_mutex in exec if we are performing an exec that
> changes the processes credentials.

May be... but afaics this is more complicated because of LSM hooks.
To me one the main problems is that lsm hook can fail if the task is
traced, that is why we can't simply take cred_guard_mutex and check
->ptrace after de_thread(). But again, lets discuss this later.

> Right now I don't think it introduces any new security information
> disclosures but the moving of flush_signal_handlers outside of
> cred_guard_mutex feels wrong.

Why?

Just in case, we can do flush_signal_handlers() only after we unshare
->sighand, that is why it was moved outside of cred_guard_mutex.

But why this is bad? collect_sigign_sigcatch() is called without this
mutex, who else can look at sa.sa_handler?

Oleg.