From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from thejh.net ([37.221.195.125]:35088 "EHLO thejh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752710AbcKBUuQ (ORCPT ); Wed, 2 Nov 2016 16:50:16 -0400 Date: Wed, 2 Nov 2016 21:50:11 +0100 From: Jann Horn To: Oleg Nesterov Cc: Alexander Viro , Roland McGrath , John Johansen , James Morris , "Serge E. Hallyn" , Paul Moore , Stephen Smalley , Eric Paris , Casey Schaufler , Kees Cook , Andrew Morton , Janis Danisevskis , Seth Forshee , "Eric W. Biederman" , Thomas Gleixner , Benjamin LaHaise , Ben Hutchings , Andy Lutomirski , Linus Torvalds , Krister Johansen , 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 Message-ID: <20161102205011.GF8196@pc.thejh.net> References: <1477863998-3298-1-git-send-email-jann@thejh.net> <1477863998-3298-2-git-send-email-jann@thejh.net> <20161102181806.GB1112@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="+QwZB9vYiNIzNXIj" Content-Disposition: inline In-Reply-To: <20161102181806.GB1112@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: --+QwZB9vYiNIzNXIj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 debugg= ed > > task. >=20 > 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? cred_guard_light would mean that you could theoretically still create deadlocks, but afaics only if you do things like trying to read /proc/$pid/mem in the FUSE read handler for the file that is currently being executed - and in that case, I think it's okay to have a killable deadlock. Do you think that, if (apart from execve) only PTRACE_ATTACH and SECCOMP_FILTER_FLAG_TSYNC remain as users of cred_guard_mutex and everything else used my new cred_guard_light, that would be sufficient to fix the races you are concerned about? It seems to me like SECCOMP_FILTER_FLAG_TSYNC doesn't really have deadlocking issues. PTRACE_ATTACH isn't that clear to me; if a debugger tries to attach to a newly spawned thread while another ptraced thread is dying because of de_thread() in a third thread, that might still cause the debugger to deadlock, right? The problem with PTRACE_ATTACH is basically that bprm->unsafe is used in the bprm_set_creds LSM hook, so it needs to have been calculated when that hook is executed. (Also in the bprm_secureexec hook, but that one happens after install_exec_creds(), so that's unproblematic.) security_bprm_set_creds() is called in prepare_binprm(), which is executed very early in do_execveat_common(), at a point where failures should still be graceful (return an error code instead of killing the whole process), and therefore other threads can still run and debuggers can still attach. The LSM hooks that execute at that point e.g. inspect and modify bprm->cred, and they can still cleanly prohibit execution. E.g. SELinux does this - it can cancel execution with errors like -EPERM and -EACCES. AFAICS the hard case is: - Multithreaded process with tasks A and B is running. - Task C attaches to B via ptrace. - Task A calls execve(), takes the mutex, reaches de_thread(), kills task B. - Task C tries to attach to A, tries to take the mutex again, deadlock. I'm not sure whether it'd be possible to get rid of the deadlock for PTRACE_ATTACH without ABI changes, and I would be surprised if it was doable without nontrivial additional logic. --+QwZB9vYiNIzNXIj Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJYGlGDAAoJED4KNFJOeCOoFDkQAMksGyLn3MV2JM+zX2Jx5kGI 8p+saNZW2XRg5lKBswoXhvzRUw7Xge/AGUIR8TakjKZaiOuqsiJQt4XBtSY6Jyan th5TYtxiFztieC0aZ0GjPz8n8BCP18nqjm3ZUUi65n/fvttegGtXDM080V84Rvnx MbJW68r3iGKeE+Ssuxqu2Ms2nehEMb5Qfj6NDfDFhjvRYIhsT0b5vHUxBokKEzsX 9gKaq8M6WqMzNB843LQj483rV2NCTyrTyUsUZZB0YnFzJv9Ivnlm8sUAYijSCgV5 JqiPkdRT3vJ2+o1a39JnwkN0f57P0gr9cGVXLP7mjx7pgzt9k6XvBz5+2NWqkmmS jF8EHr7dIM28BOtMHFDDAVhi5e/owpayCeAxCCOTXbzxhDYClNFndXO4Ne1pJyFa rHzpW5aAx44yH5Dbgkf52EN0zjpzDbuJVjZH8r8H6W5i0S9bZw3VOzxmrUCyXyJ0 GA8nVrm5xbFcNUHdqEMmsg8N1CPpGkpI1o+4mRJzSQ+/DMwo2VEBkwVMV3Wyo/H5 BHv9UgoRxb0TpFgBfi0a+H87kKs5SEnRL8j7ryDdJZbM9Hq9KFC2F7+yZ2Yjd1qT fpkAUHqVzrjasbJOt+p4JsQmuqof5apbHp4Hi3xjjrekzGMghe9jopJQwqxceGJa q5/cn9Y/r1Uu3dATpRV5 =A67Z -----END PGP SIGNATURE----- --+QwZB9vYiNIzNXIj--