[PATCH] proc: add config to block FOLL_FORCE in mem writes

[PATCH] proc: add config to block FOLL_FORCE in mem writes

[Adrian Ratiu]

This simple Kconfig option removes the FOLL_FORCE flag from
procfs write calls because it can be abused.

Enabling it breaks some debuggers like GDB so it defaults off.

Previously we tried a more sophisticated approach allowing
distributions to fine-tune proc/pid/mem behaviour via both
kconfig and boot params, however that got NAK-ed by Linus [1]
who prefers this simpler approach.

Link: https://lore.kernel.org/lkml/CAHk-=wiGWLChxYmUA5HrT5aopZrB7_2VTa0NLZcxORgkUe5tEQ@mail.gmail.com/ [1]
Cc: Doug Anderson <dianders@chromium.org>
Cc: Jeff Xu <jeffxu@google.com>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <kees@kernel.org>
Cc: Christian Brauner <brauner@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
---
 fs/proc/base.c   |  6 +++++-
 security/Kconfig | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 72a1acd03675..53ad71d7d785 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -855,7 +855,11 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
 	if (!mmget_not_zero(mm))
 		goto free;
 
-	flags = FOLL_FORCE | (write ? FOLL_WRITE : 0);
+	flags = (write ? FOLL_WRITE : 0);
+
+#ifndef CONFIG_SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE
+	flags |= FOLL_FORCE;
+#endif
 
 	while (count > 0) {
 		size_t this_len = min_t(size_t, count, PAGE_SIZE);
diff --git a/security/Kconfig b/security/Kconfig
index 412e76f1575d..24053b8ff786 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -19,6 +19,20 @@ config SECURITY_DMESG_RESTRICT
 
 	  If you are unsure how to answer this question, answer N.
 
+config SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE
+	bool "Remove FOLL_FORCE usage from /proc/pid/mem writes"
+	default n
+	help
+	  This restricts FOLL_FORCE flag usage in procfs mem write calls
+	  because it bypasses memory permission checks and can be used by
+	  attackers to manipulate process memory contents that would be
+	  otherwise protected.
+
+	  Enabling this will break GDB, gdbserver and other debuggers
+	  which require FOLL_FORCE for basic functionalities.
+
+	  If you are unsure how to answer this question, answer N.
+
 config SECURITY
 	bool "Enable different security models"
 	depends on SYSFS
-- 
2.44.2

Re: [PATCH] proc: add config to block FOLL_FORCE in mem writes

[Kees Cook]

On Wed, Jul 17, 2024 at 02:13:58PM +0300, Adrian Ratiu wrote:
> This simple Kconfig option removes the FOLL_FORCE flag from
> procfs write calls because it can be abused.

For this to be available for general distros, I still want to have a
bootparam to control this, otherwise this mitigation will never see much
testing as most kernel deployments don't build their own kernels. A
simple __ro_after_init variable can be used.

In the future if folks want a more flexible version, we could make this
a one-way per-process flag, like no_new_privs.

-- 
Kees Cook

Re: [PATCH] proc: add config to block FOLL_FORCE in mem writes

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Wed, 17 Jul 2024 at 10:23, Kees Cook <kees@kernel.org> wrote:
>
> For this to be available for general distros, I still want to have a
> bootparam to control this, otherwise this mitigation will never see much
> testing as most kernel deployments don't build their own kernels. A
> simple __ro_after_init variable can be used.

Oh, btw, I looked at the FOLL_FORCE back in 2017 when we did this:

    8ee74a91ac30 ("proc: try to remove use of FOLL_FORCE entirely")

and then we had to undo that with

    f511c0b17b08 (""Yes, people use FOLL_FORCE ;)"")

but at the time I also had an experimental patch that worked for me,
but I seem to have only sent that out in private to the people
involved with the original issue.

And then that whole discussion petered out, and nothing happened.

But maybe we can try again.

In particular, while people piped up about other uses (see the quotes
in that commit f511c0b17b08) they were fairly rare and specialized.

The one *common* use was gdb.

But my old diff from years ago mostly still applies, so I resurrected it.

It basically restricts FOLL_FORCE to just ptracers.

That's *not* good for some of the people that piped up back when (eg
Julia JIT), but it might be a more palatable halfway state.

In particular, this patch would make it easy to make that
SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE config option be a "choice"
where you pick "never, ptrace, always" by just changing the rules in
proc_is_ptracing().

I guess that function should be renamed too, I only did a minimal
"forward-port an old patch" thing.

               Linus

[-- Attachment #2: foll_force.patch --]
[-- Type: text/x-patch, Size: 1081 bytes --]

 fs/proc/base.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 72a1acd03675..1b646cb96509 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -835,6 +835,18 @@ static int mem_open(struct inode *inode, struct file *file)
 	return ret;
 }
 
