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-=wjhZ4uS6XKxJdV0MZO1AN-vhvVmeSeGFSntk1GjpyPwjg@mail.gmail.com>
Date:   Thu, 30 Apr 2020 09:40:44 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Bernd Edlinger <bernd.edlinger@...mail.de>
Cc:     Jann Horn <jannh@...gle.com>, Oleg Nesterov <oleg@...hat.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Waiman Long <longman@...hat.com>,
        Ingo Molnar <mingo@...nel.org>, Will Deacon <will@...nel.org>,
        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 30, 2020 at 7:29 AM Bernd Edlinger
<bernd.edlinger@...mail.de> wrote:
>
> Ah, now I see, that was of course not the intended effect,
> but that is not where the pseudo-deadlock happens at all,
> would returning -RESTARTNOINTR in this function make this
> patch acceptable, it will not have an effect on the test case?

So that was why I suggested doing it all with a helper function, and
also doing that

        set_thread_flag(TIF_SIGPENDING);

because without going through the "check-for-signals" code at return
to user space, -ERESTARTNOINTR doesn't actually _do_ any restart.

However, the more I looked at it, the less I actually liked that hack.

Part of it is simply because it can cause the exact same problem that
ptrace() does (at least in theory). And even if you don't get the
livelock thing, you can get the "use 100% CPU time" thing, because if
that case ever triggers, and we re-try, it will generally just _keep_
on triggering (think "execve is waiting for a zombie, nobody is
reaping it").

IOW, restarting doesn't really fix the problem, or guarantee any
forward progress.

So I'd have been ok with your "unsafe_exec_flag" if

 (a) it had been done in one place with a helper function.

 (b) it would _only_ trigger for ptrace (and perhaps seccomp).

but I don't think it works for that write() case.

That said, I'm not 100% convinced that that write() case really even
needs that cred_guard_mutex (renamed or not).

Maybe we can introduce a new mutex just against concurrent ptrace
(which is what at least the _comment_ says_ that
security_setprocattr() wants - I didn't check the actual low-level
security code).

So maybe that proc_pid_attr_write() case could be done some other way entirely.

Th emore we go through all this, the more I really think that Oleg's
patch to just delay the waiting for things until after dropping the
mutex in execve() is the way to go.

Is it a "simple" and small patch? No. But it really addresses the core
issue, without introducing new odd rules or special cases, or making a
lock that doesn't reliably work as a lock.

                      Linus