linux-kernel - Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1

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: <CAHk-=whM3r7zrm8mSi7HJhuZbYiXx9PFU5VQYeKm6Low=r15eQ@mail.gmail.com>
Date:   Thu, 2 Apr 2020 12:52:50 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Bernd Edlinger <bernd.edlinger@...mail.de>
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexey Gladkov <gladkov.alexey@...il.com>
Subject: Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1

On Thu, Apr 2, 2020 at 12:31 PM Bernd Edlinger
<bernd.edlinger@...mail.de> wrote:
>
> This is at least what is my impression how the existing mutexes are used,
> a mutex called "cred_guard_mutex" is a not very good self explaining name,
> in my opinion, it is totally unclear what it does "guard", and why.

Oh, I absolutely agree that cred_guard_mutex is a horrible lock.

It actually _used_ to be a lot more understandable, and the name used
to make more sense in the context it was used.

See commit

  a2a8474c3fff ("exec: do not sleep in TASK_TRACED under ->cred_guard_mutex")

for when it changed from "somewhat understandable" to "really hard to follow".

Don't get me wrong - that commit has a very good reason for it, but it
does make the locking really hard to understand.

It all used to be in one function - do_execve() - and it was holding
the lock over a fairly obvious range, starting at

    bprm->cred = prepare_exec_creds();

and ending at basically "we're done with execve()".

So basically, cred_guard_mutex ends up being the thing that is held
all the way from the "before execve looks at the old creds" to "execve
is done, and has changed the creds".

The reason it's needed is exactly that there are some nasty situations
where execve() itself does things with creds to determine that the new
creds are ok. And it uses the old creds to do that, but it also uses
the task->flags and task->ptrace.

So think of cred_guard_mutex as a lock around not just the creds, but
the combination of creds and the task flags/ptrace.

Anybody who changes the task ptrace setting needs to serialize with
execve(). Or anybody who tests for "dumpable()", for example.

If *all* you care about is just the creds, then you don't need it.
It's really only users that do more checks than just credentials.
"dumpable()" is I think the common one.

And that's why cred_guard_mutex has that big range - it starts when we
read the original creds (because it will use those creds to determine
how the *new* creds will affect dumpability etc), and it ends when it
has updated not only to the new creds, but it has set all those other
flags too.

So I'm not at all against splitting the lock up, and trying to make it
more directed and specific.

My complaints were about how the new lock wasn't much better. It was
still completely incomprehensible, the conditional unlocking was hard
to follow, and it really wasn't obvious that the converted users were
fine.

See?

               Linus