All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jann Horn <jann@thejh.net>
To: Alexander Viro <viro@zeniv.linux.org.uk>,
	Roland McGrath <roland@hack.frob.com>,
	Oleg Nesterov <oleg@redhat.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>
Cc: linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, security@kernel.org
Subject: [PATCH v2 5/8] proc: lock properly in ptrace_may_access callers
Date: Fri, 23 Sep 2016 22:40:35 +0200	[thread overview]
Message-ID: <1474663238-22134-6-git-send-email-jann@thejh.net> (raw)
In-Reply-To: <1474663238-22134-1-git-send-email-jann@thejh.net>

Use the new cred_guard_light to prevent information leaks
through races in procfs.

changed in v2:
 - also use mm_access() for proc_map_files_readdir() (0day test robot)

Signed-off-by: Jann Horn <jann@thejh.net>
---
 fs/proc/array.c      |   7 +++
 fs/proc/base.c       | 134 ++++++++++++++++++++++++++++++---------------------
 fs/proc/namespaces.c |  14 ++++++
 3 files changed, 99 insertions(+), 56 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 3349742..c28f254 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -410,9 +410,15 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	unsigned long rsslim = 0;
 	char tcomm[sizeof(task->comm)];
 	unsigned long flags;
+	int err;
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
+
+	err = mutex_lock_killable(&task->signal->cred_guard_light);
+	if (err)
+		return err;
+
 	permitted = proc_ptrace_may_access_seq(task,
 			PTRACE_MODE_READ_FSCREDS | PTRACE_MODE_NOAUDIT, m);
 	mm = get_task_mm(task);
@@ -568,6 +574,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	seq_putc(m, '\n');
 	if (mm)
 		mmput(mm);
+	mutex_unlock(&task->signal->cred_guard_light);
 	return 0;
 }
 
diff --git a/fs/proc/base.c b/fs/proc/base.c
index d6c98ab..15845cf 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -135,6 +135,25 @@ struct pid_entry {
 		NULL, &proc_single_file_operations,	\
 		{ .proc_show = show } )
 
+static int lock_trace(struct seq_file *m, struct task_struct *task,
+		      unsigned int mode)
+{
+	int err = mutex_lock_killable(&task->signal->cred_guard_light);
+
+	if (err)
+		return err;
+	if (!proc_ptrace_may_access_seq(task, mode | PTRACE_MODE_FSCREDS, m)) {
+		mutex_unlock(&task->signal->cred_guard_light);
+		return -EPERM;
+	}
+	return 0;
+}
+
+static void unlock_trace(struct task_struct *task)
+{
+	mutex_unlock(&task->signal->cred_guard_light);
+}
+
 /*
  * Count the number of hardlinks for the pid_entry table, excluding the .
  * and .. links.
@@ -428,36 +447,20 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
 	unsigned long wchan;
 	char symname[KSYM_NAME_LEN];
 
-	wchan = get_wchan(task);
-
-	if (wchan &&
-	    proc_ptrace_may_access_seq(task, PTRACE_MODE_READ_FSCREDS, m) &&
-	    !lookup_symbol_name(wchan, symname))
-		seq_printf(m, "%s", symname);
-	else
-		seq_putc(m, '0');
+	if (lock_trace(m, task, PTRACE_MODE_READ) == 0) {
+		wchan = get_wchan(task);
+		unlock_trace(task);
+		if (wchan && !lookup_symbol_name(wchan, symname)) {
+			seq_printf(m, "%s", symname);
+			return 0;
+		}
+	}
 
+	seq_putc(m, '0');
 	return 0;
 }
 #endif /* CONFIG_KALLSYMS */
 
-static int lock_trace(struct seq_file *m, struct task_struct *task)
-{
-	int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
-	if (err)
-		return err;
-	if (!proc_ptrace_may_access_seq(task, PTRACE_MODE_ATTACH_FSCREDS, m)) {
-		mutex_unlock(&task->signal->cred_guard_mutex);
-		return -EPERM;
-	}
-	return 0;
-}
-
-static void unlock_trace(struct task_struct *task)
-{
-	mutex_unlock(&task->signal->cred_guard_mutex);
-}
-
 #ifdef CONFIG_STACKTRACE
 
 #define MAX_STACK_TRACE_DEPTH	64
@@ -479,7 +482,7 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 	trace.entries		= entries;
 	trace.skip		= 0;
 
