Message-ID: <87fuhpjeco.fsf_-_@xmission.com>
Date:   Mon, 03 Apr 2017 17:49:59 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        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, linux-api@...r.kernel.org
Subject: scope of cred_guard_mutex.

Oleg Nesterov <oleg@...hat.com> writes:

> On 04/02, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@...hat.com> writes:
>>
>> > Anyway, Eric, even if we can and want to do this, why we can't do this on
>> > top of my fix?
>>
>> Because your reduction in scope of cred_guard_mutex is fundamentally
>> broken and unnecessary.
>
> And you never explained why it is wrong, or I failed to understand you.
>
>> > I simply fail to understand why you dislike it that much. Yes it is not
>> > pretty, I said this many times, but it is safe in that it doesn't really
>> > change the current behaviour.
>>
>> No it is not safe.  And it promotes wrong thinking which is even more
>> dangerous.
>
> So please explain why it is not safe and why it is dangerous.
>
> Just in case, if you mean flush_signal_handlers() outside of cred_guard_mutex,
> please explain what I have missed in case you still think this is wrong.

>> I reviewed the code and cred_guard_mutex needs to cover what it covers.
>
> I strongly, strongly disagree. Its scope is unnecessary huge, we should narrow
> it in any case, even if the current code was not bugy. But this is almost
> offtopic, lets discuss this separately.

You have asked why I have problems with your patch and so I am going to
try to explain.  Partly I want to see a clean set of patches that we
can merge into Linus's tree before we make any compromises.  Because the
work preparing a clean patchset may inform us of something better.  Plus
we need to make something clean and long term maintainable in any event.

Partly I object because your understanding and my understanding of
cred_guard_mutex are very different.

As I read and understand the code the job of cred_guard_mutex is to keep
ptrace (and other threads of the proccess) from interferring in
exec and to ensure old resources are accessed with permission checks
using our original credentials and that new and modified resources are
accessed with permission checks using our new credentials.


I object to your patch in particular because you deliberately mess up
the part of only making old resources available with old creds and
new resources available with new creds.  Even if the current permission
checks are a don't care it still remains conceptually wrong.  And
conceptually wrong tends code tends towards maintenance problems
and real surprises when someone makes small changes to the code.  Which
is what I mean when I say your patch is dangerous.


AKA What I see neededing to be protected looks something like:
	mutex_lock();
        new_cred = compute_new_cred(tsk);
        new_mm = compute_new_mm(tsk);
        tsk->mm = new_mm;
        tsk->cred = new_cred;
        zap_other_threads(tsk);
        update_sighand(tsk);
        update_signal(tsk);
        do_close_on_exec();
        update_tsk_fields(tsk);
        mutex_unlock();

The only way I can see of reducing the scope of cred_guard_mutex is
performing work in such a way that ptrace and the other threads can't
interfere and then taking the lock.  Computing the new mm and the new
credentials are certainly candidates for that kind of treatment.

Eric

Powered by blists - more mailing lists