Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1579808 > unrolled thread

[PATCH 0/2] fix the traced mt-exec deadlock

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] fix the traced mt-exec deadlock Oleg Nesterov <oleg@redhat.com> - 2017-02-13 15:20 +0100
    [PATCH 1/2] exec: don't wait for zombie threads with  cred_guard_mutex held Oleg Nesterov <oleg@redhat.com> - 2017-02-13 15:20 +0100
      Re: [PATCH 1/2] exec: don't wait for zombie threads with  cred_guard_mutex held kbuild test robot <lkp@intel.com> - 2017-02-13 17:10 +0100
        Re: [PATCH 1/2] exec: don't wait for zombie threads with  cred_guard_mutex held Oleg Nesterov <oleg@redhat.com> - 2017-02-13 17:50 +0100
      Re: [PATCH 1/2] exec: don't wait for zombie threads with  cred_guard_mutex held kbuild test robot <lkp@intel.com> - 2017-02-13 17:50 +0100
      Re: [PATCH 1/2] exec: don't wait for zombie threads with  cred_guard_mutex held Oleg Nesterov <oleg@redhat.com> - 2017-02-13 19:10 +0100
      [PATCH V2 1/2] exec: don't wait for zombie threads with  cred_guard_mutex held Oleg Nesterov <oleg@redhat.com> - 2017-02-13 19:10 +0100
        Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held ebiederm@xmission.com (Eric W. Biederman) - 2017-02-16 12:50 +0100
          Re: [PATCH V2 1/2] exec: don't wait for zombie threads with         cred_guard_mutex held Oleg Nesterov <oleg@redhat.com> - 2017-02-20 16:30 +0100
            Re: [PATCH V2 1/2] exec: don't wait for zombie threads with         cred_guard_mutex held Oleg Nesterov <oleg@redhat.com> - 2017-02-20 16:40 +0100
            Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held ebiederm@xmission.com (Eric W. Biederman) - 2017-02-20 23:40 +0100
              Re: [PATCH V2 1/2] exec: don't wait for zombie threads with         cred_guard_mutex held Oleg Nesterov <oleg@redhat.com> - 2017-02-21 19:00 +0100
                Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held ebiederm@xmission.com (Eric W. Biederman) - 2017-02-21 21:30 +0100
                  Re: [PATCH V2 1/2] exec: don't wait for zombie threads with         cred_guard_mutex held Oleg Nesterov <oleg@redhat.com> - 2017-02-22 19:10 +0100
        Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held ebiederm@xmission.com (Eric W. Biederman) - 2017-02-17 05:50 +0100
          Re: [PATCH V2 1/2] exec: don't wait for zombie threads with         cred_guard_mutex held Oleg Nesterov <oleg@redhat.com> - 2017-02-20 17:00 +0100
    Re: [PATCH 0/2] fix the traced mt-exec deadlock Oleg Nesterov <oleg@redhat.com> - 2017-02-24 17:20 +0100

#1579808 — [PATCH 0/2] fix the traced mt-exec deadlock

Hello,

Lets finally fix this problem, it was reported several times. I still think that
in the longer term we should (try to) rework the security hooks and (partially)
revert this change, but this is not trivial and we need something backportable
anyway.

Eric, Jann, we already discussed this change. 1/2 is the same patch I suggested 3
months ago except now it compiles and moves flush_signal_handlers() to de_thread().

Both patches ask for subsequent cleanups, see the changelogs.

Oleg.

 arch/x86/ia32/ia32_aout.c |   3 ++
 fs/binfmt_aout.c          |   3 ++
 fs/binfmt_elf.c           |   6 ++-
 fs/binfmt_elf_fdpic.c     |   4 ++
 fs/binfmt_flat.c          |   3 ++
 fs/exec.c                 | 128 +++++++++++++++++++++++-----------------------
 include/linux/binfmts.h   |   1 +
 kernel/exit.c             |   5 +-
 kernel/signal.c           |  21 +++++---
 9 files changed, 101 insertions(+), 73 deletions(-)

[toc] | [next] | [standalone]

#1579810 — [PATCH 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

de_thread() waits for other threads with ->cred_guard_mutex held and this
is really bad because the time is not bounded, debugger can delay the exit
and this lock has a lot of users (mostly abusers imo) in fs/proc and more.
And this leads to deadlock if debugger tries to take the same mutex:

	#include <unistd.h>
	#include <signal.h>
	#include <sys/ptrace.h>
	#include <pthread.h>

	void *thread(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return NULL;
	}

	int main(void)
	{
		int pid = fork();

		if (!pid) {
			pthread_t pt;
			pthread_create(&pt, NULL, thread, NULL);
			pthread_join(pt, NULL);
			execlp("echo", "echo", "passed", NULL);
		}

		sleep(1);
		// or anything else which needs ->cred_guard_mutex,
		// say open(/proc/$pid/mem)
		ptrace(PTRACE_ATTACH, pid, 0,0);
		kill(pid, SIGCONT);

		return 0;
	}

This hangs because de_thread() waits for debugger which should release the
killed thread with cred_guard_mutex held, while the debugger sleeps waiting
for the same mutex. Not really that bad, the tracer can be killed, but still
this is a bug and people hit it in practice.

The patch changes flush_old_exec() to wait until all the threads have passed
exit_notify(), we use the new kill_sub_threads() helper instead of de_thread().

However, we still need to wait until they all disappear. The main reason is
that we can not unshare sighand until that, execing thread and zombies must
use the same sighand->siglock to serialize the access to ->thread_head/etc.
So the patch changes ->load_binary() methods to call de_thread() right after
install_exec_creds() drops cred_guard_mutex.

Note that de_thread() is simplified, it no longer needs to send SIGKILL to
sub-threads and wait for the group_leader to become a zombie. But since it
is called after flush_old_exec() we also need to move flush_signal_handlers()
to de_thread().

