All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Al Viro <viro@ZenIV.linux.org.uk>,
	Kees Cook <keescook@chromium.org>,
	KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>,
	Michal Hocko <mhocko@suse.cz>, Sameer Nanda <snanda@chromium.org>,
	Sergey Dyasly <dserrg@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic
Date: Fri, 22 Nov 2013 18:54:42 +0100	[thread overview]
Message-ID: <20131122175442.GA31453@redhat.com> (raw)
In-Reply-To: <20131122175424.GA31432@redhat.com>

fs_struct->in_exec == T means that this ->fs is used by a single
process (thread group), and one of the treads does do_execve().

To avoid the mt-exec races this code has the following complications:

	1. check_unsafe_exec() returns -EBUSY if ->in_exec was
	   already set by another thread.

	2. do_execve_common() records "clear_in_exec" to ensure
	   that the error path can only clear ->in_exec if it was
	   set by current.

However, after 9b1bf12d5d51 "signals: move cred_guard_mutex from
task_struct to signal_struct" we do not need these complications:

	1. We can't race with our sub-thread, this is called under
	   per-process ->cred_guard_mutex. And we can't race with
	   another CLONE_FS task, we already checked that this fs
	   is not shared.

	   We can remove the  dead -EAGAIN logic.

	2. "out_unmark:" in do_execve_common() is either called
	   under ->cred_guard_mutex, or after de_thread() which
	   kills other threads, so we can't race with sub-thread
	   which could set ->in_exec. And if ->fs is shared with
	   another process ->in_exec should be false anyway.

	   We can clear in_exec unconditionally.

This also means that check_unsafe_exec() can be void.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/exec.c |   29 ++++++++---------------------
 1 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 0cd9c25..60eb5c5 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1225,11 +1225,10 @@ EXPORT_SYMBOL(install_exec_creds);
  * - the caller must hold ->cred_guard_mutex to protect against
  *   PTRACE_ATTACH
  */
-static int check_unsafe_exec(struct linux_binprm *bprm)
+static void check_unsafe_exec(struct linux_binprm *bprm)
 {
 	struct task_struct *p = current, *t;
 	unsigned n_fs;
-	int res = 0;
 
 	if (p->ptrace) {
 		if (p->ptrace & PT_PTRACE_CAP)
@@ -1255,22 +1254,15 @@ static int check_unsafe_exec(struct linux_binprm *bprm)
 	}
 	rcu_read_unlock();
 
-	if (p->fs->users > n_fs) {
+	if (p->fs->users > n_fs)
 		bprm->unsafe |= LSM_UNSAFE_SHARE;
-	} else {
-		res = -EAGAIN;
-		if (!p->fs->in_exec) {
-			p->fs->in_exec = 1;
-			res = 1;
-		}
-	}
+	else
+		p->fs->in_exec = 1;
 	spin_unlock(&p->fs->lock);
-
-	return res;
 }
 
-/* 
- * Fill the binprm structure from the inode. 
+/*
+ * Fill the binprm structure from the inode.
  * Check permissions, then read the first 128 (BINPRM_BUF_SIZE) bytes
  *
  * This may be called multiple times for binary chains (scripts for example).
@@ -1461,7 +1453,6 @@ static int do_execve_common(const char *filename,
 	struct linux_binprm *bprm;
 	struct file *file;
 	struct files_struct *displaced;
-	bool clear_in_exec;
 	int retval;
 
 	/*
@@ -1493,10 +1484,7 @@ static int do_execve_common(const char *filename,
 	if (retval)
 		goto out_free;
 
-	retval = check_unsafe_exec(bprm);
-	if (retval < 0)
-		goto out_free;
-	clear_in_exec = retval;
+	check_unsafe_exec(bprm);
 	current->in_execve = 1;
 
 	file = open_exec(filename);
@@ -1565,8 +1553,7 @@ out_file:
 	}
 
 out_unmark:
-	if (clear_in_exec)
-		current->fs->in_exec = 0;
+	current->fs->in_exec = 0;
 	current->in_execve = 0;
 
 out_free:
-- 
1.5.5.1


  parent reply	other threads:[~2013-11-22 17:53 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-22 17:54 [PATCH 0/4] in_exec/etc cleanups Oleg Nesterov
2013-11-22 17:54 ` [PATCH 1/4] check_unsafe_exec: use while_each_thread() rather than next_thread() Oleg Nesterov
2013-11-22 19:42   ` KOSAKI Motohiro
2013-11-22 20:24     ` Oleg Nesterov
2013-11-22 20:32       ` KOSAKI Motohiro
2013-11-22 17:54 ` Oleg Nesterov [this message]
2013-11-22 20:27   ` [PATCH 2/4] check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic KOSAKI Motohiro
2013-11-22 20:49     ` Oleg Nesterov
2013-11-22 21:00       ` KOSAKI Motohiro
2013-11-23 15:32         ` Oleg Nesterov
2013-11-22 17:54 ` [PATCH 3/4] exec: move the final allow_write_access/fput into free_bprm() Oleg Nesterov
2013-11-22 20:29   ` KOSAKI Motohiro
2013-11-23 19:22   ` Kees Cook
2013-11-22 17:54 ` [PATCH 4/4] kill task_struct->did_exec Oleg Nesterov
2013-11-22 19:46   ` KOSAKI Motohiro
2013-11-22 20:33     ` [PATCH v2 " Oleg Nesterov
2013-11-22 20:33       ` KOSAKI Motohiro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131122175442.GA31453@redhat.com \
    --to=oleg@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=dserrg@gmail.com \
    --cc=keescook@chromium.org \
    --cc=kosaki.motohiro@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhocko@suse.cz \
    --cc=snanda@chromium.org \
    --cc=viro@ZenIV.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.