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 W. 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>,
Krister Johansen <kjlx@templeofstupid.com>
Cc: linux-fsdevel@vger.kernel.org,
linux-security-module@vger.kernel.org, security@kernel.org
Subject: [PATCH v3 5/8] proc: lock properly in ptrace_may_access callers
Date: Sun, 30 Oct 2016 22:46:35 +0100 [thread overview]
Message-ID: <1477863998-3298-6-git-send-email-jann@thejh.net> (raw)
In-Reply-To: <1477863998-3298-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)
changed in v3:
- drop the lock_trace() stuff (Oleg Nesterov)
Signed-off-by: Jann Horn <jann@thejh.net>
---
fs/proc/array.c | 7 +++++
fs/proc/base.c | 77 +++++++++++++++++++++++++++++++++-------------------
fs/proc/namespaces.c | 21 +++++++++++---
3 files changed, 73 insertions(+), 32 deletions(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 524cff7e0104..e180d4d82d21 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -405,9 +405,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);
@@ -564,6 +570,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 d3b20d3a01c9..32ea9bc3d320 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -668,23 +668,39 @@ 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 proc_fd_access_allowed_unlock(*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;
@@ -705,6 +721,8 @@ 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 should hold cred_guard_mutex or
+ * cred_guard_light.
*/
static bool has_pid_permissions(struct pid_namespace *pid,
struct task_struct *task,
@@ -1615,15 +1633,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;
@@ -1662,12 +1682,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;
@@ -2062,17 +2084,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);
@@ -2085,6 +2105,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);
@@ -2115,17 +2136,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;
@@ -2154,8 +2175,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) {
@@ -2186,8 +2206,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:
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index 51b8b0a8ad91..c8dee46939df 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,17 @@ static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int bufl
if (!task)
return res;
- if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
+ 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);
+ if (res >= 0)
+ res = readlink_copy(buffer, buflen, name);
+out_put_task:
put_task_struct(task);
return res;
}
--
2.1.4
next prev parent reply other threads:[~2016-10-30 21:46 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-30 21:46 [PATCH v3 0/8] Various fixes related to ptrace_may_access() Jann Horn
2016-10-30 21:46 ` [PATCH v3 1/8] exec: introduce cred_guard_light Jann Horn
2016-11-02 18:18 ` Oleg Nesterov
2016-11-02 20:50 ` Jann Horn
2016-11-02 21:38 ` Ben Hutchings
2016-11-02 21:54 ` Jann Horn
2016-11-03 18:12 ` Oleg Nesterov
2016-11-03 21:17 ` Jann Horn
2016-11-04 13:26 ` Eric W. Biederman
2016-11-04 15:00 ` Eric W. Biederman
2016-11-04 18:04 ` Oleg Nesterov
2016-11-04 18:45 ` Oleg Nesterov
2016-11-05 14:56 ` Oleg Nesterov
2016-11-09 0:34 ` Eric W. Biederman
2016-11-16 20:03 ` Eric W. Biederman
2016-11-08 22:02 ` Kees Cook
2016-11-08 22:46 ` Eric W. Biederman
2016-11-08 22:56 ` Benjamin LaHaise
2016-11-08 23:33 ` Eric W. Biederman
2016-10-30 21:46 ` [PATCH v3 2/8] exec: add privunit to task_struct Jann Horn
2016-10-30 21:46 ` [PATCH v3 3/8] proc: use open()-time creds for ptrace checks Jann Horn
2016-10-30 21:46 ` [PATCH v3 4/8] futex: don't leak robust_list pointer Jann Horn
2016-10-30 21:46 ` Jann Horn [this message]
2016-10-30 21:46 ` [PATCH v3 6/8] fs/proc: fix attr access check Jann Horn
2016-10-30 21:46 ` [PATCH v3 7/8] proc: fix timerslack_ns handling Jann Horn
2016-10-30 21:46 ` [PATCH v3 8/8] Documentation: add security/ptrace_checks.txt Jann Horn
2016-11-01 23:57 ` [PATCH v3 0/8] Various fixes related to ptrace_may_access() Linus Torvalds
2016-11-02 18:38 ` Oleg Nesterov
2016-11-02 21:40 ` Jann Horn
2016-11-03 19:09 ` Andrew Morton
2016-11-03 20:01 ` Jann Horn
2016-11-04 0:57 ` James Morris
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=1477863998-3298-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=kjlx@templeofstupid.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).