linux-kernel - [PATCH v2 2/6] exec: Lock more defensively in exec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ftalla80.fsf_-_@x220.int.ebiederm.org>
Date:   Tue, 23 Jun 2020 16:54:07 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     <linux-kernel@...r.kernel.org>
Cc:     <linux-fsdevel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>, Jann Horn <jannh@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Bernd Edlinger <bernd.edlinger@...mail.de>
Subject: [PATCH v2 2/6] exec: Lock more defensively in exec


When taking the task_list_lock in de_thread also take the siglock.  This
makes de_thread closer to fork the canonical place where these locks are
taken.

To complete the defensiveness always take siglock when clearing
group_exit_task and notify_count.

This gives now gives the guarantee that group_exit_task and notify_count
are now always changed under siglock.  As anything multi-threaded in exec
is a rare and slow path I don't think we care if we take an extra lock in
practice.

The practical reason for doing this is to enable setting signal->flags along
with group_exit_task so that the function signal_group_exit can be simplified.

Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
---
 fs/exec.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..33b5d9229c01 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1171,6 +1171,7 @@ static int de_thread(struct task_struct *tsk)
 		for (;;) {
 			cgroup_threadgroup_change_begin(tsk);
 			write_lock_irq(&tasklist_lock);
+			spin_lock(lock);
 			/*
 			 * Do this under tasklist_lock to ensure that
 			 * exit_notify() can't miss ->group_exit_task
@@ -1179,6 +1180,7 @@ static int de_thread(struct task_struct *tsk)
 			if (likely(leader->exit_state))
 				break;
 			__set_current_state(TASK_KILLABLE);
+			spin_unlock(lock);
 			write_unlock_irq(&tasklist_lock);
 			cgroup_threadgroup_change_end(tsk);
 			schedule();
@@ -1234,14 +1236,17 @@ static int de_thread(struct task_struct *tsk)
 		 */
 		if (unlikely(leader->ptrace))
 			__wake_up_parent(leader, leader->parent);
+		spin_unlock(lock);
 		write_unlock_irq(&tasklist_lock);
 		cgroup_threadgroup_change_end(tsk);
 
 		release_task(leader);
 	}
 
+	spin_lock_irq(lock);
 	sig->group_exit_task = NULL;
 	sig->notify_count = 0;
+	spin_unlock_irq(lock);
 
 no_thread_group:
 	/* we have changed execution domain */
@@ -1252,10 +1257,12 @@ static int de_thread(struct task_struct *tsk)
 
 killed:
 	/* protects against exit_notify() and __exit_signal() */
-	read_lock(&tasklist_lock);
+	read_lock_irq(&tasklist_lock);
+	spin_lock(lock);
 	sig->group_exit_task = NULL;
 	sig->notify_count = 0;
-	read_unlock(&tasklist_lock);
+	spin_unlock(lock);
+	read_unlock_irq(&tasklist_lock);
 	return -EAGAIN;
 }
 
-- 
2.20.1