+static bool proc_is_ptracing(struct file *file, struct mm_struct *mm)
+{
+	bool ptrace_active = false;
+	struct task_struct *task = get_proc_task(file_inode(file));
+
+	if (task) {
+		ptrace_active = task->ptrace && task->mm == mm && task->parent == current;
+		put_task_struct(task);
+	}
+	return ptrace_active;
+}
+
 static ssize_t mem_rw(struct file *file, char __user *buf,
 			size_t count, loff_t *ppos, int write)
 {
@@ -855,7 +867,9 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
 	if (!mmget_not_zero(mm))
 		goto free;
 
-	flags = FOLL_FORCE | (write ? FOLL_WRITE : 0);
+	flags = write ? FOLL_WRITE : 0;
+	if (proc_is_ptracing(file, mm))
+		flags |= FOLL_FORCE;
 
 	while (count > 0) {
 		size_t this_len = min_t(size_t, count, PAGE_SIZE);

Re: [PATCH] proc: add config to block FOLL_FORCE in mem writes

[Kees Cook]

On Wed, Jul 17, 2024 at 11:16:56AM -0700, Linus Torvalds wrote:
> On Wed, 17 Jul 2024 at 10:23, Kees Cook <kees@kernel.org> wrote:
> >
> > For this to be available for general distros, I still want to have a
> > bootparam to control this, otherwise this mitigation will never see much
> > testing as most kernel deployments don't build their own kernels. A
> > simple __ro_after_init variable can be used.
> 
> Oh, btw, I looked at the FOLL_FORCE back in 2017 when we did this:
> 
>     8ee74a91ac30 ("proc: try to remove use of FOLL_FORCE entirely")
> 
> and then we had to undo that with
> 
>     f511c0b17b08 (""Yes, people use FOLL_FORCE ;)"")
> 
> but at the time I also had an experimental patch that worked for me,
> but I seem to have only sent that out in private to the people
> involved with the original issue.
> 
> And then that whole discussion petered out, and nothing happened.
> 
> But maybe we can try again.
> 
> In particular, while people piped up about other uses (see the quotes
> in that commit f511c0b17b08) they were fairly rare and specialized.
> 
> The one *common* use was gdb.
> 
> But my old diff from years ago mostly still applies, so I resurrected it.
> 
> It basically restricts FOLL_FORCE to just ptracers.
> 
> That's *not* good for some of the people that piped up back when (eg
> Julia JIT), but it might be a more palatable halfway state.
> 
> In particular, this patch would make it easy to make that
> SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE config option be a "choice"
> where you pick "never, ptrace, always" by just changing the rules in
> proc_is_ptracing().

So the original patch could be reduced to just the single tristate option
instead of 3 tristates? I think that would be a decent middle ground,
and IIUC, will still provide the coverage Chrome OS is looking for[1].

-Kees

[1] https://lore.kernel.org/lkml/CABi2SkWDwAU2ARyMVTeCqFeOXyQZn3hbkdWv-1OzzgG=MNoU8Q@mail.gmail.com/

-- 
Kees Cook

Re: [PATCH] proc: add config to block FOLL_FORCE in mem writes

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

On Wed, 17 Jul 2024 at 15:24, Kees Cook <kees@kernel.org> wrote:
>
> > In particular, this patch would make it easy to make that
> > SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE config option be a "choice"
> > where you pick "never, ptrace, always" by just changing the rules in
> > proc_is_ptracing().
>
> So the original patch could be reduced to just the single tristate option
> instead of 3 tristates? I think that would be a decent middle ground,
> and IIUC, will still provide the coverage Chrome OS is looking for[1].

So here's what I kind of think might be ok.

ENTIRELY UNTESTED! This is more of a "look, something like this,
perhaps" patch than a real one.

If somebody tests this, and it is ok for Chrome OS, you can consider
this signed-off-on, but only with actual testing. I might have gotten
something hroribly wrong.

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2554 bytes --]

 fs/proc/base.c   | 22 +++++++++++++++++++++-
 security/Kconfig | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 72a1acd03675..fbe9a96c2d98 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -835,6 +835,24 @@ static int mem_open(struct inode *inode, struct file *file)
 	return ret;
 }
 
