From: ebiederm@xmission.com (Eric W. Biederman)
To: Willy Tarreau <w@1wt.eu>
Cc: Kees Cook <keescook@chromium.org>,
Linux Containers <containers@lists.linux-foundation.org>,
Oleg Nesterov <oleg@redhat.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
Linux FS Devel <linux-fsdevel@vger.kernel.org>,
Michal Hocko <mhocko@kernel.org>, Jann Horn <jann@thejh.net>,
Andy Lutomirski <luto@amacapital.net>
Subject: [REVIEW][PATCH 2/3] ptrace: Don't allow accessing an undumpable mm
Date: Thu, 17 Nov 2016 16:50:35 -0600 [thread overview]
Message-ID: <87shqpzpok.fsf_-_@xmission.com> (raw)
In-Reply-To: <874m3522sy.fsf@xmission.com> (Eric W. Biederman's message of "Thu, 17 Nov 2016 15:51:09 -0600")
It is the reasonable expectation that if an executable file is not
readable there will be no way for a user without special privileges to
read the file. This is enforced in ptrace_attach but if ptrace
is already attached before exec there is no enforcement for read-only
executables.
As the only way to read such an mm is through access_process_vm
spin a variant called ptrace_access_vm that will fail if the
target process is not being ptraced by the current process, or
the current process did not have sufficient privileges when ptracing
began to read the target processes mm.
In the ptrace implementations replace access_process_vm by
ptrace_access_vm. There remain several ptrace sites that still use
access_process_vm as they are reading the target executables
instructions (for kernel consumption) or register stacks. As such it
does not appear necessary to add a permission check to those calls.
This bug has always existed in Linux.
Fixes: v1.0
Cc: stable@vger.kernel.org
Reported-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
arch/alpha/kernel/ptrace.c | 2 +-
arch/blackfin/kernel/ptrace.c | 4 ++--
arch/cris/arch-v32/kernel/ptrace.c | 2 +-
arch/ia64/kernel/ptrace.c | 2 +-
arch/mips/kernel/ptrace32.c | 4 ++--
arch/powerpc/kernel/ptrace32.c | 4 ++--
include/linux/mm.h | 2 ++
include/linux/ptrace.h | 3 +++
kernel/ptrace.c | 41 ++++++++++++++++++++++++++++++++------
mm/memory.c | 2 +-
mm/nommu.c | 2 +-
11 files changed, 51 insertions(+), 17 deletions(-)
diff --git a/arch/alpha/kernel/ptrace.c b/arch/alpha/kernel/ptrace.c
index d9ee81769899..619d8b4bc890 100644
--- a/arch/alpha/kernel/ptrace.c
+++ b/arch/alpha/kernel/ptrace.c
@@ -281,7 +281,7 @@ long arch_ptrace(struct task_struct *child, long request,
/* When I and D space are separate, these will need to be fixed. */
case PTRACE_PEEKTEXT: /* read word at location addr. */
case PTRACE_PEEKDATA:
- copied = access_process_vm(child, addr, &tmp, sizeof(tmp), 0);
+ copied = ptrace_access_vm(child, addr, &tmp, sizeof(tmp), 0);
ret = -EIO;
if (copied != sizeof(tmp))
break;
diff --git a/arch/blackfin/kernel/ptrace.c b/arch/blackfin/kernel/ptrace.c
index 8b8fe671b1a6..7d8ece6a93fb 100644
--- a/arch/blackfin/kernel/ptrace.c
+++ b/arch/blackfin/kernel/ptrace.c
@@ -270,7 +270,7 @@ long arch_ptrace(struct task_struct *child, long request,
switch (bfin_mem_access_type(addr, to_copy)) {
case BFIN_MEM_ACCESS_CORE:
case BFIN_MEM_ACCESS_CORE_ONLY:
- copied = access_process_vm(child, addr, &tmp,
+ copied = ptrace_access_vm(child, addr, &tmp,
to_copy, 0);
if (copied)
break;
@@ -323,7 +323,7 @@ long arch_ptrace(struct task_struct *child, long request,
switch (bfin_mem_access_type(addr, to_copy)) {
case BFIN_MEM_ACCESS_CORE:
case BFIN_MEM_ACCESS_CORE_ONLY:
- copied = access_process_vm(child, addr, &data,
+ copied = ptrace_access_vm(child, addr, &data,
to_copy, 1);
break;
case BFIN_MEM_ACCESS_DMA:
diff --git a/arch/cris/arch-v32/kernel/ptrace.c b/arch/cris/arch-v32/kernel/ptrace.c
index f085229cf870..04251c6cb5f9 100644
--- a/arch/cris/arch-v32/kernel/ptrace.c
+++ b/arch/cris/arch-v32/kernel/ptrace.c
@@ -147,7 +147,7 @@ long arch_ptrace(struct task_struct *child, long request,
/* The trampoline page is globally mapped, no page table to traverse.*/
tmp = *(unsigned long*)addr;
} else {
- copied = access_process_vm(child, addr, &tmp, sizeof(tmp), 0);
+ copied = ptrace_access_vm(child, addr, &tmp, sizeof(tmp), 0);
if (copied != sizeof(tmp))
break;
diff --git a/arch/ia64/kernel/ptrace.c b/arch/ia64/kernel/ptrace.c
index 6f54d511cc50..4c46672f3ac1 100644
--- a/arch/ia64/kernel/ptrace.c
+++ b/arch/ia64/kernel/ptrace.c
@@ -1156,7 +1156,7 @@ arch_ptrace (struct task_struct *child, long request,
case PTRACE_PEEKTEXT:
case PTRACE_PEEKDATA:
/* read word at location addr */
- if (access_process_vm(child, addr, &data, sizeof(data), 0)
+ if (ptrace_access_vm(child, addr, &data, sizeof(data), 0)
!= sizeof(data))
return -EIO;
/* ensure return value is not mistaken for error code */
diff --git a/arch/mips/kernel/ptrace32.c b/arch/mips/kernel/ptrace32.c
index 283b5a1967d1..114b577c5a51 100644
--- a/arch/mips/kernel/ptrace32.c
+++ b/arch/mips/kernel/ptrace32.c
@@ -69,7 +69,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
if (get_user(addrOthers, (u32 __user * __user *) (unsigned long) addr) != 0)
break;
- copied = access_process_vm(child, (u64)addrOthers, &tmp,
+ copied = ptrace_access_vm(child, (u64)addrOthers, &tmp,
sizeof(tmp), 0);
if (copied != sizeof(tmp))
break;
@@ -178,7 +178,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
if (get_user(addrOthers, (u32 __user * __user *) (unsigned long) addr) != 0)
break;
ret = 0;
- if (access_process_vm(child, (u64)addrOthers, &data,
+ if (ptrace_access_vm(child, (u64)addrOthers, &data,
sizeof(data), 1) == sizeof(data))
break;
ret = -EIO;
diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c
index f52b7db327c8..2e4f01dc9d64 100644
--- a/arch/powerpc/kernel/ptrace32.c
+++ b/arch/powerpc/kernel/ptrace32.c
@@ -73,7 +73,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
if (get_user(addrOthers, (u32 __user * __user *)addr) != 0)
break;
- copied = access_process_vm(child, (u64)addrOthers, &tmp,
+ copied = ptrace_access_vm(child, (u64)addrOthers, &tmp,
sizeof(tmp), 0);
if (copied != sizeof(tmp))
break;
@@ -178,7 +178,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
if (get_user(addrOthers, (u32 __user * __user *)addr) != 0)
break;
ret = 0;
- if (access_process_vm(child, (u64)addrOthers, &tmp,
+ if (ptrace_access_vm(child, (u64)addrOthers, &tmp,
sizeof(tmp), 1) == sizeof(tmp))
break;
ret = -EIO;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index e9caec6a51e9..f49727403cce 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1269,6 +1269,8 @@ static inline int fixup_user_fault(struct task_struct *tsk,
extern int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write);
extern int access_remote_vm(struct mm_struct *mm, unsigned long addr,
void *buf, int len, int write);
+extern int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
+ unsigned long addr, void *buf, int len, int write);
long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
unsigned long start, unsigned long nr_pages,
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index e13bfdf7f314..7ef2f2b0a02e 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -8,6 +8,9 @@
#include <linux/pid_namespace.h> /* For task_active_pid_ns. */
#include <uapi/linux/ptrace.h>
+extern int ptrace_access_vm(struct task_struct *tsk, unsigned long addr,
+ void *buf, int len, int write);
+
/*
* Ptrace flags
*
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 982505497680..20288a3b3796 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -27,6 +27,35 @@
#include <linux/cn_proc.h>
#include <linux/compat.h>
+/*
+ * Access another process' address space via ptrace.
+ * Source/target buffer must be kernel space,
+ * Do not walk the page table directly, use get_user_pages
+ */
+int ptrace_access_vm(struct task_struct *tsk, unsigned long addr,
+ void *buf, int len, int write)
+{
+ struct mm_struct *mm;
+ int ret;
+
+ mm = get_task_mm(tsk);
+ if (!mm)
+ return 0;
+
+ if (!tsk->ptrace ||
+ (current != tsk->parent) ||
+ ((get_dumpable(mm) != SUID_DUMP_USER) &&
+ !ptracer_capable(tsk, mm->user_ns))) {
+ mmput(mm);
+ return 0;
+ }
+
+ ret = __access_remote_vm(tsk, mm, addr, buf, len, write);
+ mmput(mm);
+
+ return ret;
+}
+
/*
* ptrace a task: make the debugger its new parent and
@@ -535,7 +564,7 @@ int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst
int this_len, retval;
this_len = (len > sizeof(buf)) ? sizeof(buf) : len;
- retval = access_process_vm(tsk, src, buf, this_len, 0);
+ retval = ptrace_access_vm(tsk, src, buf, this_len, 0);
if (!retval) {
if (copied)
break;
@@ -562,7 +591,7 @@ int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long ds
this_len = (len > sizeof(buf)) ? sizeof(buf) : len;
if (copy_from_user(buf, src, this_len))
return -EFAULT;
- retval = access_process_vm(tsk, dst, buf, this_len, 1);
+ retval = ptrace_access_vm(tsk, dst, buf, this_len, 1);
if (!retval) {
if (copied)
break;
@@ -1125,7 +1154,7 @@ int generic_ptrace_peekdata(struct task_struct *tsk, unsigned long addr,
unsigned long tmp;
int copied;
- copied = access_process_vm(tsk, addr, &tmp, sizeof(tmp), 0);
+ copied = ptrace_access_vm(tsk, addr, &tmp, sizeof(tmp), 0);
if (copied != sizeof(tmp))
return -EIO;
return put_user(tmp, (unsigned long __user *)data);
@@ -1136,7 +1165,7 @@ int generic_ptrace_pokedata(struct task_struct *tsk, unsigned long addr,
{
int copied;
- copied = access_process_vm(tsk, addr, &data, sizeof(data), 1);
+ copied = ptrace_access_vm(tsk, addr, &data, sizeof(data), 1);
return (copied == sizeof(data)) ? 0 : -EIO;
}
@@ -1153,7 +1182,7 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
switch (request) {
case PTRACE_PEEKTEXT:
case PTRACE_PEEKDATA:
- ret = access_process_vm(child, addr, &word, sizeof(word), 0);
+ ret = ptrace_access_vm(child, addr, &word, sizeof(word), 0);
if (ret != sizeof(word))
ret = -EIO;
else
@@ -1162,7 +1191,7 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
case PTRACE_POKETEXT:
case PTRACE_POKEDATA:
- ret = access_process_vm(child, addr, &data, sizeof(data), 1);
+ ret = ptrace_access_vm(child, addr, &data, sizeof(data), 1);
ret = (ret != sizeof(data) ? -EIO : 0);
break;
diff --git a/mm/memory.c b/mm/memory.c
index fc1987dfd8cc..87bed1520690 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3868,7 +3868,7 @@ EXPORT_SYMBOL_GPL(generic_access_phys);
* Access another process' address space as given in mm. If non-NULL, use the
* given task for page fault accounting.
*/
-static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
+int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
unsigned long addr, void *buf, int len, int write)
{
struct vm_area_struct *vma;
diff --git a/mm/nommu.c b/mm/nommu.c
index 95daf81a4855..281d5adda9ef 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1816,7 +1816,7 @@ void filemap_map_pages(struct fault_env *fe,
}
EXPORT_SYMBOL(filemap_map_pages);
-static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
+int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
unsigned long addr, void *buf, int len, int write)
{
struct vm_area_struct *vma;
--
2.10.1
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2016-11-17 22:53 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-17 16:39 [REVIEW][PATCH] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access Eric W. Biederman
2016-10-17 17:25 ` Jann Horn
2016-10-17 17:33 ` Eric W. Biederman
2016-10-18 13:50 ` Michal Hocko
2016-10-18 13:57 ` Jann Horn
2016-10-18 14:56 ` Eric W. Biederman
2016-10-18 15:05 ` Jann Horn
2016-10-18 15:35 ` Eric W. Biederman
2016-10-18 19:12 ` Jann Horn
2016-10-18 21:07 ` Eric W. Biederman
2016-10-18 21:15 ` [REVIEW][PATCH] exec: Don't exec files the userns root can not read Eric W. Biederman
2016-10-19 6:13 ` Amir Goldstein
2016-10-19 13:33 ` Eric W. Biederman
2016-10-19 17:04 ` Eric W. Biederman
2016-10-19 15:30 ` Andy Lutomirski
2016-10-19 16:52 ` Eric W. Biederman
2016-10-19 17:29 ` Jann Horn
2016-10-19 17:32 ` Andy Lutomirski
2016-10-19 17:55 ` Eric W. Biederman
2016-10-19 18:38 ` Andy Lutomirski
2016-10-19 21:26 ` Eric W. Biederman
2016-10-19 23:17 ` Andy Lutomirski
2016-11-17 17:02 ` [REVIEW][PATCH 0/3] Fixing ptrace vs exec vs userns interactions Eric W. Biederman
2016-11-17 17:05 ` [REVIEW][PATCH 1/3] ptrace: Capture the ptracer's creds not PT_PTRACE_CAP Eric W. Biederman
2016-11-17 23:14 ` Kees Cook
2016-11-18 18:56 ` Eric W. Biederman
2016-11-17 23:27 ` Andy Lutomirski
2016-11-17 23:44 ` Eric W. Biederman
2016-11-17 17:08 ` [REVIEW][PATCH 2/3] exec: Don't allow ptracing an exec of an unreadable file Eric W. Biederman
2016-11-17 20:47 ` Willy Tarreau
2016-11-17 21:07 ` Kees Cook
2016-11-17 21:32 ` Willy Tarreau
2016-11-17 21:51 ` Eric W. Biederman
2016-11-17 22:50 ` Eric W. Biederman [this message]
2016-11-17 23:17 ` [REVIEW][PATCH 2/3] ptrace: Don't allow accessing an undumpable mm Kees Cook
2016-11-17 23:28 ` [REVIEW][PATCH 2/3] exec: Don't allow ptracing an exec of an unreadable file Andy Lutomirski
2016-11-17 23:29 ` Andy Lutomirski
2016-11-17 23:55 ` Eric W. Biederman
2016-11-18 0:10 ` Andy Lutomirski
2016-11-18 0:35 ` Eric W. Biederman
2016-11-17 17:10 ` [REVIEW][PATCH 3/3] exec: Ensure mm->user_ns contains the execed files Eric W. Biederman
2016-11-19 7:17 ` [REVIEW][PATCH 0/3] Fixing ptrace vs exec vs userns interactions Willy Tarreau
2016-11-19 9:28 ` Willy Tarreau
2016-11-19 9:33 ` Willy Tarreau
2016-11-19 18:44 ` Eric W. Biederman
2016-11-19 18:35 ` Eric W. Biederman
2016-11-19 18:37 ` Eric W. Biederman
2016-10-19 18:36 ` [REVIEW][PATCH] exec: Don't exec files the userns root can not read Andy Lutomirski
2016-10-18 18:06 ` [REVIEW][PATCH] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access Michal Hocko
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=87shqpzpok.fsf_-_@xmission.com \
--to=ebiederm@xmission.com \
--cc=containers@lists.linux-foundation.org \
--cc=jann@thejh.net \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@amacapital.net \
--cc=mhocko@kernel.org \
--cc=oleg@redhat.com \
--cc=w@1wt.eu \
/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