-	err = lock_trace(m, task);
+	err = lock_trace(m, task, PTRACE_MODE_ATTACH);
 	if (!err) {
 		save_stack_trace_tsk(task, &trace);
 
@@ -661,7 +664,7 @@ static int proc_pid_syscall(struct seq_file *m, struct pid_namespace *ns,
 	unsigned long args[6], sp, pc;
 	int res;
 
-	res = lock_trace(m, task);
+	res = lock_trace(m, task, PTRACE_MODE_ATTACH);
 	if (res)
 		return res;
 
@@ -685,23 +688,38 @@ static int proc_pid_syscall(struct seq_file *m, struct pid_namespace *ns,
 /*                       Here the fs part begins                        */
 /************************************************************************/
 
-/* permission checks */
-static int proc_fd_access_allowed(struct inode *inode)
+/* permission checks.
+ * If this returns 1, you'll have to unlock_trace(*taskp) afterwards.
+ */
+static int proc_fd_access_allowed_lock(struct inode *inode,
+					 struct task_struct **taskp)
 {
 	struct task_struct *task;
 	int allowed = 0;
-	/* Allow access to a task's file descriptors if it is us or we
-	 * may use ptrace attach to the process and find out that
-	 * information.
-	 */
+
 	task = get_proc_task(inode);
-	if (task) {
-		allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
+	if (!task)
+		return 0;
+	if (mutex_lock_killable(&task->signal->cred_guard_light))
+		goto out_put;
+	allowed = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
+	if (!allowed)
+		mutex_unlock(&task->signal->cred_guard_light);
+out_put:
+	if (allowed)
+		*taskp = task;
+	else
 		put_task_struct(task);
-	}
+
 	return allowed;
 }
 
+static void proc_fd_access_allowed_unlock(struct task_struct *task)
+{
+	mutex_unlock(&task->signal->cred_guard_light);
+	put_task_struct(task);
+}
+
 int proc_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	int error;
@@ -722,6 +740,7 @@ int proc_setattr(struct dentry *dentry, struct iattr *attr)
 /*
  * May current process learn task's sched/cmdline info (for hide_pid_min=1)
  * or euid/egid (for hide_pid_min=2)?
+ * NOTE: When you call this, you must hold cred_guard_mutex or cred_guard_light.
  */
 static bool has_pid_permissions(struct pid_namespace *pid,
 				 struct task_struct *task,
@@ -1600,15 +1619,17 @@ static const char *proc_pid_get_link(struct dentry *dentry,
 {
 	struct path path;
 	int error = -EACCES;
+	struct task_struct *task;
 
 	if (!dentry)
 		return ERR_PTR(-ECHILD);
 
 	/* Are we allowed to snoop on the tasks file descriptors? */
-	if (!proc_fd_access_allowed(inode))
+	if (!proc_fd_access_allowed_lock(inode, &task))
 		goto out;
 
 	error = PROC_I(inode)->op.proc_get_link(dentry, &path);
+	proc_fd_access_allowed_unlock(task);
 	if (error)
 		goto out;
 
@@ -1647,12 +1668,14 @@ static int proc_pid_readlink(struct dentry * dentry, char __user * buffer, int b
 	int error = -EACCES;
 	struct inode *inode = d_inode(dentry);
 	struct path path;
+	struct task_struct *task;
 
 	/* Are we allowed to snoop on the tasks file descriptors? */
-	if (!proc_fd_access_allowed(inode))
+	if (!proc_fd_access_allowed_lock(inode, &task))
 		goto out;
 
 	error = PROC_I(inode)->op.proc_get_link(dentry, &path);
+	proc_fd_access_allowed_unlock(task);
 	if (error)
 		goto out;
 
@@ -2047,17 +2070,15 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 	if (!task)
 		goto out;
 
-	result = -EACCES;
-	if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
+	mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
+	if (IS_ERR(mm))
+		result = PTR_ERR(mm);
+	if (IS_ERR_OR_NULL(mm))
 		goto out_put_task;
 
 	result = -ENOENT;
 	if (dname_to_vma_addr(dentry, &vm_start, &vm_end))
-		goto out_put_task;
-
-	mm = get_task_mm(task);
-	if (!mm)
-		goto out_put_task;
+		goto out_put_mm;
 
 	down_read(&mm->mmap_sem);
 	vma = find_exact_vma(mm, vm_start, vm_end);
@@ -2070,6 +2091,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 
 out_no_vma:
 	up_read(&mm->mmap_sem);
+out_put_mm:
 	mmput(mm);
 out_put_task:
 	put_task_struct(task);
@@ -2100,17 +2122,17 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
 	if (!task)
 		goto out;
 
-	ret = -EACCES;
-	if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
+	ret = 0;
+
+	mm = mm_access(task, PTRACE_MODE_READ_FSCREDS);
+	if (IS_ERR(mm))
+		ret = PTR_ERR(mm);
+	if (IS_ERR_OR_NULL(mm))
 		goto out_put_task;
 
-	ret = 0;
 	if (!dir_emit_dots(file, ctx))
-		goto out_put_task;
+		goto out_mmput;
 
-	mm = get_task_mm(task);
-	if (!mm)
-		goto out_put_task;
 	down_read(&mm->mmap_sem);
 
 	nr_files = 0;
@@ -2139,8 +2161,7 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
 			if (fa)
 				flex_array_free(fa);
 			up_read(&mm->mmap_sem);
-			mmput(mm);
-			goto out_put_task;
+			goto out_mmput;
 		}
 		for (i = 0, vma = mm->mmap, pos = 2; vma;
 				vma = vma->vm_next) {
@@ -2171,8 +2192,9 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
 	}
 	if (fa)
 		flex_array_free(fa);
-	mmput(mm);
 
+out_mmput:
+	mmput(mm);
 out_put_task:
 	put_task_struct(task);
 out:
@@ -2839,7 +2861,7 @@ static const struct file_operations proc_setgroups_operations = {
 static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
 				struct pid *pid, struct task_struct *task)
 {
-	int err = lock_trace(m, task);
+	int err = lock_trace(m, task, PTRACE_MODE_ATTACH);
 	if (!err) {
 		seq_printf(m, "%08x\n", task->personality);
 		unlock_trace(task);
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index 51b8b0a..e1246e8 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -49,11 +49,18 @@ static const char *proc_ns_get_link(struct dentry *dentry,
 	if (!task)
 		return error;
 
+	error = ERR_PTR(mutex_lock_killable(&task->signal->cred_guard_light));
+	if (error)
+		goto out_put_task;
+
+	error = ERR_PTR(-EACCES);
 	if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
 		error = ns_get_path(&ns_path, task, ns_ops);
 		if (!error)
 			nd_jump_link(&ns_path);
 	}
+	mutex_unlock(&task->signal->cred_guard_light);
+out_put_task:
 	put_task_struct(task);
 	return error;
 }
@@ -70,11 +77,18 @@ static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int bufl
 	if (!task)
 		return res;
 
+	res = mutex_lock_killable(&task->signal->cred_guard_light);
+	if (res)
+		goto out_put_task;
+
+	res = -EACCES;
 	if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
 		res = ns_get_name(name, sizeof(name), task, ns_ops);
 		if (res >= 0)
 			res = readlink_copy(buffer, buflen, name);
 	}
+	mutex_unlock(&task->signal->cred_guard_light);
+out_put_task:
 	put_task_struct(task);
 	return res;
 }
-- 
2.1.4


  parent reply	other threads:[~2016-09-23 20:40 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-23 20:40 [PATCH v2 0/8] Various fixes related to ptrace_may_access() Jann Horn
2016-09-23 20:40 ` [PATCH v2 1/8] exec: introduce cred_guard_light Jann Horn
2016-09-30 15:35   ` Oleg Nesterov
2016-09-30 18:27     ` Eric W. Biederman
2016-10-03 16:02       ` Oleg Nesterov
2016-10-30 21:12     ` Jann Horn
2016-09-23 20:40 ` [PATCH v2 2/8] exec: turn self_exec_id into self_privunit Jann Horn
2016-09-23 21:04   ` Andy Lutomirski
2016-09-23 21:33     ` Jann Horn
2016-09-30 13:20   ` Oleg Nesterov
2016-09-30 13:44     ` Oleg Nesterov
2016-09-30 18:30       ` Kees Cook
2016-09-30 18:59         ` Jann Horn
2016-09-30 19:05           ` Kees Cook
2016-10-03 16:37         ` Oleg Nesterov
2016-09-23 20:40 ` [PATCH v2 3/8] proc: use open()-time creds for ptrace checks Jann Horn
2016-09-23 20:40 ` [PATCH v2 4/8] futex: don't leak robust_list pointer Jann Horn
2016-09-30 14:52   ` Oleg Nesterov
2016-10-30 17:16     ` Jann Horn
2016-11-02 21:39       ` Jann Horn
2016-11-02 22:47         ` Jann Horn
2016-09-23 20:40 ` Jann Horn [this message]
2016-09-23 20:40 ` [PATCH v2 6/8] ptrace: warn on ptrace_may_access without proper locking Jann Horn
2016-09-23 20:40 ` [PATCH v2 7/8] fs/proc: fix attr access check Jann Horn
2016-09-23 20:40 ` [PATCH v2 8/8] Documentation: add security/ptrace_checks.txt Jann Horn
2016-10-02  3:16   ` Krister Johansen
2016-10-30 19:09     ` Jann Horn
2016-10-31  4:14       ` Eric W. Biederman
2016-10-31 13:39         ` Jann Horn
2016-11-03 20:43         ` Krister Johansen

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=1474663238-22134-6-git-send-email-jann@thejh.net \
    --to=jann@thejh.net \
    --cc=akpm@linux-foundation.org \
    --cc=aul@paul-moore.com \
    --cc=bcrl@kvack.org \
    --cc=ben@decadent.org.uk \
    --cc=casey@schaufler-ca.com \
    --cc=ebiederm@xmission.com \
    --cc=eparis@parisplace.org \
    --cc=james.l.morris@oracle.com \
    --cc=jdanis@google.com \
    --cc=john.johansen@canonical.com \
    --cc=keescook@chromium.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=oleg@redhat.com \
    --cc=roland@hack.frob.com \
    --cc=sds@tycho.nsa.gov \
    --cc=security@kernel.org \
    --cc=serge@hallyn.com \
    --cc=seth.forshee@canonical.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.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.