+static bool proc_mem_foll_force(struct file *file, struct mm_struct *mm)
+{
+#if defined(CONFIG_PROC_MEM_NO_FORCE)
+	return false;
+#elif defined(CONFIG_PROC_MEM_FORCE_PTRACE)
+	bool ptrace_active = false;
+	struct task_struct *task = get_proc_task(file_inode(file));
+
+	if (task) {
+		ptrace_active = task->ptrace && task->mm == mm && task->parent == current;
+		put_task_struct(task);
+	}
+	return ptrace_active;
+#else
+	return true;
+#endif
+}
+
 static ssize_t mem_rw(struct file *file, char __user *buf,
 			size_t count, loff_t *ppos, int write)
 {
@@ -855,7 +873,9 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
 	if (!mmget_not_zero(mm))
 		goto free;
 
-	flags = FOLL_FORCE | (write ? FOLL_WRITE : 0);
+	flags = write ? FOLL_WRITE : 0;
+	if (proc_mem_foll_force(file, mm))
+		flags |= FOLL_FORCE;
 
 	while (count > 0) {
 		size_t this_len = min_t(size_t, count, PAGE_SIZE);
diff --git a/security/Kconfig b/security/Kconfig
index 412e76f1575d..b201ae3feeab 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -19,6 +19,38 @@ config SECURITY_DMESG_RESTRICT
 
 	  If you are unsure how to answer this question, answer N.
 
+choice
+	prompt "Allow /proc/pid/mem access override"
+	default PROC_PID_MEM_ALWAYS_FORCE
+	help
+	  Traditionally /proc/pid/mem allows users to override memory
+	  permissions for users like ptrace, assuming they have ptrace
+	  capability.
+
+	  This allows people to limit that - either never override, or
+	  require actual active ptrace attachment.
+
+	  Defaults to the traditional behavior (for now)
+
+config PROC_PID_MEM_ALWAYS_FORCE
+	bool "Traditional /proc/pid/mem behavior"
+	help
+	  This allows /proc/pid/mem accesses to override memory mapping
+	  permissions if you have ptrace access rights.
+
+config CONFIG_PROC_MEM_FORCE_PTRACE
+	bool "Require active ptrace() use for access override"
+	help
+	  This allows /proc/pid/mem accesses to override memory mapping
+	  permissions for active ptracers like gdb.
+
+config CONFIG_PROC_MEM_NO_FORCE
+	bool "Never"
+	help
+	  Never override memory mapping permissions
+
+endchoice
+
 config SECURITY
 	bool "Enable different security models"
 	depends on SYSFS

Re: [PATCH] proc: add config to block FOLL_FORCE in mem writes

[Adrian Ratiu]

On Thursday, July 18, 2024 03:04 EEST, Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 17 Jul 2024 at 15:24, Kees Cook <kees@kernel.org> wrote:
> >
> > > In particular, this patch would make it easy to make that
> > > SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE config option be a "choice"
> > > where you pick "never, ptrace, always" by just changing the rules in
> > > proc_is_ptracing().
> >
> > So the original patch could be reduced to just the single tristate option
> > instead of 3 tristates? I think that would be a decent middle ground,
> > and IIUC, will still provide the coverage Chrome OS is looking for[1].
> 
> So here's what I kind of think might be ok.
> 
> ENTIRELY UNTESTED! This is more of a "look, something like this,
> perhaps" patch than a real one.
> 
> If somebody tests this, and it is ok for Chrome OS, you can consider
> this signed-off-on, but only with actual testing. I might have gotten
> something hroribly wrong.

Thanks for the patch!

I tested it on ChromeOS and it does what it intends, just with two
minor fixes applied:

--- a/security/Kconfig
+++ b/security/Kconfig
-config CONFIG_PROC_MEM_FORCE_PTRACE
+config PROC_MEM_FORCE_PTRACE
.....
 -config CONFIG_PROC_MEM_NO_FORCE
+config PROC_MEM_NO_FORCE

As Kees suggested, I'll add a bootparam with a simple __ro_after_init
variable to select this and then send a v2 for review.

Re: [PATCH] proc: add config to block FOLL_FORCE in mem writes

[Eric Biggers]

On Wed, Jul 17, 2024 at 02:13:58PM +0300, Adrian Ratiu wrote:
> +config SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE
> +	bool "Remove FOLL_FORCE usage from /proc/pid/mem writes"
> +	default n
> +	help
> +	  This restricts FOLL_FORCE flag usage in procfs mem write calls
> +	  because it bypasses memory permission checks and can be used by
> +	  attackers to manipulate process memory contents that would be
> +	  otherwise protected.
> +
> +	  Enabling this will break GDB, gdbserver and other debuggers
> +	  which require FOLL_FORCE for basic functionalities.
> +
> +	  If you are unsure how to answer this question, answer N.

FOLL_FORCE is an internal flag, and people who aren't kernel developers aren't
going to know what it is.  Could this option be named and documented in a way
that would be more understandable to people who aren't kernel developers?  What
is the effect on how /proc/pid/mem behaves?

- Eric

Re: [PATCH] proc: add config to block FOLL_FORCE in mem writes

[Kees Cook]

On Wed, Jul 17, 2024 at 01:53:35PM -0700, Eric Biggers wrote:
> On Wed, Jul 17, 2024 at 02:13:58PM +0300, Adrian Ratiu wrote:
> > +config SECURITY_PROC_MEM_RESTRICT_FOLL_FORCE
> > +	bool "Remove FOLL_FORCE usage from /proc/pid/mem writes"
> > +	default n
> > +	help
> > +	  This restricts FOLL_FORCE flag usage in procfs mem write calls
> > +	  because it bypasses memory permission checks and can be used by
> > +	  attackers to manipulate process memory contents that would be
> > +	  otherwise protected.
> > +
> > +	  Enabling this will break GDB, gdbserver and other debuggers
> > +	  which require FOLL_FORCE for basic functionalities.
> > +
> > +	  If you are unsure how to answer this question, answer N.
> 
> FOLL_FORCE is an internal flag, and people who aren't kernel developers aren't
> going to know what it is.  Could this option be named and documented in a way
> that would be more understandable to people who aren't kernel developers?  What
> is the effect on how /proc/pid/mem behaves?

"Do not bypass RO memory permissions via /proc/$pid/mem writes" ?

-- 
Kees Cook