From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 518B8C18E5C for ; Tue, 10 Mar 2020 19:09:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 22FE520727 for ; Tue, 10 Mar 2020 19:09:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726497AbgCJTI7 (ORCPT ); Tue, 10 Mar 2020 15:08:59 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:43668 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726403AbgCJTI7 (ORCPT ); Tue, 10 Mar 2020 15:08:59 -0400 Received: from in02.mta.xmission.com ([166.70.13.52]) by out02.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jBkFN-0001yX-6z; Tue, 10 Mar 2020 13:08:57 -0600 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1jBkFL-0004Uf-I6; Tue, 10 Mar 2020 13:08:57 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Bernd Edlinger Cc: Christian Brauner , Kees Cook , Jann Horn , Jonathan Corbet , Alexander Viro , Andrew Morton , Alexey Dobriyan , Thomas Gleixner , Oleg Nesterov , Frederic Weisbecker , Andrei Vagin , Ingo Molnar , "Peter Zijlstra \(Intel\)" , Yuyang Du , David Hildenbrand , Sebastian Andrzej Siewior , Anshuman Khandual , David Howells , James Morris , Greg Kroah-Hartman , Shakeel Butt , Jason Gunthorpe , Christian Kellner , Andrea Arcangeli , Aleksa Sarai , "Dmitry V. Levin" , "linux-doc\@vger.kernel.org" , "linux-kernel\@vger.kernel.org" , "linux-fsdevel\@vger.kernel.org" , "linux-mm\@kvack.org" , "stable\@vger.kernel.org" , "linux-api\@vger.kernel.org" References: <87tv32cxmf.fsf_-_@x220.int.ebiederm.org> <87v9ne5y4y.fsf_-_@x220.int.ebiederm.org> <87zhcq4jdj.fsf_-_@x220.int.ebiederm.org> <878sk94eay.fsf@x220.int.ebiederm.org> <87r1y12yc7.fsf@x220.int.ebiederm.org> <87k13t2xpd.fsf@x220.int.ebiederm.org> <87d09l2x5n.fsf@x220.int.ebiederm.org> <871rq12vxu.fsf@x220.int.ebiederm.org> <877dzt1fnf.fsf@x220.int.ebiederm.org> <875zfcxlwy.fsf@x220.int.ebiederm.org> Date: Tue, 10 Mar 2020 14:06:36 -0500 In-Reply-To: (Bernd Edlinger's message of "Tue, 10 Mar 2020 18:45:47 +0100") Message-ID: <874kuwvxkz.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1jBkFL-0004Uf-I6;;;mid=<874kuwvxkz.fsf@x220.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18WbrkloOri1g4xv59aV91UgTyj+0r0tb4= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 3/4] proc: io_accounting: Use new infrastructure to fix deadlocks in execve X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-doc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-doc@vger.kernel.org Bernd Edlinger writes: > This changes do_io_accounting to use the new exec_update_mutex > instead of cred_guard_mutex. > > This fixes possible deadlocks when the trace is accessing > /proc/$pid/io for instance. > > This should be safe, as the credentials are only used for reading. This is an improvement. We probably want to do this just as an incremental step in making things better but perhaps I am blind but I am not finding the reason for guarding this with the cred_guard_mutex to be at all persuasive. I think moving the ptrace_may_access check down to after the unlock_task_sighand would be just as effective at addressing the concerns raised in the original commit. I think the task_lock provides all of the barrier we need to make it safe to move the ptrace_may_access checks safe. The reason I say this is I don't see exec changing ->ioac. Just performing some I/O which would update the io accounting statistics. Can anyone see if I am wrong? Eric commit 293eb1e7772b25a93647c798c7b89bf26c2da2e0 Author: Vasiliy Kulikov Date: Tue Jul 26 16:08:38 2011 -0700 proc: fix a race in do_io_accounting() If an inode's mode permits opening /proc/PID/io and the resulting file descriptor is kept across execve() of a setuid or similar binary, the ptrace_may_access() check tries to prevent using this fd against the task with escalated privileges. Unfortunately, there is a race in the check against execve(). If execve() is processed after the ptrace check, but before the actual io information gathering, io statistics will be gathered from the privileged process. At least in theory this might lead to gathering sensible information (like ssh/ftp password length) that wouldn't be available otherwise. Holding task->signal->cred_guard_mutex while gathering the io information should protect against the race. The order of locking is similar to the one inside of ptrace_attach(): first goes cred_guard_mutex, then lock_task_sighand(). Signed-off-by: Vasiliy Kulikov Cc: Al Viro Cc: Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds > Signed-off-by: Bernd Edlinger > --- > fs/proc/base.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 4fdfe4f..529d0c6 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2770,7 +2770,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh > unsigned long flags; > int result; > > - result = mutex_lock_killable(&task->signal->cred_guard_mutex); > + result = mutex_lock_killable(&task->signal->exec_update_mutex); > if (result) > return result; > > @@ -2806,7 +2806,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh > result = 0; > > out_unlock: > - mutex_unlock(&task->signal->cred_guard_mutex); > + mutex_unlock(&task->signal->exec_update_mutex); > return result; > }