Re: [PATCH v3 1/8] exec: introduce cred_guard_light

From: Jann Horn <jann@thejh.net>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Roland McGrath <roland@hack.frob.com>,
	John Johansen <john.johansen@canonical.com>,
	James Morris <james.l.morris@oracle.com>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Paul Moore <aul@paul-moore.com>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	Eric Paris <eparis@parisplace.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Janis Danisevskis <jdanis@google.com>,
	Seth Forshee <seth.forshee@canonical.com>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Benjamin LaHaise <bcrl@kvack.org>,
	Ben Hutchings <ben@decadent.org.uk>,
	Andy Lutomirski <luto@amacapital.net>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Krister Johansen <kjlx@templeofstupid.com>,
	linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, security@kernel.org
Subject: Re: [PATCH v3 1/8] exec: introduce cred_guard_light
Date: Thu, 3 Nov 2016 22:17:04 +0100	[thread overview]
Message-ID: <20161103211704.GO8196@pc.thejh.net> (raw)
In-Reply-To: <20161103181225.GA11212@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]

On Thu, Nov 03, 2016 at 07:12:25PM +0100, Oleg Nesterov wrote:
> On 11/02, Jann Horn wrote:
> >
> > On Wed, Nov 02, 2016 at 07:18:06PM +0100, Oleg Nesterov wrote:
> > > On 10/30, Jann Horn wrote:
[...]
> > I believe that it should be possible to convert most existing users of the
> > cred_guard_mutex to the new cred_guard_light - exceptions to that that I
> > see are:
> >
> >  - PTRACE_ATTACH
> 
> This is the main problem afaics. So "strace -f" can hang if it races
> with mt-exec. And we need to fix this. I constantly forget about this
> problem, but I tried many times to find a reasonable solution, still
> can't.

Ah, okay, it wasn't clear to me that you consider the race with
PTRACE_ATTACH to be a similarly big problem as the other ones.

> IMO, it would be nice to rework the lsm hooks, so that we could take
> cred_guard_mutex after de_thread() (like your cred_guard_light) or
> at least drop it earlier, but unlikely this is possible...

An idea: Maybe we can change the LSM hook so that, immediately before
de_thread(), the LSMs can handle the execve() based on the current
state and report the circumstances under which they would deny the
execution or treat it differently. Then, during de_thread(), we can
immediately reject any access that would have changed the execution
and immediately permit any access that wouldn't have.

This could theoretically cause userland to see spurious permission
denials, but only if an LSM has an inconsistent security policy where
some access degrades execution although it would have been permitted
after a normal execution.

Does that make sense?

> So the only plan I currently have is change de_thread() to wait until
> other threads pass exit_notify() or even exit_signals(), but I don't
> like this.
[...]
> My point is, imo you should not add the new mutex. Just use the old
> one in (say) 4/8 (which I do not personally like as you know ;), this
> won't add the new problem.

Okay, I'll do that.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]