From patchwork Sun Oct 30 21:46:35 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jann Horn X-Patchwork-Id: 9404739 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 84C166022E for ; Sun, 30 Oct 2016 21:47:00 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 80F9628E58 for ; Sun, 30 Oct 2016 21:47:00 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 73EDB28E73; Sun, 30 Oct 2016 21:47:00 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E404328E58 for ; Sun, 30 Oct 2016 21:46:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753837AbcJ3Vq4 (ORCPT ); Sun, 30 Oct 2016 17:46:56 -0400 Received: from thejh.net ([37.221.195.125]:54398 "EHLO thejh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751378AbcJ3Vqv (ORCPT ); Sun, 30 Oct 2016 17:46:51 -0400 Received: from pc.thejh.net (pc.vpn [192.168.44.2]) by thejh.net (Postfix) with ESMTPSA id 4630E181353; Sun, 30 Oct 2016 22:46:49 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=thejh.net; s=s2016; t=1477864010; bh=zE0y0uAOi1fjUNleKupH6QhVgSbt+gsZuqKJk1bRsSo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=0xm6wajAnWvo1DjUGhkAUcY+SHIvgm/DsYDOWSJJdDb3w8rGBkI3hx26rlxuc0Y51 GIIzURipnSPgOIsJAkI2sgZrbIsHf2gIRvlmvYwjNqKkZuGCdUpf2EwEeBiseKnuec pJv8dxhl/PdCvVoeNUcOZwRQkMsuHSWfpFmDb9u7Aat1eufNlJoEqOy2cRyvsXPBsT DinmuZtV9jfHRiDg03dTP+/APqSiHLw4zZPshkRbiV/XmX5TXpr5mgJBVPVlL0AZ1m UMrB8VyHFA4zg/A/TMMO+iXI4GZXpXMdIPRVPKfQSkC9rs/o3ZTnLNeDe8CyWM8Zcy l1Oh4pKG6amXA== From: Jann Horn To: Alexander Viro , Roland McGrath , Oleg Nesterov , John Johansen , James Morris , "Serge E. Hallyn" , Paul Moore , Stephen Smalley , Eric Paris , Casey Schaufler , Kees Cook , Andrew Morton , Janis Danisevskis , Seth Forshee , "Eric W. Biederman" , Thomas Gleixner , Benjamin LaHaise , Ben Hutchings , Andy Lutomirski , Linus Torvalds , Krister Johansen 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 Message-Id: <1477863998-3298-6-git-send-email-jann@thejh.net> X-Mailer: git-send-email 2.1.4 In-Reply-To: <1477863998-3298-1-git-send-email-jann@thejh.net> References: <1477863998-3298-1-git-send-email-jann@thejh.net> Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- 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; }