Re: [PATCH v2 2/8] exec: turn self_exec_id into self_privunit

From: Oleg Nesterov <oleg@redhat.com>
To: Jann Horn <jann@thejh.net>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Roland McGrath <roland@hack.frob.com>,
	John Johansen <john.johansen@canonical.com>,
	James Morris <james.l.morris@oracle.com>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Paul Moore <aul@paul-moore.com>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	Eric Paris <eparis@parisplace.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Janis Danisevskis <jdanis@google.com>,
	Seth Forshee <seth.forshee@canonical.com>,
	"Eric . Biederman" <ebiederm@xmission.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Benjamin LaHaise <bcrl@kvack.org>,
	Ben Hutchings <ben@decadent.org.uk>,
	Andy Lutomirski <luto@amacapital.net>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, security@kernel.org
Subject: Re: [PATCH v2 2/8] exec: turn self_exec_id into self_privunit
Date: Fri, 30 Sep 2016 15:20:47 +0200	[thread overview]
Message-ID: <20160930132046.GA12047@redhat.com> (raw)
In-Reply-To: <1474663238-22134-3-git-send-email-jann@thejh.net>

On 09/23, Jann Horn wrote:
>
> One reason for doing this is that it prevents an attacker from sending an
> arbitrary signal to a parent process after performing 2^32-1 execve()
> calls.

I think we should simply kill self/parent_exec_id's. I am going to send
the patch below after re-check/testing.

Oleg.

--- x/include/linux/sched.h
+++ x/include/linux/sched.h
@@ -1677,9 +1677,6 @@ struct task_struct {
 #endif
 	struct seccomp seccomp;
 
-/* Thread group tracking */
-   	u32 parent_exec_id;
-   	u32 self_exec_id;
 /* Protection of (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed,
  * mempolicy */
 	spinlock_t alloc_lock;
--- x/fs/exec.c
+++ x/fs/exec.c
@@ -1163,6 +1163,14 @@ static int de_thread(struct task_struct 
 no_thread_group:
 	/* we have changed execution domain */
 	tsk->exit_signal = SIGCHLD;
+	if (!list_empty(&father->children)) {
+		struct task_struct *child;
+
+		read_lock(&tasklist_lock);
+		list_for_each_entry(child, &father->children, sibling)
+			child->exit_signal = SIGCHLD;
+		read_unlock(&tasklist_lock);
+	}
 
 	exit_itimers(sig);
 	flush_itimer_signals();
@@ -1306,9 +1314,6 @@ void setup_new_exec(struct linux_binprm 
 			set_dumpable(current->mm, suid_dumpable);
 	}
 
-	/* An exec changes our domain. We are no longer part of the thread
-	   group */
-	current->self_exec_id++;
 	flush_signal_handlers(current, 0);
 	do_close_on_exec(current->files);
 }
--- x/kernel/fork.c
+++ x/kernel/fork.c
@@ -1573,13 +1573,10 @@ static struct task_struct *copy_process(
 	write_lock_irq(&tasklist_lock);
 
 	/* CLONE_PARENT re-uses the old parent */
-	if (clone_flags & (CLONE_PARENT|CLONE_THREAD)) {
+	if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
 		p->real_parent = current->real_parent;
-		p->parent_exec_id = current->parent_exec_id;
-	} else {
+	else
 		p->real_parent = current;
-		p->parent_exec_id = current->self_exec_id;
-	}
 
 	spin_lock(&current->sighand->siglock);
 
--- x/kernel/signal.c
+++ x/kernel/signal.c
@@ -1585,15 +1585,6 @@ bool do_notify_parent(struct task_struct
 	BUG_ON(!tsk->ptrace &&
 	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
 
-	if (sig != SIGCHLD) {
-		/*
-		 * This is only possible if parent == real_parent.
-		 * Check if it has changed security domain.
-		 */
-		if (tsk->parent_exec_id != tsk->parent->self_exec_id)
-			sig = SIGCHLD;
-	}
-
 	info.si_signo = sig;
 	info.si_errno = 0;
 	/*