TODO:

- Move install_exec_creds() and de_thread() into setup_new_exec(), unexport
  de_thread().

- Avoid tasklist_lock in kill_sub_threads(), we can change exit_notify() to
  set ->exit_state under siglock.

- Simplify/cleanup the ->notify_count logic, it is really ugly. I think we
  should just turn this counter into signal->nr_live_threads. __exit_signal
  can use signal->nr_threads instead.

- 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.

Reported-by: Ulrich Obergfell <uobergfe@redhat.com>
Reported-by: Attila Fazekas <afazekas@redhat.com>
Reported-by: Aleksa Sarai <asarai@suse.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/ia32/ia32_aout.c |   3 ++
 fs/binfmt_aout.c          |   3 ++
 fs/binfmt_elf.c           |   6 ++-
 fs/binfmt_elf_fdpic.c     |   4 ++
 fs/binfmt_flat.c          |   3 ++
 fs/exec.c                 | 128 +++++++++++++++++++++++-----------------------
 include/linux/binfmts.h   |   1 +
 kernel/exit.c             |   5 +-
 kernel/signal.c           |   3 +-
 9 files changed, 87 insertions(+), 69 deletions(-)

diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index 7c0a711..a6b9cc9 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -312,6 +312,9 @@ static int load_aout_binary(struct linux_binprm *bprm)
 		return retval;
 
 	install_exec_creds(bprm);
