Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Bernd Edlinger <bernd.edlinger@hotmail.de>
Cc:     "gregkh\@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        Kirill Tkhai <ktkhai@virtuozzo.com>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Kees Cook <keescook@chromium.org>,
        "jannh\@google.com" <jannh@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        "adobriyan\@gmail.com" <adobriyan@gmail.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Oleg Nesterov <oleg@redhat.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        "avagin\@gmail.com" <avagin@gmail.com>,
        Ingo Molnar <mingo@kernel.org>,
        "Peter Zijlstra \(Intel\)" <peterz@infradead.org>,
        "duyuyang\@gmail.com" <duyuyang@gmail.com>,
        David Hildenbrand <david@redhat.com>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        David Howells <dhowells@redhat.com>,
        James Morris <jamorris@linux.microsoft.com>,
        Shakeel Butt <shakeelb@google.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        "christian\@kellner.me" <christian@kellner.me>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Aleksa Sarai <cyphar@cyphar.com>,
        "Dmitry V. Levin" <ldv@altlinux.org>,
        "linux-doc\@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-fsdevel\@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-mm\@kvack.org" <linux-mm@kvack.org>,
        "stable\@vger.kernel.org" <stable@vger.kernel.org>,
        "linux-api\@vger.kernel.org" <linux-api@vger.kernel.org>
References: <077b63b7-6f5e-aa8e-bf96-a586b481cc46@hotmail.de>
        <b6537ae6-31b1-5c50-f32b-8b8332ace882@hotmail.de>
Date:   Wed, 25 Mar 2020 09:27:18 -0500
In-Reply-To: <b6537ae6-31b1-5c50-f32b-8b8332ace882@hotmail.de> (Bernd
        Edlinger's message of "Sat, 21 Mar 2020 02:46:39 +0000")
Message-ID: <87a7448q7t.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
Subject: Re: [PATCH v6 15/16] exec: Fix dead-lock in de_thread with ptrace_attach
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

> This removes the last users of cred_guard_mutex
> and replaces it with a new mutex exec_guard_mutex,
> and a boolean unsafe_execve_in_progress.
>
> This addresses the case when at least one of the
> sibling threads is traced, and therefore the trace
> process may dead-lock in ptrace_attach, but de_thread
> will need to wait for the tracer to continue execution.
>
> The solution is to detect this situation and make
> ptrace_attach and similar functions return -EAGAIN,
> but only in a situation where a dead-lock is imminent.
>
> This means this is an API change, but only when the
> process is traced while execve happens in a
> multi-threaded application.
>
> See tools/testing/selftests/ptrace/vmaccess.c
> for a test case that gets fixed by this change.

Hmm.  The logic with unsafe_execve_in_progress is interesting.
I think I see what you are aiming for.

So far as you have hit what you are aiming for I think this is
a safe change as the only cases that will change are the cases
that would deadlock today.

At a minimum the code is subtle and I don't see big fat
warning comments that subtle code needs to keep people
from using it wrong.

Further while the change below to proc_pid_attr_write looks
like it is being treated the same as ptrace_attach.  When in
fact proc_pid_attr_write needs the no_new_privs and ptrace_attach
protection the same as exec.  As the updated cred won't be used in an
ongoing exec, exec does not need protection from proc_pid_attr_write,
other than deadlock protection.

Having the relevant lock be per task_struct lock would probably be a
better way to avoid deadlock with a concurrent proc_pid_attr_write.


So I am going to pass on these last two patches for now, and apply the
rest and get them into linux-next.

Eric


> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6b13fc4..a428536 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2680,14 +2680,17 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>  	}
>  
>  	/* Guard against adverse ptrace interaction */
> -	rv = mutex_lock_interruptible(&current->signal->cred_guard_mutex);
> +	rv = mutex_lock_interruptible(&current->signal->exec_guard_mutex);
>  	if (rv < 0)
>  		goto out_free;
>  
> -	rv = security_setprocattr(PROC_I(inode)->op.lsm,
> -				  file->f_path.dentry->d_name.name, page,
> -				  count);
> -	mutex_unlock(&current->signal->cred_guard_mutex);
> +	if (unlikely(current->signal->unsafe_execve_in_progress))
> +		rv = -EAGAIN;
> +	else
> +		rv = security_setprocattr(PROC_I(inode)->op.lsm,
> +					  file->f_path.dentry->d_name.name,
> +					  page, count);
> +	mutex_unlock(&current->signal->exec_guard_mutex);
>  out_free:
>  	kfree(page);
>  out: