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

From: Ben Hutchings <ben@decadent.org.uk>
To: Jann Horn <jann@thejh.net>, 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>,
	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: Wed, 02 Nov 2016 15:38:53 -0600	[thread overview]
Message-ID: <1478122733.29107.1.camel@decadent.org.uk> (raw)
In-Reply-To: <20161102205011.GF8196@pc.thejh.net>

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

On Wed, 2016-11-02 at 21:50 +0100, Jann Horn wrote:
> On Wed, Nov 02, 2016 at 07:18:06PM +0100, Oleg Nesterov wrote:
> > On 10/30, Jann Horn wrote:
> > > 
> > > This is a new per-threadgroup lock that can often be taken instead of
> > > cred_guard_mutex and has less deadlock potential. I'm doing this because
> > > Oleg Nesterov mentioned the potential for deadlocks, in particular if a
> > > debugged task is stuck in execve, trying to get rid of a ptrace-stopped
> > > thread, and the debugger attempts to inspect procfs files of the debugged
> > > task.
> > 
> > 
> > Yes, but let me repeat that we need to fix this anyway. So I don't really
> > understand why should we add yet another mutex.
> 
> 
> execve() only takes the new mutex immediately after de_thread(), so this
> problem shouldn't occur there. Basically, I think that I'm not making the
> problem worse with my patches this way.
> 
> 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
>  - SECCOMP_FILTER_FLAG_TSYNC (sets NO_NEW_PRIVS on remote task)
> 
> Beyond that, conceptually, the new cred_guard_light could also be turned
> into a read-write mutex to prevent deadlocks between its users (where
> execve would take it for writing and everyone else would take it for
> reading), but afaik the kernel doesn't have an implementation of
> read-write mutexes yet?
[...]

It does (rw_semaphore), but with PREEMPT_RT enabled they become simple
mutexes.  So I don't think we should rely on that to avoid deadlock.

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]