Re: [PATCH v2 00/39] Shadowstacks for userspace
From: Kees Cook <hidden>
Date: 2022-10-04 19:28:19
Also in:
linux-arch, linux-doc, linux-mm, lkml
Subsystem:
filesystems (vfs and infrastructure), proc filesystem, ptrace support, the rest · Maintainers:
Alexander Viro, Christian Brauner, Oleg Nesterov, Linus Torvalds
On Tue, Oct 04, 2022 at 09:57:48AM +0000, David Laight wrote:
From: Kees Cook <redacted> ...quoted
quoted
If you don't want /proc/$pid/mem to be able to do stuff like that, then IMO the way to go is to change when /proc/$pid/mem uses FOLL_FORCE, or to limit overall write access to /proc/$pid/mem.Yeah, all reasonable. I just wish we could ditch FOLL_FORCE; it continues to weird me out how powerful that fd's side-effects are.Could you remove FOLL_FORCE from /proc/$pid/mem and add a /proc/$pid/mem_force that enable FOLL_FORCE but requires root (or similar) access. Although I suspect gdb may like to have write access to code?
As Jann has reminded me out of band, while FOLL_FORCE is still worrisome, it's really /proc/$pid/mem access at all without an active ptrace attachment (and to self). Here's my totally untested idea to require access to /proc/$pid/mem having an established ptrace relationship:
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index c952c5ba8fab..0393741eeabb 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h@@ -64,6 +64,7 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead); #define PTRACE_MODE_NOAUDIT 0x04 #define PTRACE_MODE_FSCREDS 0x08 #define PTRACE_MODE_REALCREDS 0x10 +#define PTRACE_MODE_ATTACHED 0x20 /* shorthands for READ/ATTACH and FSCREDS/REALCREDS combinations */ #define PTRACE_MODE_READ_FSCREDS (PTRACE_MODE_READ | PTRACE_MODE_FSCREDS)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 93f7e3d971e4..fadec587d133 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c@@ -826,7 +826,7 @@ static int __mem_open(struct inode *inode, struct file *file, unsigned int mode) static int mem_open(struct inode *inode, struct file *file) { - int ret = __mem_open(inode, file, PTRACE_MODE_ATTACH); + int ret = __mem_open(inode, file, PTRACE_MODE_ATTACHED); /* OK to pass negative loff_t, we can catch out-of-range */ file->f_mode |= FMODE_UNSIGNED_OFFSET;
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 1893d909e45c..c97e6d734ae5 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c@@ -304,6 +304,12 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode) * or halting the specified task is impossible. */ + /* + * If an existing ptrace relationship is required, not even + * introspection is allowed. + */ + if ((mode & PTRACE_MODE_ATTACHED) && ptrace_parent(task) != current) + return -EPERM; /* Don't let security modules deny introspection */ if (same_thread_group(task, current)) return 0;
--
Kees Cook