diff mbox series

proc: add config to block FOLL_FORCE in mem writes

Message ID 20240717111358.415712-1-adrian.ratiu@collabora.com (mailing list archive)
State New, archived
Headers show
Series proc: add config to block FOLL_FORCE in mem writes | expand

Commit Message

Adrian Ratiu July 17, 2024, 11:13 a.m. UTC
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(-)

Comments

Kees Cook July 17, 2024, 5:22 p.m. UTC | #1
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.
Linus Torvalds July 17, 2024, 6:16 p.m. UTC | #2
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
Eric Biggers July 17, 2024, 8:53 p.m. UTC | #3
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
Kees Cook July 17, 2024, 9:28 p.m. UTC | #4
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 July 17, 2024, 10:23 p.m. UTC | #5
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/
Linus Torvalds July 18, 2024, 12:04 a.m. UTC | #6
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
Adrian Ratiu July 18, 2024, 3:58 p.m. UTC | #7
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.
diff mbox series

Patch

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