From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753693AbdBTPYo (ORCPT ); Mon, 20 Feb 2017 10:24:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33368 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753106AbdBTPYl (ORCPT ); Mon, 20 Feb 2017 10:24:41 -0500 Date: Mon, 20 Feb 2017 16:22:03 +0100 From: Oleg Nesterov To: "Eric W. Biederman" Cc: Andrew Morton , Mika =?iso-8859-1?Q?Penttil=E4?= , Aleksa Sarai , Andy Lutomirski , Attila Fazekas , Jann Horn , Kees Cook , Michal Hocko , Ulrich Obergfell , linux-kernel@vger.kernel.org Subject: Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held Message-ID: <20170220152202.GA13726@redhat.com> References: <20170213141452.GA30203@redhat.com> <20170213141516.GA30233@redhat.com> <20170213180454.GA2858@redhat.com> <87zihmpdkf.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87zihmpdkf.fsf@xmission.com> User-Agent: Mutt/1.5.18 (2008-05-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Mon, 20 Feb 2017 15:23:38 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Eric, Thanks for looking into this! and sorry for delay. On 02/17, Eric W. Biederman wrote: > > Oleg Nesterov writes: > > > - In any case we should limit the scope of cred_guard_mutex in execve paths. > > It is not clear why do we take it at the start of execve, and worse, it is > > not clear why we do we actually overload this mutex to exclude other threads > > (except check_unsafe_exec() but this is solveable). The original motivation > > was signal->in_exec_mm protection but this idea was replaced by 3c77f8457221 > > ("exec: make argv/envp memory visible to oom-killer"). It is just ugly to > > call copy_strings/etc with this mutex held. > > > The original changes that introduced cred_guard_mutex are: > a6f76f23d297 ("CRED: Make execve() take advantage of copy-on-write credentials") > d84f4f992cbd ("CRED: Inaugurate COW credentials") > > So I don't think you actually have your history right. > > Beyond that there is a compelling reason to have exec appear atomic from > the perspective of ptrace_attach. If the operation is a setuid exec > and the tracer does not have permission to trace the original or the > result of the exec there could be some significant information leakage > if the exec operation is not atomic from the perspective of > ptrace_attach. Yes sure. But I meant execve() should not take cred_guard_mutex at the start, it should take it later even if we do not rework the security hooks. At least it should take it after copy_strings(), but probably this needs some work. > Additionally your comment makes me nervous when you are wondering why we > take this mutex to exclude other threads and I look in the git history > and see: > > commit 9b1bf12d5d51bca178dea21b04a0805e29d60cf1 > Author: KOSAKI Motohiro > Date: Wed Oct 27 15:34:08 2010 -0700 > > signals: move cred_guard_mutex from task_struct to signal_struct > > Oleg Nesterov pointed out we have to prevent multiple-threads-inside-exec > itself and we can reuse ->cred_guard_mutex for it. Yes, concurrent > execve() has no worth. > > Let's move ->cred_guard_mutex from task_struct to signal_struct. It > naturally prevent multiple-threads-inside-exec. Yes, and let me explain the original motivation for this change. To remind, we had a problem with copy_strings() which can use a lot of memory, and this memory was not visible to OOM-killer. So we were going to add the new member, signal_struct->in_exec_mm = bprm->mm and change OOM-killer to account both task->mm and task->signal->in_exec_mm. And in this case we obviously need to ensure that only one thread can enter exec and use signal_struct->in_exec_mm. That patch was ready, but then we found another (better) solution: 3c77f8457221 ("exec: make argv/envp memory visible to oom-killer"). So I do not think we need to exclude other threads today, and we do not need to hold cred_guard_mutex throughout the whole execve path. Again, this needs some work. For example check_unsafe_exec() assumes it can't race with another thread, see 9e00cdb091b008cb3c78192651180 "exec:check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic". But this looks solvable. > So while I fully agree we have issues here that we need to address and > fix your patch description does not inspire confidence. See above... what do you think I should change in this part of changelog? Thanks, Oleg.