From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754901AbdBPLsn (ORCPT ); Thu, 16 Feb 2017 06:48:43 -0500 Received: from out02.mta.xmission.com ([166.70.13.232]:59776 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932179AbdBPLq5 (ORCPT ); Thu, 16 Feb 2017 06:46:57 -0500 From: ebiederm@xmission.com (Eric W. Biederman) To: Oleg Nesterov Cc: Andrew Morton , Mika =?utf-8?Q?Penttil?= =?utf-8?Q?=C3=A4?= , Aleksa Sarai , Andy Lutomirski , Attila Fazekas , Jann Horn , Kees Cook , Michal Hocko , Ulrich Obergfell , linux-kernel@vger.kernel.org References: <20170213141452.GA30203@redhat.com> <20170213141516.GA30233@redhat.com> <20170213180454.GA2858@redhat.com> Date: Fri, 17 Feb 2017 00:42:08 +1300 In-Reply-To: <20170213180454.GA2858@redhat.com> (Oleg Nesterov's message of "Mon, 13 Feb 2017 19:04:54 +0100") Message-ID: <87zihmpdkf.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1ceKWF-0007oo-IH;;;mid=<87zihmpdkf.fsf@xmission.com>;;;hst=in01.mta.xmission.com;;;ip=101.100.131.232;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1/oTocLSWQp8lbWn0cTVe86aKFZ8sq6SCQ= X-SA-Exim-Connect-IP: 101.100.131.232 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.7 XMSubLong Long Subject * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_02 5+ unique symbols in subject * 0.2 T_XMDrugObfuBody_14 obfuscated drug references * 1.0 T_XMHurry_00 Hurry and Do Something * 0.0 T_TooManySym_01 4+ unique symbols in subject X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: *;Oleg Nesterov X-Spam-Relay-Country: X-Spam-Timing: total 5547 ms - load_scoreonly_sql: 0.06 (0.0%), signal_user_changed: 3.5 (0.1%), b_tie_ro: 2.5 (0.0%), parse: 1.00 (0.0%), extract_message_metadata: 16 (0.3%), get_uri_detail_list: 2.4 (0.0%), tests_pri_-1000: 7 (0.1%), tests_pri_-950: 1.20 (0.0%), tests_pri_-900: 1.02 (0.0%), tests_pri_-400: 26 (0.5%), check_bayes: 25 (0.5%), b_tokenize: 8 (0.1%), b_tok_get_all: 9 (0.2%), b_comp_prob: 2.4 (0.0%), b_tok_touch_all: 3.3 (0.1%), b_finish: 0.64 (0.0%), tests_pri_0: 515 (9.3%), check_dkim_signature: 0.56 (0.0%), check_dkim_adsp: 3.2 (0.1%), tests_pri_500: 4973 (89.6%), poll_dns_idle: 4965 (89.5%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org I am slowly working my way through this and I have found something that I have an issue with. 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. This is the kind of thing that could easily defeat address space layout randomization of setuid executables if we get this wrong. So I don't think this is a small thing we are talking about. And while I would like to merge this patch, a patch description that appears to encourage people to make changes that will create security vulnerabilities does not make me comfortable. 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. Signed-off-by: KOSAKI Motohiro Reviewed-by: Oleg Nesterov Acked-by: Roland McGrath Acked-by: David Howells Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds AKA it was your idea and you reviewed the patch that made it happen. So while I fully agree we have issues here that we need to address and fix your patch description does not inspire confidence. Eric