Re: [PATCH 2/2] exec: Add a exec_update_mutex to replace cred_guard_mutex

ebiederm@xmission.com (Eric W. Biederman) writes:

ebiederm@xmission.com (Eric W. Biederman) writes:

quoted

Bernd Edlinger [off-list ref] writes:

quoted

On 3/6/20 6:17 AM, Eric W. Biederman wrote:

quoted

Bernd Edlinger [off-list ref] writes:

quoted

On 3/5/20 10:16 PM, Eric W. Biederman wrote:

quoted

The cred_guard_mutex is problematic.  The cred_guard_mutex is held
over the userspace accesses as the arguments from userspace are read.
The cred_guard_mutex is held of PTRACE_EVENT_EXIT as the the other
threads are killed.  The cred_guard_mutex is held over
"put_user(0, tsk->clear_child_tid)" in exit_mm().

I am all for this patch, and the direction it is heading, Eric.

I just wanted to add a note that I think it is
possible that exec_mm_release can also invoke put_user(0, tsk->clear_child_tid),
under the new exec_update_mutex, since vm_access increments the
mm->mm_users, under the cred_update_mutex, but releases the mutex,
and the caller can hold the reference for a while and then exec_mmap is not
releasing the last reference.

Good catch.  I really appreciate your close look at the details.

I am wondering if process_vm_readv and process_vm_writev could be
safely changed to use mmgrab and mmdrop, instead of mmget and mmput.

That would resolve the potential issue you have pointed out.  I just
haven't figured out if it is safe.  The mm code has been seriously
refactored since I knew how it all worked.

Nope, mmget can not be replaced by mmgrab.

It might be possible to do something creative like store a cred in place
of the userns on the mm and use that for mm_access permission checks.
Still we are talking a pretty narrow window, and a case that no one has
figured out how to trigger yet.  So I will leave that corner case as
something for future improvements.

My brain is restless and keep looking at it.

The worst case is processes created with CLONE_VM|CLONE_CHILD_CLEARTID
but not CLONE_THREAD.  For those that put_user will occur ever time
in exec_mmap.

The only solution that I can see is to move taking the new mutex after
exec_mm_release.  Which may be feasible given how close exec_mmap
follows de_thread.

I am going to sleep on that and perhaps I will be able to see how to
move taking the mutex lower.

It would be very nice not to have a known issue going into this set of
changes.

Eric

It was pointed out that de_thread may return -ENOMEM
when it already terminated threads, and returning
an error from execve, except when a fatal signal is
being delivered is not an option any more.

Allocate the memory for the signal table earlier,
and make sure that -ENOMEM is returned before the
unrecoverable actions are started.

Signed-off-by: Bernd Edlinger <redacted>
---
Eric, what do you think, might this be helpful
to move the "point of no return" lower, and simplify
your patch?

 fs/exec.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 74d88da..a0328dc 100644
--- a/fs/exec.c
+++ b/fs/exec.c

@@ -1057,16 +1057,26 @@ static int exec_mmap(struct mm_struct *mm)
  * disturbing other processes.  (Other processes might share the signal
  * table via the CLONE_SIGHAND option to clone().)
  */
-static int de_thread(struct task_struct *tsk)
+static int de_thread(void)
 {
+	struct task_struct *tsk = current;
 	struct signal_struct *sig = tsk->signal;
 	struct sighand_struct *oldsighand = tsk->sighand;
 	spinlock_t *lock = &oldsighand->siglock;
+	struct sighand_struct *newsighand = NULL;
 
 	if (thread_group_empty(tsk))
 		goto no_thread_group;
 
 	/*
+	 * This is the last time for an out of memory error.
+	 * After this point only fatal signals are are okay.
+	 */
+	newsighand = kmem_cache_alloc(sighand_cachep, GFP_KERNEL);
+	if (!newsighand)
+		return -ENOMEM;
+
+	/*
 	 * Kill all other threads in the thread group.
 	 */
 	spin_lock_irq(lock);

@@ -1076,7 +1086,7 @@ static int de_thread(struct task_struct *tsk)
 		 * return so that the signal is processed.
 		 */
 		spin_unlock_irq(lock);
-		return -EAGAIN;
+		goto err_free;
 	}
 
 	sig->group_exit_task = tsk;

@@ -1191,14 +1201,16 @@ static int de_thread(struct task_struct *tsk)
 #endif
 
 	if (refcount_read(&oldsighand->count) != 1) {
-		struct sighand_struct *newsighand;
 		/*
 		 * This ->sighand is shared with the CLONE_SIGHAND
 		 * but not CLONE_THREAD task, switch to the new one.
 		 */
-		newsighand = kmem_cache_alloc(sighand_cachep, GFP_KERNEL);
-		if (!newsighand)
-			return -ENOMEM;
+		if (!newsighand) {
+			newsighand = kmem_cache_alloc(sighand_cachep,
+						      GFP_KERNEL);
+			if (!newsighand)
+				return -ENOMEM;
+		}
 
 		refcount_set(&newsighand->count, 1);
 		memcpy(newsighand->action, oldsighand->action,

@@ -1211,7 +1223,8 @@ static int de_thread(struct task_struct *tsk)
 		write_unlock_irq(&tasklist_lock);
 
 		__cleanup_sighand(oldsighand);
-	}
+	} else if (newsighand)
+		kmem_cache_free(sighand_cachep, newsighand);
 
 	BUG_ON(!thread_group_leader(tsk));
 	return 0;

@@ -1222,6 +1235,8 @@ static int de_thread(struct task_struct *tsk)
 	sig->group_exit_task = NULL;
 	sig->notify_count = 0;
 	read_unlock(&tasklist_lock);
+err_free:
+	kmem_cache_free(sighand_cachep, newsighand);
 	return -EAGAIN;
 }
 

@@ -1262,7 +1277,7 @@ int flush_old_exec(struct linux_binprm * bprm)
 	 * Make sure we have a private signal table and that
 	 * we are unassociated from the previous thread group.
 	 */
-	retval = de_thread(current);
+	retval = de_thread();
 	if (retval)
 		goto out;
 

-- 
1.9.1