+	retval = de_thread(current);
+	if (retval)
+		return retval;
 
 	if (N_MAGIC(ex) == OMAGIC) {
 		unsigned long text_addr, map_size;
diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index 2a59139..edd1335 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -256,6 +256,9 @@ static int load_aout_binary(struct linux_binprm * bprm)
 		return retval;
 
 	install_exec_creds(bprm);
+	retval = de_thread(current);
+	if (retval)
+		return retval;
 
 	if (N_MAGIC(ex) == OMAGIC) {
 		unsigned long text_addr, map_size;
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 4223702..79508f7 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -855,13 +855,17 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	setup_new_exec(bprm);
 	install_exec_creds(bprm);
 
+	retval = de_thread(current);
+	if (retval)
+		goto out_free_dentry;
+
 	/* Do this so that we can load the interpreter, if need be.  We will
 	   change some of these later */
 	retval = setup_arg_pages(bprm, randomize_stack_top(STACK_TOP),
 				 executable_stack);
 	if (retval < 0)
 		goto out_free_dentry;
-	
+
 	current->mm->start_stack = bprm->p;
 
 	/* Now we do a little grungy work by mmapping the ELF image into
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index d2e36f8..75fd6d8 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -430,6 +430,10 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
 #endif
 
 	install_exec_creds(bprm);
+	retval = de_thread(current);
+	if (retval)
+		goto error;
+
 	if (create_elf_fdpic_tables(bprm, current->mm,
 				    &exec_params, &interp_params) < 0)
 		goto error;
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 9b2917a..a0ad9a3 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -953,6 +953,9 @@ static int load_flat_binary(struct linux_binprm *bprm)
 	}
 
 	install_exec_creds(bprm);
+	res = de_thread(current);
+	if (res)
+		return res;
 
 	set_binfmt(&flat_format);
 
diff --git a/fs/exec.c b/fs/exec.c
index e579466..8591c56 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1036,13 +1036,62 @@ static int exec_mmap(struct mm_struct *mm)
 	return 0;
 }
 
+static int wait_for_notify_count(struct task_struct *tsk, struct signal_struct *sig)
+{
+	for (;;) {
+		if (unlikely(__fatal_signal_pending(tsk)))
+			goto killed;
+		set_current_state(TASK_KILLABLE);
+		if (!sig->notify_count)
+			break;
+		schedule();
+	}
+	__set_current_state(TASK_RUNNING);
+	return 0;
+
+killed:
+	/* protects against exit_notify() and __exit_signal() */
+	read_lock(&tasklist_lock);
+	sig->group_exit_task = NULL;
+	sig->notify_count = 0;
+	read_unlock(&tasklist_lock);
+	return -EINTR;
+}
+
+/*
+ * Kill all the sub-threads and wait until they all pass exit_notify().
+ */
+static int kill_sub_threads(struct task_struct *tsk)
+{
+	struct signal_struct *sig = tsk->signal;
+	int err = -EINTR;
+
+	if (thread_group_empty(tsk))
+		return 0;
+
+	read_lock(&tasklist_lock);
+	spin_lock_irq(&tsk->sighand->siglock);
+	if (!signal_group_exit(sig)) {
+		sig->group_exit_task = tsk;
+		sig->notify_count = -zap_other_threads(tsk);
+		err = 0;
+	}
+	spin_unlock_irq(&tsk->sighand->siglock);
+	read_unlock(&tasklist_lock);
+
+	if (!err)
+		err = wait_for_notify_count(tsk, sig);
+	return err;
+
+}
+
 /*
- * This function makes sure the current process has its own signal table,
- * so that flush_signal_handlers can later reset the handlers without
- * disturbing other processes.  (Other processes might share the signal
- * table via the CLONE_SIGHAND option to clone().)
+ * This function makes sure the current process has no other threads and
+ * has a private signal table so that flush_signal_handlers() can reset
+ * the handlers without disturbing other processes which might share the
+ * signal table via the CLONE_SIGHAND option to clone().
  */
-static int de_thread(struct task_struct *tsk)
+int de_thread(struct task_struct *tsk)
 {
 	struct signal_struct *sig = tsk->signal;
 	struct sighand_struct *oldsighand = tsk->sighand;
@@ -1051,60 +1100,24 @@ static int de_thread(struct task_struct *tsk)
 	if (thread_group_empty(tsk))
 		goto no_thread_group;
 
-	/*
-	 * Kill all other threads in the thread group.
-	 */
 	spin_lock_irq(lock);
-	if (signal_group_exit(sig)) {
-		/*
-		 * Another group action in progress, just
-		 * return so that the signal is processed.
-		 */
-		spin_unlock_irq(lock);
-		return -EAGAIN;
-	}
-
-	sig->group_exit_task = tsk;
-	sig->notify_count = zap_other_threads(tsk);
+	sig->notify_count = sig->nr_threads;
 	if (!thread_group_leader(tsk))
 		sig->notify_count--;
-
-	while (sig->notify_count) {
-		__set_current_state(TASK_KILLABLE);
-		spin_unlock_irq(lock);
-		schedule();
-		if (unlikely(__fatal_signal_pending(tsk)))
-			goto killed;
-		spin_lock_irq(lock);
-	}
 	spin_unlock_irq(lock);
 
+	if (wait_for_notify_count(tsk, sig))
+		return -EINTR;
+
 	/*
 	 * At this point all other threads have exited, all we have to
-	 * do is to wait for the thread group leader to become inactive,
-	 * and to assume its PID:
+	 * do is to reap the old leader and assume its PID.
 	 */
 	if (!thread_group_leader(tsk)) {
 		struct task_struct *leader = tsk->group_leader;
 
-		for (;;) {
-			threadgroup_change_begin(tsk);
-			write_lock_irq(&tasklist_lock);
-			/*
-			 * Do this under tasklist_lock to ensure that
-			 * exit_notify() can't miss ->group_exit_task
-			 */
-			sig->notify_count = -1;
-			if (likely(leader->exit_state))
-				break;
-			__set_current_state(TASK_KILLABLE);
-			write_unlock_irq(&tasklist_lock);
-			threadgroup_change_end(tsk);
-			schedule();
-			if (unlikely(__fatal_signal_pending(tsk)))
-				goto killed;
-		}
-
+		threadgroup_change_begin(tsk);
+		write_lock_irq(&tasklist_lock);
 		/*
 		 * The only record we have of the real-time age of a
 		 * process, regardless of execs it's done, is start_time.
@@ -1162,10 +1175,9 @@ static int de_thread(struct task_struct *tsk)
 		release_task(leader);
 	}
 
+no_thread_group:
 	sig->group_exit_task = NULL;
 	sig->notify_count = 0;
-
-no_thread_group:
 	/* we have changed execution domain */
 	tsk->exit_signal = SIGCHLD;
 
@@ -1198,15 +1210,8 @@ static int de_thread(struct task_struct *tsk)
 	}
 
 	BUG_ON(!thread_group_leader(tsk));
+	flush_signal_handlers(current, 0);
 	return 0;
-
-killed:
-	/* protects against exit_notify() and __exit_signal() */
-	read_lock(&tasklist_lock);
-	sig->group_exit_task = NULL;
-	sig->notify_count = 0;
-	read_unlock(&tasklist_lock);
-	return -EAGAIN;
 }
 
 char *get_task_comm(char *buf, struct task_struct *tsk)
@@ -1237,11 +1242,7 @@ int flush_old_exec(struct linux_binprm * bprm)
 {
 	int retval;
 
-	/*
-	 * Make sure we have a private signal table and that
-	 * we are unassociated from the previous thread group.
-	 */
-	retval = de_thread(current);
+	retval = kill_sub_threads(current);
 	if (retval)
 		goto out;
 
@@ -1336,7 +1337,6 @@ void setup_new_exec(struct linux_binprm * bprm)
 	/* An exec changes our domain. We are no longer part of the thread
 	   group */
 	current->self_exec_id++;
-	flush_signal_handlers(current, 0);
 }
 EXPORT_SYMBOL(setup_new_exec);
 
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 1303b57..06a5a7b 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -101,6 +101,7 @@ extern int __must_check remove_arg_zero(struct linux_binprm *);
 extern int search_binary_handler(struct linux_binprm *);
 extern int flush_old_exec(struct linux_binprm * bprm);
 extern void setup_new_exec(struct linux_binprm * bprm);
+extern int de_thread(struct task_struct *tsk);
 extern void would_dump(struct linux_binprm *, struct file *);
 
 extern int suid_dumpable;
diff --git a/kernel/exit.c b/kernel/exit.c
index 8f14b86..169d9f2 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -699,8 +699,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	if (tsk->exit_state == EXIT_DEAD)
 		list_add(&tsk->ptrace_entry, &dead);
 
-	/* mt-exec, de_thread() is waiting for group leader */
-	if (unlikely(tsk->signal->notify_count < 0))
+	/* mt-exec, kill_sub_threads() is waiting for group exit */
+	if (unlikely(tsk->signal->notify_count < 0) &&
+	    !++tsk->signal->notify_count)
 		wake_up_process(tsk->signal->group_exit_task);
 	write_unlock_irq(&tasklist_lock);
 
diff --git a/kernel/signal.c b/kernel/signal.c
index 3603d93..b78ce63 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1200,13 +1200,12 @@ int zap_other_threads(struct task_struct *p)
 
 	while_each_thread(p, t) {
 		task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
-		count++;
-
 		/* Don't bother with already dead threads */
 		if (t->exit_state)
 			continue;
 		sigaddset(&t->pending.signal, SIGKILL);
 		signal_wake_up(t, 1);
+		count++;
 	}
 
 	return count;
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1579914 — Re: [PATCH 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

[Multipart message — attachments visible in raw view] — view raw

Hi Oleg,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc8 next-20170213]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Oleg-Nesterov/fix-the-traced-mt-exec-deadlock/20170213-221943
config: m68k-sun3_defconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

>> ERROR: "de_thread" [fs/binfmt_aout.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1579937 — Re: [PATCH 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

On 02/14, kbuild test robot wrote:
>
> Hi Oleg,
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.10-rc8 next-20170213]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Oleg-Nesterov/fix-the-traced-mt-exec-deadlock/20170213-221943
> config: m68k-sun3_defconfig (attached as .config)
> compiler: m68k-linux-gcc (GCC) 4.9.0
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=m68k 
>
> All errors (new ones prefixed by >>):
> 
> >> ERROR: "de_thread" [fs/binfmt_aout.ko] undefined!

Aaah thanks...

Of course, I forgot to add EXPORT_SYMBOL(de_thread), will send V2.

Thanks a lot!

Oleg.

[toc] | [prev] | [next] | [standalone]

#1579941 — Re: [PATCH 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

[Multipart message — attachments visible in raw view] — view raw

Hi Oleg,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc8 next-20170213]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Oleg-Nesterov/fix-the-traced-mt-exec-deadlock/20170213-221943
config: x86_64-randconfig-ne0-02132256 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "de_thread" [arch/x86/ia32/ia32_aout.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1579972 — Re: [PATCH 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

On 02/13, Mika Penttilä wrote:
>
> > +int de_thread(struct task_struct *tsk)
> >  {
> >  	struct signal_struct *sig = tsk->signal;
> >  	struct sighand_struct *oldsighand = tsk->sighand;
> > @@ -1051,60 +1100,24 @@ static int de_thread(struct task_struct *tsk)
> >  	if (thread_group_empty(tsk))
> >  		goto no_thread_group;
> >
> > -	/*
> > -	 * Kill all other threads in the thread group.
> > -	 */
> >  	spin_lock_irq(lock);
> > -	if (signal_group_exit(sig)) {
> > -		/*
> > -		 * Another group action in progress, just
> > -		 * return so that the signal is processed.
> > -		 */
> > -		spin_unlock_irq(lock);
> > -		return -EAGAIN;
> > -	}
> > -
> > -	sig->group_exit_task = tsk;
> > -	sig->notify_count = zap_other_threads(tsk);
> > +	sig->notify_count = sig->nr_threads;
>
>
> maybe nr_threads - 1 since nr_threads includes us ?

Damn. Of course you are right, thanks a lot! Please see v2.

Hmm. I didn't even notice my own test-case didn't pass because of this
off-by-one.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1579974 — [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

de_thread() waits for other threads with ->cred_guard_mutex held and this
is really bad because the time is not bounded, debugger can delay the exit
and this lock has a lot of users (mostly abusers imo) in fs/proc and more.
And this leads to deadlock if debugger tries to take the same mutex:

	#include <unistd.h>
	#include <signal.h>
	#include <sys/ptrace.h>
	#include <pthread.h>

	void *thread(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return NULL;
	}

	int main(void)
	{
		int pid = fork();

		if (!pid) {
			pthread_t pt;
			pthread_create(&pt, NULL, thread, NULL);
			pthread_join(pt, NULL);
			execlp("echo", "echo", "passed", NULL);
		}

		sleep(1);
		// or anything else which needs ->cred_guard_mutex,
		// say open(/proc/$pid/mem)
		ptrace(PTRACE_ATTACH, pid, 0,0);
		kill(pid, SIGCONT);

		return 0;
	}

This hangs because de_thread() waits for debugger which should release the
killed thread with cred_guard_mutex held, while the debugger sleeps waiting
for the same mutex. Not really that bad, the tracer can be killed, but still
this is a bug and people hit it in practice.

The patch changes flush_old_exec() to wait until all the threads have passed
exit_notify(), we use the new kill_sub_threads() helper instead of de_thread().

However, we still need to wait until they all disappear. The main reason is
that we can not unshare sighand until that, execing thread and zombies must
use the same sighand->siglock to serialize the access to ->thread_head/etc.
So the patch changes ->load_binary() methods to call de_thread() right after
install_exec_creds() drops cred_guard_mutex.

Note that de_thread() is simplified, it no longer needs to send SIGKILL to
sub-threads and wait for the group_leader to become a zombie. But since it
is called after flush_old_exec() we also need to move flush_signal_handlers()
to de_thread().

TODO:

- Move install_exec_creds() and de_thread() into setup_new_exec(), unexport
  de_thread().

- Avoid tasklist_lock in kill_sub_threads(), we can change exit_notify() to
  set ->exit_state under siglock.

- Simplify/cleanup the ->notify_count logic, it is really ugly. I think we
  should just turn this counter into signal->nr_live_threads. __exit_signal
  can use signal->nr_threads instead.

- 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.

v2:
- add EXPORT_SYMBOL(de_thread)
- fix the usage of nr_threads in de_thread(), pointed by
  Mika Penttila <mika.penttila@nextfour.com>

Reported-by: Ulrich Obergfell <uobergfe@redhat.com>
Reported-by: Attila Fazekas <afazekas@redhat.com>
Reported-by: Aleksa Sarai <asarai@suse.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/ia32/ia32_aout.c |   3 ++
 fs/binfmt_aout.c          |   3 ++
 fs/binfmt_elf.c           |   6 ++-
 fs/binfmt_elf_fdpic.c     |   4 ++
 fs/binfmt_flat.c          |   3 ++
 fs/exec.c                 | 129 +++++++++++++++++++++++-----------------------
 include/linux/binfmts.h   |   1 +
 kernel/exit.c             |   5 +-
 kernel/signal.c           |   3 +-
 9 files changed, 88 insertions(+), 69 deletions(-)

diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index 7c0a711..a6b9cc9 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -312,6 +312,9 @@ static int load_aout_binary(struct linux_binprm *bprm)
 		return retval;
 
 	install_exec_creds(bprm);
+	retval = de_thread(current);
+	if (retval)
+		return retval;
 
 	if (N_MAGIC(ex) == OMAGIC) {
 		unsigned long text_addr, map_size;
diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index 2a59139..edd1335 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -256,6 +256,9 @@ static int load_aout_binary(struct linux_binprm * bprm)
 		return retval;
 
 	install_exec_creds(bprm);
+	retval = de_thread(current);
+	if (retval)
+		return retval;
 
 	if (N_MAGIC(ex) == OMAGIC) {
 		unsigned long text_addr, map_size;
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 4223702..79508f7 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -855,13 +855,17 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	setup_new_exec(bprm);
 	install_exec_creds(bprm);
 
+	retval = de_thread(current);
+	if (retval)
+		goto out_free_dentry;
+
 	/* Do this so that we can load the interpreter, if need be.  We will
 	   change some of these later */
 	retval = setup_arg_pages(bprm, randomize_stack_top(STACK_TOP),
 				 executable_stack);
 	if (retval < 0)
 		goto out_free_dentry;
-	
+
 	current->mm->start_stack = bprm->p;
 
 	/* Now we do a little grungy work by mmapping the ELF image into
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index d2e36f8..75fd6d8 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -430,6 +430,10 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
 #endif
 
 	install_exec_creds(bprm);
+	retval = de_thread(current);
+	if (retval)
+		goto error;
+
 	if (create_elf_fdpic_tables(bprm, current->mm,
 				    &exec_params, &interp_params) < 0)
 		goto error;
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 9b2917a..a0ad9a3 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -953,6 +953,9 @@ static int load_flat_binary(struct linux_binprm *bprm)
 	}
 
 	install_exec_creds(bprm);
+	res = de_thread(current);
+	if (res)
+		return res;
 
 	set_binfmt(&flat_format);
 
diff --git a/fs/exec.c b/fs/exec.c
index e579466..8e8f2ef 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1036,13 +1036,62 @@ static int exec_mmap(struct mm_struct *mm)
 	return 0;
 }
 
+static int wait_for_notify_count(struct task_struct *tsk, struct signal_struct *sig)
+{
+	for (;;) {
+		if (unlikely(__fatal_signal_pending(tsk)))
+			goto killed;
+		set_current_state(TASK_KILLABLE);
+		if (!sig->notify_count)
+			break;
+		schedule();
+	}
+	__set_current_state(TASK_RUNNING);
+	return 0;
+
+killed:
+	/* protects against exit_notify() and __exit_signal() */
+	read_lock(&tasklist_lock);
+	sig->group_exit_task = NULL;
+	sig->notify_count = 0;
+	read_unlock(&tasklist_lock);
+	return -EINTR;
+}
+
+/*
+ * Kill all the sub-threads and wait until they all pass exit_notify().
+ */
+static int kill_sub_threads(struct task_struct *tsk)
+{
+	struct signal_struct *sig = tsk->signal;
+	int err = -EINTR;
+
+	if (thread_group_empty(tsk))
+		return 0;
+
+	read_lock(&tasklist_lock);
+	spin_lock_irq(&tsk->sighand->siglock);
+	if (!signal_group_exit(sig)) {
+		sig->group_exit_task = tsk;
+		sig->notify_count = -zap_other_threads(tsk);
+		err = 0;
+	}
+	spin_unlock_irq(&tsk->sighand->siglock);
+	read_unlock(&tasklist_lock);
+
+	if (!err)
+		err = wait_for_notify_count(tsk, sig);
+	return err;
+
+}
+
 /*
- * This function makes sure the current process has its own signal table,
- * so that flush_signal_handlers can later reset the handlers without
- * disturbing other processes.  (Other processes might share the signal
- * table via the CLONE_SIGHAND option to clone().)
+ * This function makes sure the current process has no other threads and
+ * has a private signal table so that flush_signal_handlers() can reset
+ * the handlers without disturbing other processes which might share the
+ * signal table via the CLONE_SIGHAND option to clone().
  */
-static int de_thread(struct task_struct *tsk)
+int de_thread(struct task_struct *tsk)
 {
 	struct signal_struct *sig = tsk->signal;
 	struct sighand_struct *oldsighand = tsk->sighand;
@@ -1051,60 +1100,24 @@ static int de_thread(struct task_struct *tsk)
 	if (thread_group_empty(tsk))
 		goto no_thread_group;
 
-	/*
-	 * Kill all other threads in the thread group.
-	 */
 	spin_lock_irq(lock);
-	if (signal_group_exit(sig)) {
-		/*
-		 * Another group action in progress, just
-		 * return so that the signal is processed.
-		 */
-		spin_unlock_irq(lock);
-		return -EAGAIN;
-	}
-
-	sig->group_exit_task = tsk;
-	sig->notify_count = zap_other_threads(tsk);
+	sig->notify_count = sig->nr_threads - 1;
 	if (!thread_group_leader(tsk))
 		sig->notify_count--;
-
-	while (sig->notify_count) {
-		__set_current_state(TASK_KILLABLE);
-		spin_unlock_irq(lock);
-		schedule();
-		if (unlikely(__fatal_signal_pending(tsk)))
-			goto killed;
-		spin_lock_irq(lock);
-	}
 	spin_unlock_irq(lock);
 
+	if (wait_for_notify_count(tsk, sig))
+		return -EINTR;
+
 	/*
 	 * At this point all other threads have exited, all we have to
-	 * do is to wait for the thread group leader to become inactive,
-	 * and to assume its PID:
+	 * do is to reap the old leader and assume its PID.
 	 */
 	if (!thread_group_leader(tsk)) {
 		struct task_struct *leader = tsk->group_leader;
 
-		for (;;) {
-			threadgroup_change_begin(tsk);
-			write_lock_irq(&tasklist_lock);
-			/*
-			 * Do this under tasklist_lock to ensure that
-			 * exit_notify() can't miss ->group_exit_task
-			 */
-			sig->notify_count = -1;
-			if (likely(leader->exit_state))
-				break;
-			__set_current_state(TASK_KILLABLE);
-			write_unlock_irq(&tasklist_lock);
-			threadgroup_change_end(tsk);
-			schedule();
-			if (unlikely(__fatal_signal_pending(tsk)))
-				goto killed;
-		}
-
+		threadgroup_change_begin(tsk);
+		write_lock_irq(&tasklist_lock);
 		/*
 		 * The only record we have of the real-time age of a
 		 * process, regardless of execs it's done, is start_time.
@@ -1162,10 +1175,9 @@ static int de_thread(struct task_struct *tsk)
 		release_task(leader);
 	}
 
+no_thread_group:
 	sig->group_exit_task = NULL;
 	sig->notify_count = 0;
-
-no_thread_group:
 	/* we have changed execution domain */
 	tsk->exit_signal = SIGCHLD;
 
@@ -1198,16 +1210,10 @@ static int de_thread(struct task_struct *tsk)
 	}
 
 	BUG_ON(!thread_group_leader(tsk));
+	flush_signal_handlers(current, 0);
 	return 0;
-
-killed:
-	/* protects against exit_notify() and __exit_signal() */
-	read_lock(&tasklist_lock);
-	sig->group_exit_task = NULL;
-	sig->notify_count = 0;
-	read_unlock(&tasklist_lock);
-	return -EAGAIN;
 }
+EXPORT_SYMBOL(de_thread);
 
 char *get_task_comm(char *buf, struct task_struct *tsk)
 {
@@ -1237,11 +1243,7 @@ int flush_old_exec(struct linux_binprm * bprm)
 {
 	int retval;
 
-	/*
-	 * Make sure we have a private signal table and that
-	 * we are unassociated from the previous thread group.
-	 */
-	retval = de_thread(current);
+	retval = kill_sub_threads(current);
 	if (retval)
 		goto out;
 
@@ -1336,7 +1338,6 @@ void setup_new_exec(struct linux_binprm * bprm)
 	/* An exec changes our domain. We are no longer part of the thread
 	   group */
 	current->self_exec_id++;
-	flush_signal_handlers(current, 0);
 }
 EXPORT_SYMBOL(setup_new_exec);
 
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 1303b57..06a5a7b 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -101,6 +101,7 @@ extern int __must_check remove_arg_zero(struct linux_binprm *);
 extern int search_binary_handler(struct linux_binprm *);
 extern int flush_old_exec(struct linux_binprm * bprm);
 extern void setup_new_exec(struct linux_binprm * bprm);
+extern int de_thread(struct task_struct *tsk);
 extern void would_dump(struct linux_binprm *, struct file *);
 
 extern int suid_dumpable;
diff --git a/kernel/exit.c b/kernel/exit.c
index 8f14b86..169d9f2 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -699,8 +699,9 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	if (tsk->exit_state == EXIT_DEAD)
 		list_add(&tsk->ptrace_entry, &dead);
 
-	/* mt-exec, de_thread() is waiting for group leader */
-	if (unlikely(tsk->signal->notify_count < 0))
+	/* mt-exec, kill_sub_threads() is waiting for group exit */
+	if (unlikely(tsk->signal->notify_count < 0) &&
+	    !++tsk->signal->notify_count)
 		wake_up_process(tsk->signal->group_exit_task);
 	write_unlock_irq(&tasklist_lock);
 
diff --git a/kernel/signal.c b/kernel/signal.c
index 3603d93..b78ce63 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1200,13 +1200,12 @@ int zap_other_threads(struct task_struct *p)
 
 	while_each_thread(p, t) {
 		task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
-		count++;
-
 		/* Don't bother with already dead threads */
 		if (t->exit_state)
 			continue;
 		sigaddset(&t->pending.signal, SIGKILL);
 		signal_wake_up(t, 1);
+		count++;
 	}
 
 	return count;
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1582502 — Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

I am slowly working my way through this and I have found something that
I have an issue with.

Oleg Nesterov <oleg@redhat.com> 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 <kosaki.motohiro@jp.fujitsu.com>
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 <kosaki.motohiro@jp.fujitsu.com>
    Reviewed-by: Oleg Nesterov <oleg@redhat.com>
    Acked-by: Roland McGrath <roland@redhat.com>
    Acked-by: David Howells <dhowells@redhat.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

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

[toc] | [prev] | [next] | [standalone]

#1584665 — Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

Eric,

Thanks for looking into this! and sorry for delay.

On 02/17, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> 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.

Yes sure.

But I meant execve() should not take cred_guard_mutex at the start, it
should take it later even if we do not rework the security hooks. At least
it should take it after copy_strings(), but probably this needs some work.

> 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 <kosaki.motohiro@jp.fujitsu.com>
> 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.

Yes, and let me explain the original motivation for this change.

To remind, we had a problem with copy_strings() which can use a lot of
memory, and this memory was not visible to OOM-killer.

So we were going to add the new member,

	signal_struct->in_exec_mm = bprm->mm

and change OOM-killer to account both task->mm and task->signal->in_exec_mm.

And in this case we obviously need to ensure that only one thread
can enter exec and use signal_struct->in_exec_mm.

That patch was ready, but then we found another (better) solution:
3c77f8457221 ("exec: make argv/envp memory visible to oom-killer").

So I do not think we need to exclude other threads today, and we do
not need to hold cred_guard_mutex throughout the whole execve path.

Again, this needs some work. For example check_unsafe_exec() assumes
it can't race with another thread, see 9e00cdb091b008cb3c78192651180
"exec:check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic".
But this looks solvable.


> So while I fully agree we have issues here that we need to address and
> fix your patch description does not inspire confidence.

See above... what do you think I should change in this part of changelog?

Thanks,

Oleg.

[toc] | [prev] | [next] | [standalone]

#1584675 — Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

On 02/20, Oleg Nesterov wrote:
>
> Again, this needs some work. For example check_unsafe_exec() assumes
> it can't race with another thread, see 9e00cdb091b008cb3c78192651180
> "exec:check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic".
> But this looks solvable.

Forgot to mention... plus check_unsafe_exec() checks ptrace, this is
another reason why we can't simply shift mutex_lock(cred_guard_mutex)
later.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1584944 — Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

Oleg,

My apologies for not replying in-line but I think I can be clearer just
saying what I think needs to be said.

Today cred_guard_mutex is part of making exec appear to be an atomic
operation to ptrace and and proc.  To make exec appear to be atomic
we do need to take the mutex at the beginning and release it at the end
of exec.

The semantics of exec appear atomic to ptrace_attach and to proc readers
are necessary to ensure we use the proper process credentials in the
event of a suid exec.

I believe making cred_guard_mutex per task is an option.  Reducing the
scope of cred_guard_mutex concerns me.  There appear to be some fields
like sighand that we currently expose in proc that might possibly be
problematic.  So I am not certain we protect enough things in our proc
accessors.

Do you know if we can make cred_guard_mutex a per-task lock again?

Eric

[toc] | [prev] | [next] | [standalone]

#1585577 — Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

On 02/21, Eric W. Biederman wrote:
>
> Today cred_guard_mutex is part of making exec appear to be an atomic
> operation to ptrace and and proc.  To make exec appear to be atomic
> we do need to take the mutex at the beginning and release it at the end
> of exec.
>
> The semantics of exec appear atomic to ptrace_attach and to proc readers
> are necessary to ensure we use the proper process credentials in the
> event of a suid exec.

This is clear. My point is that imo a) it is over-used in fs/proc and b)
the scope of this mutex if execve is too huge. I see absolutely no reason
to do copy_strings() with this mutex held, for example. And note that
copy_strings() can use a lot of memory/time, it can trigger oom,swapping,
etc.

But let me repeat, this is a bit off-topic right now, this patch doesn't
change anything in this respect, afaics.


> I believe making cred_guard_mutex per task is an option.  Reducing the
> scope of cred_guard_mutex concerns me.  There appear to be some fields
> like sighand that we currently expose in proc

please see another email, collect_sigign_sigcatch() is called without this
mutex.

> Do you know if we can make cred_guard_mutex a per-task lock again?

I think we can, but this needs some (afaics simple) changes too.

But for what? Note that the problem fixed by this series won't go away
if we do this.


So what do you think about this series?

Oleg.

[toc] | [prev] | [next] | [standalone]

#1585715 — Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

Oleg Nesterov <oleg@redhat.com> writes:

> On 02/21, Eric W. Biederman wrote:
>>
>> Today cred_guard_mutex is part of making exec appear to be an atomic
>> operation to ptrace and and proc.  To make exec appear to be atomic
>> we do need to take the mutex at the beginning and release it at the end
>> of exec.
>>
>> The semantics of exec appear atomic to ptrace_attach and to proc readers
>> are necessary to ensure we use the proper process credentials in the
>> event of a suid exec.
>
> This is clear. My point is that imo a) it is over-used in fs/proc and b)
> the scope of this mutex if execve is too huge. I see absolutely no reason
> to do copy_strings() with this mutex held, for example. And note that
> copy_strings() can use a lot of memory/time, it can trigger oom,swapping,
> etc.

I agree that we can do things like copy_strings that don't change the
data structures and of the task without before taking the cred_guard_mutex.

> But let me repeat, this is a bit off-topic right now, this patch doesn't
> change anything in this respect, afaics.
>
>
>> I believe making cred_guard_mutex per task is an option.  Reducing the
>> scope of cred_guard_mutex concerns me.  There appear to be some fields
>> like sighand that we currently expose in proc
>
> please see another email, collect_sigign_sigcatch() is called without this
> mutex.

I agree that it is called without the mutex.  It is not clear to me that
is the correct behavior.  It violates the fundamental property that
exec of a setuid executable should be an atomic operation.  I don't know
how much we care but it disturbs me that we can read something of a
processes signal handling state with the wrong credentials.

Adopting an implementation where we can never fix this apparent bug
really really disturbs me.

>> Do you know if we can make cred_guard_mutex a per-task lock again?
>
> I think we can, but this needs some (afaics simple) changes too.
>
> But for what? Note that the problem fixed by this series won't go away
> if we do this.

I believe it will if the other waiters use mutex_lock_killable.

> So what do you think about this series?

I like the second patch.  That seems clean and reasonable.

I really don't like the first patch.  It makes an information leak part
a required detail of the implementation and as such possibly something
we can never change.  It attempts to paint a picture for a full fix in
the future that appears to result in an incorrect kernel.  That really
bugs me.

I suspect that a good fix that respects that proc and ptrace_attach need
to exclude the setuid exec case for semantic reasons would have a similar
complexity.

I think a mutex doing the job that cred_guard_mutex is doing especially
when we have multiple readers and a single writer is the wrong locking
primative.  A reader-writer lock or something even cheaper would
probably be much better.

I think fixing the deadlock is important.

I think structuring the fix in such a way that the code is easily
maintainable in the future and is also very important.

Right now it feels like your fix in patch 1 makes things a bit more
brittle and I don't like that at all.

Eric

[toc] | [prev] | [next] | [standalone]

#1586365 — Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

On 02/22, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> >> Reducing the
> >> scope of cred_guard_mutex concerns me.  There appear to be some fields
> >> like sighand that we currently expose in proc
> >
> > please see another email, collect_sigign_sigcatch() is called without this
> > mutex.
>
> I agree that it is called without the mutex.  It is not clear to me that
> is the correct behavior.

I fail to understand how/why this can be wrong.

> >> Do you know if we can make cred_guard_mutex a per-task lock again?
> >
> > I think we can, but this needs some (afaics simple) changes too.
> >
> > But for what? Note that the problem fixed by this series won't go away
> > if we do this.
>
> I believe it will if the other waiters use mutex_lock_killable.

No. They already use mutex_lock_killable/interruptible. And the test-case
can be killed, it is not the hard-lockup.

> I really don't like the first patch.

Just in case, I don't really like it too. Simply because it makes execve
more complex, we need to wait for sub-threads twice.

> It makes an information leak part
> a required detail of the implementation and as such possibly something
> we can never change.

Again, I simply can't understand how flush_signal_handlers() outside of
cred_guard_mutex can be treated as information leak. Even _if_
collect_sigign_sigcatch() was called with this mutex held.

Or do you mean something else?

> I suspect that a good fix that respects that proc and ptrace_attach need
> to exclude the setuid exec case for semantic reasons would have a similar
> complexity.

I am not sure I understand how we can do this. We need cred_guard_mutex
or something else even if exec is not setuid and does not change the
credentials, an LSM module can nack exec-under-ptrace by any reason.

> I think fixing the deadlock is important.

Yes. People actually hit this bug, it was reported several times.

> Right now it feels like your fix in patch 1 makes things a bit more
> brittle and I don't like that at all.

See above, I am not proud of this change too. I even mentioned on 0/2
that it would be nice to reconsider this change in the long term.

But I do not see another simple and _backportable_ solution for now.

What do you think we can do instead for stable trees?

Oleg.

[toc] | [prev] | [next] | [standalone]

#1583087 — Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

Oleg Nesterov <oleg@redhat.com> writes:

> de_thread() waits for other threads with ->cred_guard_mutex held and this
> is really bad because the time is not bounded, debugger can delay the exit
> and this lock has a lot of users (mostly abusers imo) in fs/proc and more.
> And this leads to deadlock if debugger tries to take the same mutex:

Oleg.  I looked at the history in proc of users of cred_guard_mutex
and the proc users are grabbing cred_guard_mutex for the proper
semantic reasons.  To avoid races with setuid exec that could result in
an information disclosure.

I do agree that a mutex is the wrong data structure for the job
cred_guard_mutex is performing.  The job of sorting ensuring debuggers
and proc processes see either the old version or the new version of the
task.

I need to play with the code but I suspect the best we can handle this
preventing both security issues and problems in the future is to create
a new task struct and populate it appropriate with the new data from
exec (at least in the case of setuid exec).

I am thinking of generalizing the case of a non-leader exec where we
have to assume the leaders pid.

I don't yet know what the performance implications would be but
that would clean the users up a lot.

On that score I believe we can incrementally approach that point and
only grab the cred_guard_mutex in exec if we are performing an exec that
changes the processes credentials.

Right now I don't think it introduces any new security information
disclosures but the moving of flush_signal_handlers outside of
cred_guard_mutex feels wrong.

Eric

[toc] | [prev] | [next] | [standalone]

#1584695 — Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

On 02/17, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > de_thread() waits for other threads with ->cred_guard_mutex held and this
> > is really bad because the time is not bounded, debugger can delay the exit
> > and this lock has a lot of users (mostly abusers imo) in fs/proc and more.
> > And this leads to deadlock if debugger tries to take the same mutex:
>
> Oleg.  I looked at the history in proc of users of cred_guard_mutex
> and the proc users are grabbing cred_guard_mutex for the proper
> semantic reasons.  To avoid races with setuid exec that could result in
> an information disclosure.

This is clear. However I really think it is over-used. For example, I do
think that lock_trace() should die.

OK, of course I can be wrong. And in any case this is almost off-topic
right now, lets discuss this separately.

> On that score I believe we can incrementally approach that point and
> only grab the cred_guard_mutex in exec if we are performing an exec that
> changes the processes credentials.

May be... but afaics this is more complicated because of LSM hooks.
To me one the main problems is that lsm hook can fail if the task is
traced, that is why we can't simply take cred_guard_mutex and check
->ptrace after de_thread(). But again, lets discuss this later.

> Right now I don't think it introduces any new security information
> disclosures but the moving of flush_signal_handlers outside of
> cred_guard_mutex feels wrong.

Why?

Just in case, we can do flush_signal_handlers() only after we unshare
->sighand, that is why it was moved outside of cred_guard_mutex.

But why this is bad? collect_sigign_sigcatch() is called without this
mutex, who else can look at sa.sa_handler?

Oleg.

[toc] | [prev] | [next] | [standalone]

#1587757

Eric,

our discussion was a bit confusing, and it seems that we did not
fully convince each other. So let me ask what do you finally think
about this fix.

Let me repeat. Even if I do not agree with some of your objections,
I do agree that 1/2 does not look nice and clean. And we seem to
agree that either way, with or without this fix, we need more changes
in this area.

But we need a simple and backportable fix for stable trees, say for
rhel7. This bug was reported many times, and this is the simplest
solution I was able to find.

Oleg.


On 02/13, Oleg Nesterov wrote:
>
> Hello,
>
> Lets finally fix this problem, it was reported several times. I still think that
> in the longer term we should (try to) rework the security hooks and (partially)
> revert this change, but this is not trivial and we need something backportable
> anyway.
>
> Eric, Jann, we already discussed this change. 1/2 is the same patch I suggested 3
> months ago except now it compiles and moves flush_signal_handlers() to de_thread().
>
> Both patches ask for subsequent cleanups, see the changelogs.
>
> Oleg.
>
>  arch/x86/ia32/ia32_aout.c |   3 ++
>  fs/binfmt_aout.c          |   3 ++
>  fs/binfmt_elf.c           |   6 ++-
>  fs/binfmt_elf_fdpic.c     |   4 ++
>  fs/binfmt_flat.c          |   3 ++
>  fs/exec.c                 | 128 +++++++++++++++++++++++-----------------------
>  include/linux/binfmts.h   |   1 +
>  kernel/exit.c             |   5 +-
>  kernel/signal.c           |  21 +++++---
>  9 files changed, 101 insertions(+), 73 deletions(-)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web